cmd/link: load symbols from .syso in external link mode #33140

jpap · 2019-07-16T23:25:31Z

Fix linking with a package having a .syso file in external link mode,
that would otherwise cause an error before executing the external
linker because it can't find symbols that are exported in the said
.syso file.

Fixes #33139

gopherbot · 2019-07-16T23:40:57Z

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

Please visit https://go-review.googlesource.com/c/go/+/186417 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 · 2019-07-16T23:42:34Z

Message from Gobot Gobot:

Patch Set 1:

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

Next steps:
Within the next week or so, a maintainer will review your change and provide
feedback. See https://golang.org/doc/contribute.html#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, it means that this CL will be reviewed as part of the next development
cycle. See https://golang.org/s/release for more details.

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

gopherbot · 2019-07-17T00:53:29Z

Message from Ian Lance Taylor:

Patch Set 1:

A change like this needs a test case, likely somewhere under misc/cgo.

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

gopherbot · 2019-07-17T07:22:32Z

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

Please visit https://go-review.googlesource.com/c/go/+/186417 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 · 2019-07-17T07:32:25Z

Message from John Papandriopoulos:

Patch Set 2:

Patch Set 1:

A change like this needs a test case, likely somewhere under misc/cgo.

I've updated the CL with a new test for go test cmd/go: it doesn't really rely on cgo, other than forcing external link (and the test can be written without it, using the build option -ldflags='-linkmode=external' if needed). The test passes with the changes to cmd/link, and fails when those same changes are backed out.

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

gopherbot · 2019-07-17T14:26:57Z

Message from Ian Lance Taylor:

Patch Set 2:

(5 comments)

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

gopherbot · 2019-07-18T00:00:05Z

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

Please visit https://go-review.googlesource.com/c/go/+/186417 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 · 2019-07-18T00:04:04Z

Message from John Papandriopoulos:

Patch Set 3:

(5 comments)

Patch Set 2:

(5 comments)

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

gopherbot · 2019-07-18T00:44:40Z

Message from Ian Lance Taylor:

Patch Set 3:

(1 comment)

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

gopherbot · 2019-07-18T02:47:30Z

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

Please visit https://go-review.googlesource.com/c/go/+/186417 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 · 2019-07-18T02:49:08Z

Message from John Papandriopoulos:

Patch Set 4:

(5 comments)

Patch Set 3:

(1 comment)

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

gopherbot · 2019-07-18T05:10:41Z

Message from Ian Lance Taylor:

Patch Set 4: Run-TryBot+1

Thanks. At this point in the release cycle this will have to wait until the 1.14 release.

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

gopherbot · 2019-07-18T05:15:55Z

Message from Gobot Gobot:

Patch Set 4:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=15e2427f

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

gopherbot · 2019-07-18T05:18:54Z

Message from Gobot Gobot:

Patch Set 4:

Build is still in progress...
This change failed on windows-amd64-2016:
See https://storage.googleapis.com/go-build-log/15e2427f/windows-amd64-2016_a75b1d1b.log

Other builds still in progress; subsequent failure notices suppressed until final report. Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

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

gopherbot · 2019-07-18T05:20:29Z

Message from Gobot Gobot:

Patch Set 4: TryBot-Result-1

1 of 21 TryBots failed:
Failed on windows-amd64-2016: https://storage.googleapis.com/go-build-log/15e2427f/windows-amd64-2016_a75b1d1b.log

Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

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

gopherbot · 2019-07-21T07:58:12Z

Message from John Papandriopoulos:

Patch Set 4:

The windows/amd64 build failed because loaded syso host object symbols are emitted into the go.o object file, which are duplicated in the same syso object fed to the external linker.

While this also happens on linux and darwin, there is no naming collision in the Mach-O/ELF go.o object file, so it builds and executes without error. However it also leads to some syso symbols being duplicated in the final binary which is undesirable.

I am looking into solving this properly and will update the CL soon.

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

gopherbot · 2019-07-22T10:11:24Z

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

Please visit https://go-review.googlesource.com/c/go/+/186417 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 · 2019-07-22T11:00:26Z

Message from John Papandriopoulos:

Patch Set 5:

The updated patch set:

Uses a separate Link context to load host objects and uses it to: A) mark Go symbols found in a host object as being external/dynamic import so as to include the correct relocations for the external linker, B) avoid duplicating the symbol text in the temporary go.o object file.
Has been tested on {darwin, linux, windows}/amd64.
Backs out cgo-specific package detection: it is not required.

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

gopherbot · 2019-07-22T12:04:19Z

Message from Than McIntosh:

Patch Set 5: Run-TryBot+1

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

gopherbot · 2019-07-22T12:05:20Z

Message from Gobot Gobot:

Patch Set 5:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=b93e9ab4

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

gopherbot · 2019-07-22T12:09:35Z

Message from Gobot Gobot:

Patch Set 5:

Build is still in progress...
This change failed on freebsd-amd64-12_0:
See https://storage.googleapis.com/go-build-log/b93e9ab4/freebsd-amd64-12_0_b2d301e6.log

