cmd/cgo: use --no-gc-sections if available #52815

motiejus · 2022-05-10T11:42:53Z

zig cc passes --gc-sections to the underlying linker, which then
causes undefined symbol errors when compiling with cgo but without C
code. Add -Wl,--no-gc-sections to make it work with zig cc. Minimal
example:

main.go

package main
import _ "runtime/cgo"
func main() {}

Run (works after the patch, doesn't work before):

CC="zig cc" go build main.go

Among the existing code, src/runtime/testdata/testprognet fails to
build:

src/runtime/testdata/testprognet$ CC="zig cc" go build .
net(.text): relocation target __errno_location not defined
net(.text): relocation target getaddrinfo not defined
net(.text): relocation target freeaddrinfo not defined
net(.text): relocation target gai_strerror not defined
runtime/cgo(.text): relocation target stderr not defined
runtime/cgo(.text): relocation target fwrite not defined
runtime/cgo(.text): relocation target vfprintf not defined
runtime/cgo(.text): relocation target fputc not defined
runtime/cgo(.text): relocation target abort not defined
runtime/cgo(.text): relocation target pthread_create not defined
runtime/cgo(.text): relocation target nanosleep not defined
runtime/cgo(.text): relocation target pthread_detach not defined
runtime/cgo(.text): relocation target stderr not defined
runtime/cgo(.text): relocation target strerror not defined
runtime/cgo(.text): relocation target fprintf not defined
runtime/cgo(.text): relocation target abort not defined
runtime/cgo(.text): relocation target pthread_mutex_lock not defined
runtime/cgo(.text): relocation target pthread_cond_wait not defined
runtime/cgo(.text): relocation target pthread_mutex_unlock not defined
runtime/cgo(.text): relocation target pthread_cond_broadcast not defined
runtime/cgo(.text): relocation target malloc not defined

With the patch both examples build as expected.

@ianlancetaylor suggested:

It would be fine with me if somebody wants to send a cgo patch that
passes -Wl,--no-gc-sections, with a fallback if that option is not
supported.

... and this is what we are doing. Tested with zig
0.10.0-dev.2252+a4369918b

Fixes #52690

gopherbot · 2022-05-10T11:49:07Z

This PR (HEAD: 136bf8f) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/405414 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2022-05-10T11:53:03Z

Message from Gopher Robot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
A maintainer will review your change and provide feedback. See
https://go.dev/doc/contribute#review for more info and tips to get your
patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11 or adds a tag like "wait-release", it means that this CL will be
reviewed as part of the next development cycle. See https://go.dev/s/release
for more details.

Please don’t reply on this GitHub thread. Visit golang.org/cl/405414.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2022-05-10T15:43:22Z

Message from Bryan Mills:

Patch Set 1: Run-TryBot+1 Code-Review+1

(2 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/405414.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2022-05-10T15:43:22Z

Message from Gopher Robot:

Patch Set 1:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/405414.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2022-05-10T15:59:12Z

Message from Gopher Robot:

Patch Set 1:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/405414.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2022-05-10T16:06:56Z

Message from Gopher Robot:

Patch Set 1: TryBot-Result-1

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/405414.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2022-05-10T16:14:53Z

Message from Bryan Mills:

Patch Set 1:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/405414.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2022-05-10T16:18:50Z

Message from Gopher Robot:

Patch Set 1:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/405414.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2022-05-10T16:26:49Z

Message from Gopher Robot:

Patch Set 1:

(2 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/405414.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2022-05-10T16:34:33Z

Message from Gopher Robot:

Patch Set 1:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/405414.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2022-05-10T16:38:32Z

Message from Bryan Mills:

Patch Set 1:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/405414.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2022-05-10T16:46:38Z

Message from Gopher Robot:

Patch Set 1: TryBot-Result-1

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/405414.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2022-05-10T21:53:52Z

Message from Ian Lance Taylor:

Patch Set 1:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/405414.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2022-05-11T18:44:42Z

Message from Gopher Robot:

Patch Set 1:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/405414.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2022-05-11T18:48:34Z

Message from Gopher Robot:

Patch Set 1:

(2 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/405414.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2022-05-11T19:07:40Z

Message from Gopher Robot:

Patch Set 1: TryBot-Result+1

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/405414.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2022-05-11T19:07:40Z

Message from Bryan Mills:

Patch Set 1:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/405414.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2022-05-17T14:24:45Z

Message from Motiejus Jakštys:

Patch Set 1:

(2 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/405414.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2022-05-17T21:13:32Z

Message from Bryan Mills:

Patch Set 1:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/405414.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2022-05-18T11:07:31Z

This PR (HEAD: e42ccc7) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/405414 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2022-05-18T11:25:07Z

Message from Motiejus Jakštys:

Patch Set 2:

(2 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/405414.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2022-05-18T19:19:05Z

Message from Ian Lance Taylor:

Patch Set 2:

(5 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/405414.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2022-05-19T02:48:12Z

This PR (HEAD: 856a2e0) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/405414 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2022-05-19T02:52:14Z

This PR (HEAD: 55004bc) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/405414 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2022-05-19T02:59:58Z

Message from Motiejus Jakštys:

Patch Set 5:

(6 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/405414.
After addressing review feedback, remember to publish your drafts!

As of 742dcba `go build` can accept `$CC` with spaces and quotes, which lets us easily use `zig cc` as the C compiler, or easily pass extra compiler parameters: ``` CC="zig cc" go build <...> CC="clang-13 -v" go build <...> CC="zig cc -Wl,--print-gc-sections" go build <...> ``` However, the same does not apply for building go itself: ``` $ CC="zig cc" ./make.bash Building Go cmd/dist using /usr/local/go. (go1.18.2 linux/amd64) go tool dist: cannot invoke C compiler "zig cc": exec: "zig cc": executable file not found in $PATH Go needs a system C compiler for use with cgo. To set a C compiler, set CC=the-compiler. To disable cgo, set CGO_ENABLED=0. ``` With this change Go can be built directly with `zig cc` (the linker arg will disappear with golang#52815 and/or golang#52690): ``` $ CC="zig cc -Wl,--no-gc-sections" ./make.bash Building Go cmd/dist using /usr/local/go. (go1.18.2 linux/amd64) Building Go toolchain1 using /usr/local/go. Building Go bootstrap cmd/go (go_bootstrap) using Go toolchain1. Building Go toolchain2 using go_bootstrap and Go toolchain1. Building Go toolchain3 using go_bootstrap and Go toolchain2. Building packages and commands for linux/amd64. --- Installed Go for linux/amd64 in /home/motiejus/code/go Installed commands in /home/motiejus/code/go/bin $ ../bin/go version go version devel go1.19-811f1913a8 Thu May 19 09:44:49 2022 +0300 linux/amd64 ``` Fixes golang#52990

clang and gcc accept `-r`, which is a `--relocatable` argument of ld.lld. This diff understands the flag and passes to the linker. Impact: this makes the Go's `cmd/link` test suite pass[*]. To reproduce, do in go/src directory (currently patched, hopefully not for long): ``` echo 'exec zig cc "$@"' > zig-cc; chmod a+x zig-cc CC="./zig-cc" ./run.bash -run cmd/link ``` [*]: at the moment it requires golang/go#52815 to pass

gopherbot · 2022-05-19T23:15:08Z

Message from Ian Lance Taylor:

Patch Set 5:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/405414.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2022-05-19T23:15:09Z

Message from Ian Lance Taylor:

Patch Set 5:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/405414.
After addressing review feedback, remember to publish your drafts!

@ianlancetaylor

zig cc passes `--gc-sections` to the underlying linker, which then causes undefined symbol errors when compiling with cgo but without C code. Add `-Wl,--no-gc-sections` to make it work with zig cc. Minimal example: **main.go** package main import _ "runtime/cgo" func main() {} Run (works after the patch, doesn't work before): CC="zig cc" go build main.go Among the existing code, `src/runtime/testdata/testprognet` fails to build: src/runtime/testdata/testprognet$ CC="zig cc" go build . net(.text): relocation target __errno_location not defined net(.text): relocation target getaddrinfo not defined net(.text): relocation target freeaddrinfo not defined net(.text): relocation target gai_strerror not defined runtime/cgo(.text): relocation target stderr not defined runtime/cgo(.text): relocation target fwrite not defined runtime/cgo(.text): relocation target vfprintf not defined runtime/cgo(.text): relocation target fputc not defined runtime/cgo(.text): relocation target abort not defined runtime/cgo(.text): relocation target pthread_create not defined runtime/cgo(.text): relocation target nanosleep not defined runtime/cgo(.text): relocation target pthread_detach not defined runtime/cgo(.text): relocation target stderr not defined runtime/cgo(.text): relocation target strerror not defined runtime/cgo(.text): relocation target fprintf not defined runtime/cgo(.text): relocation target abort not defined runtime/cgo(.text): relocation target pthread_mutex_lock not defined runtime/cgo(.text): relocation target pthread_cond_wait not defined runtime/cgo(.text): relocation target pthread_mutex_unlock not defined runtime/cgo(.text): relocation target pthread_cond_broadcast not defined runtime/cgo(.text): relocation target malloc not defined With the patch both examples build as expected. @ianlancetaylor suggested: > It would be fine with me if somebody wants to send a cgo patch that passes -Wl,--no-gc-sections, with a fallback if that option is not supported. ... and this is what we are doing. Tested with zig 0.10.0-dev.2252+a4369918b Fixes golang#52690

As of CL 334732 `go build` can accept `$CC` with spaces and quotes, which lets us easily use `zig cc` as the C compiler, or easily pass extra compiler parameters: ``` CC="zig cc" go build <...> CC="clang-13 -v" go build <...> CC="zig cc -Wl,--print-gc-sections" go build <...> ``` However, the same does not apply for building go itself: ``` $ CC="zig cc" ./make.bash Building Go cmd/dist using /usr/local/go. (go1.18.2 linux/amd64) go tool dist: cannot invoke C compiler "zig cc": exec: "zig cc": executable file not found in $PATH Go needs a system C compiler for use with cgo. To set a C compiler, set CC=the-compiler. To disable cgo, set CGO_ENABLED=0. ``` With this change Go can be built directly with `zig cc` (the linker arg will disappear with golang#52815 and/or golang#52690): ``` $ CC="zig cc -Wl,--no-gc-sections" ./make.bash Building Go cmd/dist using /usr/local/go. (go1.18.2 linux/amd64) Building Go toolchain1 using /usr/local/go. Building Go bootstrap cmd/go (go_bootstrap) using Go toolchain1. Building Go toolchain2 using go_bootstrap and Go toolchain1. Building Go toolchain3 using go_bootstrap and Go toolchain2. Building packages and commands for linux/amd64. --- Installed Go for linux/amd64 in /home/motiejus/code/go Installed commands in /home/motiejus/code/go/bin $ ../bin/go version go version devel go1.19-811f1913a8 Thu May 19 09:44:49 2022 +0300 linux/amd64 ``` Fixes golang#52990

gopherbot · 2022-05-20T07:28:21Z

Message from Motiejus Jakštys:

Patch Set 5:

(2 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/405414.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2022-05-20T07:28:28Z

This PR (HEAD: 58406b3) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/405414 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2022-05-20T18:02:19Z

Message from Ian Lance Taylor:

Patch Set 6: Run-TryBot+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/405414.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2022-05-20T18:09:59Z

Message from Gopher Robot:

Patch Set 6:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/405414.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2022-05-20T18:28:31Z

Message from Gopher Robot:

Patch Set 6: TryBot-Result+1

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/405414.
After addressing review feedback, remember to publish your drafts!

@ianlancetaylor

zig cc passes `--gc-sections` to the underlying linker, which then causes undefined symbol errors when compiling with cgo but without C code. Add `-Wl,--no-gc-sections` to make it work with zig cc. Minimal example: **main.go** package main import _ "runtime/cgo" func main() {} Run (works after the patch, doesn't work before): CC="zig cc" go build main.go Among the existing code, `src/runtime/testdata/testprognet` fails to build: src/runtime/testdata/testprognet$ CC="zig cc" go build . net(.text): relocation target __errno_location not defined net(.text): relocation target getaddrinfo not defined net(.text): relocation target freeaddrinfo not defined net(.text): relocation target gai_strerror not defined runtime/cgo(.text): relocation target stderr not defined runtime/cgo(.text): relocation target fwrite not defined runtime/cgo(.text): relocation target vfprintf not defined runtime/cgo(.text): relocation target fputc not defined runtime/cgo(.text): relocation target abort not defined runtime/cgo(.text): relocation target pthread_create not defined runtime/cgo(.text): relocation target nanosleep not defined runtime/cgo(.text): relocation target pthread_detach not defined runtime/cgo(.text): relocation target stderr not defined runtime/cgo(.text): relocation target strerror not defined runtime/cgo(.text): relocation target fprintf not defined runtime/cgo(.text): relocation target abort not defined runtime/cgo(.text): relocation target pthread_mutex_lock not defined runtime/cgo(.text): relocation target pthread_cond_wait not defined runtime/cgo(.text): relocation target pthread_mutex_unlock not defined runtime/cgo(.text): relocation target pthread_cond_broadcast not defined runtime/cgo(.text): relocation target malloc not defined With the patch both examples build as expected. @ianlancetaylor suggested: > It would be fine with me if somebody wants to send a cgo patch that passes -Wl,--no-gc-sections, with a fallback if that option is not supported. ... and this is what we are doing. Tested with zig 0.10.0-dev.2252+a4369918b Fixes #52690 Change-Id: Ib6d1b2bd59335e9663afefd360ddad7da358a938 GitHub-Last-Rev: 58406b3 GitHub-Pull-Request: #52815 Reviewed-on: https://go-review.googlesource.com/c/go/+/405414 Reviewed-by: Ian Lance Taylor <[email protected]> Auto-Submit: Ian Lance Taylor <[email protected]> Run-TryBot: Ian Lance Taylor <[email protected]> Run-TryBot: Ian Lance Taylor <[email protected]> Reviewed-by: Bryan Mills <[email protected]> TryBot-Result: Gopher Robot <[email protected]>

gopherbot · 2022-05-20T21:08:11Z

This PR is being closed because golang.org/cl/405414 has been merged.

ianlancetaylor · 2022-05-20T22:59:01Z

Rolling back the fix, because it broke on the AIX and iOS builders.

ianlancetaylor · 2022-05-20T23:00:20Z

Sorry, I got confused and thought this was an issue rather than a pull request.

motiejus force-pushed the no-gc-sections branch from 136bf8f to e42ccc7 Compare May 18, 2022 11:05

motiejus force-pushed the no-gc-sections branch from e42ccc7 to 856a2e0 Compare May 19, 2022 02:46

motiejus force-pushed the no-gc-sections branch from 856a2e0 to 55004bc Compare May 19, 2022 02:51

motiejus mentioned this pull request May 19, 2022

cmd/dist: support spaces and quotes in CC #52991

Closed

motiejus mentioned this pull request May 19, 2022

[ld.lld] understand -r, --relocatable ziglang/zig#11678

Closed

motiejus force-pushed the no-gc-sections branch from 55004bc to 471af45 Compare May 20, 2022 07:22

motiejus force-pushed the no-gc-sections branch from 471af45 to 58406b3 Compare May 20, 2022 07:24

gopherbot closed this May 20, 2022

ianlancetaylor reopened this May 20, 2022

ianlancetaylor closed this May 20, 2022

motiejus deleted the no-gc-sections branch May 24, 2022 08:00

cmd/cgo: use --no-gc-sections if available #52815

cmd/cgo: use --no-gc-sections if available #52815

Conversation

motiejus commented May 10, 2022 • edited Loading

gopherbot commented May 10, 2022

gopherbot commented May 10, 2022

gopherbot commented May 10, 2022

gopherbot commented May 10, 2022

gopherbot commented May 10, 2022

gopherbot commented May 10, 2022

gopherbot commented May 10, 2022

gopherbot commented May 10, 2022

gopherbot commented May 10, 2022

gopherbot commented May 10, 2022

gopherbot commented May 10, 2022

gopherbot commented May 10, 2022

gopherbot commented May 10, 2022

gopherbot commented May 11, 2022

gopherbot commented May 11, 2022

gopherbot commented May 11, 2022

gopherbot commented May 11, 2022

gopherbot commented May 17, 2022

gopherbot commented May 17, 2022

gopherbot commented May 18, 2022

gopherbot commented May 18, 2022

gopherbot commented May 18, 2022

gopherbot commented May 19, 2022

gopherbot commented May 19, 2022

gopherbot commented May 19, 2022

gopherbot commented May 19, 2022

gopherbot commented May 19, 2022

gopherbot commented May 20, 2022

gopherbot commented May 20, 2022

gopherbot commented May 20, 2022

gopherbot commented May 20, 2022

gopherbot commented May 20, 2022

gopherbot commented May 20, 2022

ianlancetaylor commented May 20, 2022

ianlancetaylor commented May 20, 2022

motiejus commented May 10, 2022 •

edited

Loading