runtime/debug: TestStack fails when GOROOT_FINAL or GOROOT is not clean

[lacombar]

[same environment as #62620]

$ cd src
$ GOROOT_FINAL=/final/destination/ ./all.bash

[...]
ok      runtime/coverage        0.003s
allocating objects
starting gc
starting dump
done dump
--- FAIL: TestStack (0.00s)
    stack_test.go:73: found GOROOT "/src/go" from environment; checking embedded GOROOT value
    stack_test.go:116: in line "\t/final/destination/src/runtime/debug/stack.go:24 +0x5e", expected prefix "\t/final/destination//src/runtime/debug/stack.go"
    stack_test.go:117: in line "\t/final/destination/src/runtime/debug/stack_test.go:31", expected prefix "\t/final/destination//src/runtime/debug/stack_test.go"
    stack_test.go:118: in line "\t/final/destination/src/runtime/debug/stack_test.go:34", expected prefix "\t/final/destination//src/runtime/debug/stack_test.go"
    stack_test.go:119: in line "\t/final/destination/src/runtime/debug/stack_test.go:56 +0x2c", expected prefix "\t/final/destination//src/runtime/debug/stack_test.go"
    stack_test.go:120: in line "\t/final/destination/src/testing/testing.go:1595 +0xff", expected prefix "\t/final/destination//src/testing/testing.go"
FAIL
FAIL    runtime/debug   0.073s
[...]

Notice the expected prefix' double path separator, /final/destination//src/, removing the trailing / from the environment variable works.

[bcmills]

In general you need to set GOROOT_FINAL to a clean path.

That said, as of Go 1.21 there isn't much point in setting GOROOT_FINAL at all (see #62047).

[bcmills]

I wonder, though: perhaps cmd/link should call filepath.Clean or similar on the value obtained from the environment?
(https://cs.opensource.google/go/go/+/master:src/cmd/link/internal/ld/pcln.go;l=813;drc=0460c61e5fd2242d27451f527dafc4d9a098fef4)

[prattmic]

It couldn’t quite be filepath, right? A Linux build of the linker cross compiling for Windows would need to clean a Windows path. We need path/filepath/windows, etc.

[bcmills]

In theory, yes.

In practice, I don't actually know how the embedded GOROOT value interacts with cross-compilation. 😅

[cherrymui]

I feel that the linker doesn't need to understand the meaning of GOROOT_FINAL. Could the go command clean it up before passing it to the linker?

[bcmills]

At the moment cmd/go isn't doing anything to it unless -trimpath is set:
https://cs.opensource.google/go/go/+/master:src/cmd/go/internal/work/gc.go;l=649;drc=fbcf43c60ba5170309a238b0e42fd5879d419776
it just assumes that the linker will do something with whatever value is in the environment:
https://cs.opensource.google/go/go/+/master:src/cmd/go/internal/work/exec.go;l=1389-1395;drc=3857a89e7eb872fa22d569e70b7e076bec74ebbb

I agree that if we keep GOROOT_FINAL it would be reasonable for cmd/go to clean it up in the environment. (That would at least fix invocations via go build, although it would still leave a bit of a gap for cmd/link invocations via third-party build tools.)

[cherrymui]

I wonder why we write "$GOROOT" at compile time and expand to the actual GOROOT at link time... I guess this may be useful previously when we had compiled .a files in GOROOT. If one moves GOROOT after compilation, those .a files are still valid. But now we don't have those .a files anymore. We probably could write the actual GOROOT (GOROOT_FINAL, if set) at compile time. Then we don't need to expand it in the linker. (We'd need to recompile if GOROOT_FINAL changes, previously we only need to relink. But that seems pretty minor.)

[iwdgo]

GOROOT_FINAL has been removed following #62047 and the referred code uses a cleaned GOROOT path as it ends with a filepath.Join when GOROOT is set.
All comments seem answered.

func expandGoroot(s string) string {
	const n = len("$GOROOT")
	if len(s) >= n+1 && s[:n] == "$GOROOT" && (s[n] == '/' || s[n] == '\\') {
		if final := buildcfg.GOROOT; final != "" {
			return filepath.ToSlash(filepath.Join(final, s[n:]))
		}
	}
	return s
}

https://cs.opensource.google/go/go/+/master:src/cmd/link/internal/ld/pcln.go;drc=507d1b22f4b58ac68841582d0c2c0ab6b20e5a98;l=810