Other builds still in progress; subsequent failure notices suppressed until final report. Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

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

gopherbot · 2019-07-22T12:15:33Z

Message from Gobot Gobot:

Patch Set 5: TryBot-Result-1

4 of 21 TryBots failed:
Failed on freebsd-amd64-12_0: https://storage.googleapis.com/go-build-log/b93e9ab4/freebsd-amd64-12_0_b2d301e6.log
Failed on linux-amd64: https://storage.googleapis.com/go-build-log/b93e9ab4/linux-amd64_16832ab0.log
Failed on linux-386: https://storage.googleapis.com/go-build-log/b93e9ab4/linux-386_48e1e7ec.log
Failed on windows-386-2008: https://storage.googleapis.com/go-build-log/b93e9ab4/windows-386-2008_900634ca.log

Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

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

gopherbot · 2019-07-24T01:51:13Z

Message from Emmanuel Odeke:

Patch Set 5:

(1 comment)

Thank you for working on this CL John and welcome to the Go project!

I've added just a minor suggestion to your CL and I'll defer to the rest
of the reviewers as the linker currently isn't fully in my circle of
competence.

Please take a look at the trybot failures, they seem related to this change:
ok misc/cgo/test 7.001s

misc/cgo/testtls.test

loadelf: $WORK/b052/pkg.a(_x003.o): malformed elf file: misc/cgo/testtls(.text)#0: reloc of invalid sym #2 tls shndx=4 type=6
FAIL misc/cgo/testtls [build failed]
2019/07/22 12:07:52 Failed: exit status 2

misc/cgo/testtls.test

loadelf: /tmp/workdir/go/pkg/obj/go-build/3f/3f8b3279547db8e3876e431c51d4d53de06ce4d20d8146a45a52998225ed175a-d(_x003.o): malformed elf file: misc/cgo/testtls(.text)#0: reloc of invalid sym #2 tls shndx=4 type=6
FAIL misc/cgo/testtls [build failed]
2019/07/22 12:07:52 Failed: exit status 2

misc/cgo/testtls.test

loadelf: /tmp/workdir/go/pkg/obj/go-build/3f/3f8b3279547db8e3876e431c51d4d53de06ce4d20d8146a45a52998225ed175a-d(_x003.o): malformed elf file: misc/cgo/testtls(.text)#0: reloc of invalid sym #2 tls shndx=4 type=6
FAIL misc/cgo/testtls [build failed]
2019/07/22 12:07:52 Failed: exit status 2

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

gopherbot · 2019-07-24T04:24:28Z

Message from John Papandriopoulos:

Patch Set 5:

Patch Set 5:

(1 comment)

Thank you for working on this CL John and welcome to the Go project!

Thank you for your welcome, Emmanuel! I'm very happy to be in your company. :)

I've added just a minor suggestion to your CL and I'll defer to the rest
of the reviewers as the linker currently isn't fully in my circle of
competence.

Please take a look at the trybot failures, they seem related to this change:
[snipped]

It has taken some time for me to set up a few VMs to reproduce the above errors; they've all been addressed off-line except the windows/386 error, which I am working on now. I'll push an update soon.

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

gopherbot · 2019-07-26T05:57:45Z

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

Please visit https://go-review.googlesource.com/c/go/+/186417 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 · 2019-07-26T06:21:47Z

Message from John Papandriopoulos:

Patch Set 6:

(1 comment)

This CL:

Fixes previous build errors: loading of host objects using features not supported by cmd/link resulted in an error that caused cmd/link to fail. We should not however restrict host files in cmd/link -- they are ultimately sent to the external linker verbatim, and we only care about the host object symbol table -- so we ignore any errors here.

We could limit the host object load to the symbol table only, and report errors, but that work might not be worth the effort in the end..?

Adds explicit support for windows/386, and arm (32-bit).
Brings back cgo package detection, which retains the current behavior of cmd/link not loading cgo host objects: this CL therefore doesn't unnecessarily slow down cgo builds, which likely a more popular usage scenario than the use of host object external linking.
Switches host object symbols from the type SDYNIMPORT to SHOSTOBJ, which is better aligned to how the SHOSTOBJ type was being used previously: for static imports, c.f. dynamic imports with SDYNIMPORT. (The changes required to support use of SDYNIMPORT on windows meant that the approach in this CL was also cleaner.)
Now that I'm aware of the go tool dist test tool :), I've been able to test the build on {darwin/amd64, linux/amd64, windows/386, windows/amd64, freebsd/amd64}. I've further verified by hand that the GOARCH=arm{,64} targets produce correct binaries, but I haven't run the full test suite on Arm.

Look forward to your review, and happy to make further changes as is recommended.

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

gopherbot · 2019-07-26T06:30:22Z

Message from Ian Lance Taylor:

Patch Set 6: Run-TryBot+1

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

gopherbot · 2019-07-26T06:30:23Z

Message from Gobot Gobot:

Patch Set 6:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=914327c0

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

gopherbot · 2019-07-26T06:37:49Z

