Skip to content

cmd/link: wrong flag order when checking supported linker flags #69350

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
siddharthab opened this issue Sep 9, 2024 · 5 comments
Closed

cmd/link: wrong flag order when checking supported linker flags #69350

siddharthab opened this issue Sep 9, 2024 · 5 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@siddharthab
Copy link
Contributor

Go version

1.23.1

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/root/.cache/go-build'
GOENV='/root/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/root/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/root/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/goroot'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/goroot/pkg/tool/linux_arm64'
GOVCS=''
GOVERSION='go1.23.1'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/root/.config/go/telemetry'
GCCGO='gccgo'
GOARM64='v8.0'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/hyperbase/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build4149942144=/tmp/go-build -gno-record-gcc-switches'

What did you do?

There can probably be a simpler repro, but I am presenting here as I discovered the issue. The context is Bazel rules for Go. As per bazel-contrib/rules_go#4091, I used the repro in https://github.com/DavidZbarsky-at/cgo-repro.

To further investigate the issue, I patched the source code for linkerFlagSupported to print the linker command being executed.

diff --git a/src/cmd/link/internal/ld/lib.go b/src/cmd/link/internal/ld/lib.go
index d66027387b..f528400a7e 100644
--- a/src/cmd/link/internal/ld/lib.go
+++ b/src/cmd/link/internal/ld/lib.go
@@ -2139,4 +2139,5 @@ func linkerFlagSupported(arch *sys.Arch, linker, altLinker, flag string) bool {
        cmd.Env = append([]string{"LC_ALL=C"}, os.Environ()...)
        out, err := cmd.CombinedOutput()
+       fmt.Printf("checking %s (%v %v) (%v): %s\n", flag, linker, strings.Join(flags, " "), err, out)
        // GCC says "unrecognized command line option <E2><80><98>-no-pie<E2><80><99>"
        // clang says "unknown argument: '-no-pie'"

What did you see happen?

All invocations of linkerFlagSupported fail because of the wrong order of linker flags:

The patched print statement reveals the following information:

checking -no-pie: external/toolchains_llvm~~llvm~llvm_toolchain/bin/cc_wrapper.sh -fuse-ld=lld -Wl,--build-id=md5 -Wl,--hash-style=gnu -Wl,-z,relro,-z,now --sysroot=external/_main~_repo_rules~org_chromium_sysroot_linux_aarch64/ -fuse-ld=lld -Wl,--build-id=md5 -Wl,--hash-style=gnu -Wl,-z,relro,-z,now --sysroot=__GO_BAZEL_CC_PLACEHOLDER__external/_main~_repo_rules~org_chromium_sysroot_linux_aarch64/ -pthread -fuse-ld=lld -Wl,--build-id=md5 -Wl,--hash-style=gnu -Wl,-z,relro,-z,now --sysroot=__GO_BAZEL_CC_PLACEHOLDER__external/_main~_repo_rules~org_chromium_sysroot_linux_aarch64/ -pthread -o /tmp/go-link-2636548368/a.out -no-pie /tmp/go-link-2636548368/trivial.c (exit status 1): ld.lld: error: cannot open crt1.o: No such file or directory

Notice the multiple values of --sysroot. The first value is coming from extldflags, and the two later values are coming from CGo directives from stdlib objects being linked in.

What did you expect to see?

CGo directives should get less priority than extldflags, and so should be placed before.

This is because of how flags are appended in

go/src/cmd/link/internal/ld/lib.go

Line 2128 in 807e01d

moreFlags := trimLinkerArgv(append(flagExtldflags, ldflag...))

Consider the correct behavior in the final link command - extldflags after CGo directives (given by the variable ldflag):

go/src/cmd/link/internal/ld/lib.go

Lines 1857 to 1882 in 807e01d

for _, p := range ldflag {
argv = append(argv, p)
checkStatic(p)
}
// When building a program with the default -buildmode=exe the
// gc compiler generates code requires DT_TEXTREL in a
// position independent executable (PIE). On systems where the
// toolchain creates PIEs by default, and where DT_TEXTREL
// does not work, the resulting programs will not run. See
// issue #17847. To avoid this problem pass -no-pie to the
// toolchain if it is supported.
if ctxt.BuildMode == BuildModeExe && !ctxt.linkShared && !(ctxt.IsDarwin() && ctxt.IsARM64()) {
// GCC uses -no-pie, clang uses -nopie.
for _, nopie := range []string{"-no-pie", "-nopie"} {
if linkerFlagSupported(ctxt.Arch, argv[0], altLinker, nopie) {
argv = append(argv, nopie)
break
}
}
}
for _, p := range flagExtldflags {
argv = append(argv, p)
checkStatic(p)
}

The fix can be as simple as to change the above line in linkerFlagSupported to:

moreFlags := []string{}
moreFlags = append(moreFlags, ldflag...)
moreFlags = append(moreFlags, flagExtldflags...)
moreFlags = trimLinkerArgv(moreFlags)
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Sep 9, 2024
@gabyhelp
Copy link

gabyhelp commented Sep 9, 2024

Related Issues and Documentation

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@siddharthab siddharthab mentioned this issue Sep 9, 2024
Open
@timothy-king timothy-king added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 9, 2024
@timothy-king timothy-king added this to the Backlog milestone Sep 9, 2024
@timothy-king
Copy link
Contributor

CC @golang/compiler, @cherrymui.

@mknyszek
Copy link
Contributor

@siddharthab Thanks for the detailed bug report and the possible fix! Would you be willing to send a patch? Thanks! See https://go.dev/doc/contribute.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/614275 mentions this issue: cmd/link: fix flags order in linkerFlagSupported

@siddharthab
Copy link
Contributor Author

Thanks @mknyszek for considering this. The sent Gerrit CR should resolve this issue.

@gabyhelp gabyhelp mentioned this issue Oct 20, 2024
Closed
@voxeljorge voxeljorge mentioned this issue Oct 20, 2024
Merged
@github-project-automation github-project-automation bot moved this from Todo to Done in Go Compiler / Runtime Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Go Compiler / Runtime
Status: Done
Milestone
Backlog
Development

No branches or pull requests
5 participants
@timothy-king @mknyszek @siddharthab @gopherbot @gabyhelp