Message from Gobot Gobot:

Patch Set 6:

Build is still in progress...
This change failed on android-amd64-emu:
See https://storage.googleapis.com/go-build-log/914327c0/android-amd64-emu_a0636106.log

Other builds still in progress; subsequent failure notices suppressed until final report. Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

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

gopherbot · 2019-07-26T06:42:50Z

Message from Gobot Gobot:

Patch Set 6: TryBot-Result-1

1 of 21 TryBots failed:
Failed on android-amd64-emu: https://storage.googleapis.com/go-build-log/914327c0/android-amd64-emu_a0636106.log

Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

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

gopherbot · 2019-07-26T06:45:58Z

Message from John Papandriopoulos:

Patch Set 6:

Patch Set 6: TryBot-Result-1

1 of 21 TryBots failed:
Failed on android-amd64-emu: https://storage.googleapis.com/go-build-log/914327c0/android-amd64-emu_a0636106.log

This failure looks to be unrelated to this change, see for example https://build.golang.org/log/d92f47c176a7c2dafb997f40c532ba14afb017ad for the same error.

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

Fix linking with a package having a .syso file in external link mode, that would otherwise cause an error before executing the external linker because it can't find symbols that are exported in the said .syso file. Fixes #33139

gopherbot · 2019-08-06T22:06:05Z

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

Please visit https://go-review.googlesource.com/c/go/+/186417 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 · 2019-08-27T19:30:51Z

Message from John Papandriopoulos:

Patch Set 7:

Ian, this CL seems to have gone cold. Any chance you could take fresh look at it?

I've been using this change myself on MacOS, Windows, and Linux for some weeks now, and haven't had any problems.

I'm happy to perform another rebase to bring the branch up-to-date. Please let me know if there is anything else I can address so that it can be merged.

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

gopherbot · 2019-08-31T03:20:42Z

Message from Ian Lance Taylor:

Patch Set 7:

(1 comment)

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

gopherbot · 2019-09-30T22:59:35Z

Message from John Papandriopoulos:

Patch Set 8: Commit message was updated.

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

gopherbot · 2019-09-30T23:16:15Z

Message from John Papandriopoulos:

Patch Set 11:

(1 comment)

Ian, thanks for taking the time to review the last patch set.

With patch set 10, I've started over from scratch, taking the direction you have suggested.

It has been manually tested on linux/386,amd64,arm,arm64, darwin/amd64,arm64, freebsd/amd64, windows/386,amd64.

I appreciate your feedback on this new approach.

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

jpap · 2019-09-30T23:16:21Z

Moved to using git codereview directly.

googlebot added the cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. label Jul 16, 2019

jpap mentioned this pull request Jul 17, 2019

cmd/link: fails to link package having a .syso file when using external linker #33139

Closed

cmd/link: load symbols from .syso in external link mode

2f8fbd3

Fix linking with a package having a .syso file in external link mode, that would otherwise cause an error before executing the external linker because it can't find symbols that are exported in the said .syso file. Fixes #33139

gopherbot force-pushed the master branch from 616c39f to f672e22 Compare September 26, 2019 20:46

jpap closed this Sep 30, 2019

cmd/link: load symbols from .syso in external link mode #33140

cmd/link: load symbols from .syso in external link mode #33140

Conversation

jpap commented Jul 16, 2019 • edited Loading

gopherbot commented Jul 16, 2019

gopherbot commented Jul 16, 2019

gopherbot commented Jul 17, 2019

gopherbot commented Jul 17, 2019

gopherbot commented Jul 17, 2019

gopherbot commented Jul 17, 2019

gopherbot commented Jul 18, 2019

gopherbot commented Jul 18, 2019

gopherbot commented Jul 18, 2019

gopherbot commented Jul 18, 2019

gopherbot commented Jul 18, 2019

gopherbot commented Jul 18, 2019

gopherbot commented Jul 18, 2019

gopherbot commented Jul 18, 2019

gopherbot commented Jul 18, 2019

gopherbot commented Jul 21, 2019

gopherbot commented Jul 22, 2019

gopherbot commented Jul 22, 2019

gopherbot commented Jul 22, 2019

gopherbot commented Jul 22, 2019

gopherbot commented Jul 22, 2019

gopherbot commented Jul 22, 2019

gopherbot commented Jul 24, 2019

misc/cgo/testtls.test

misc/cgo/testtls.test

misc/cgo/testtls.test

gopherbot commented Jul 24, 2019

gopherbot commented Jul 26, 2019

gopherbot commented Jul 26, 2019

gopherbot commented Jul 26, 2019

gopherbot commented Jul 26, 2019

gopherbot commented Jul 26, 2019

gopherbot commented Jul 26, 2019

gopherbot commented Jul 26, 2019

gopherbot commented Aug 6, 2019

gopherbot commented Aug 27, 2019

gopherbot commented Aug 31, 2019

gopherbot commented Sep 30, 2019

gopherbot commented Sep 30, 2019

jpap commented Sep 30, 2019

jpap commented Jul 16, 2019 •

edited

Loading