Skip to content
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.

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

cmd/link: stop requiring gold on arm64 when GNU ld is fixed #22040

Open
jcajka opened this issue Sep 26, 2017 · 26 comments · May be fixed by #49748
Open

cmd/link: stop requiring gold on arm64 when GNU ld is fixed #22040

jcajka opened this issue Sep 26, 2017 · 26 comments · May be fixed by #49748
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@jcajka
Copy link
Contributor

jcajka commented Sep 26, 2017

What version of Go are you using (go version)?

Any, more specifically 1.8, 1.9, master, most probably all versions supporting AArch64/arm64 target

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

linux/arm64

What did you do?

GC on linux/arm64 assumes gold linker availability see src/cmd/link/internal/ld/lib.go:1182
When gold is not available gcc fails due to it missing, when it tries to invoke it. All in shared_test.
When gold flag is remove by removing sys.ARM64 from codition on src/cmd/link/internal/ld/lib.go:1173, build/test fail with

--- FAIL: TestTrivialExecutable (0.29s)
	shared_test.go:66: executing go install -installsuffix=5577006791947779410 -linkshared trivial failed exit status 2:
		# trivial
		/root/go/pkg/tool/linux_arm64/link: running gcc failed: exit status 1
		/usr/bin/ld: /tmp/go-link-151650837/go.o(.data.rel.ro+0x2c0): unresolvable R_AARCH64_ABS64 relocation against symbol `go.link.abihash.libruntime,sync-atomic.so'
		/usr/bin/ld: final link failed: Nonrepresentable section on output
		collect2: error: ld returned 1 exit status

And so one for most of the cases shared_test.

What did you expect to see?

All tests pass. GC working with default binutils ld.

What did you see instead?

GC failing to build some binaries with binutils ld.

@ianlancetaylor ianlancetaylor changed the title GC works only with gold on AArch64 in some cases cmd/link: compiler works only with gold on AArch64 in some cases Sep 26, 2017
@ianlancetaylor
Copy link
Contributor

For background see #15696 and https://sourceware.org/bugzilla/show_bug.cgi?id=19962. I think that we have to require gold until the GNU ld bug is fixed.

I'm going to close this issue since I don't see anything we can do. Please comment if you disagree.

@jcajka
Copy link
Contributor Author

jcajka commented Sep 26, 2017

@ianlancetaylor AFAIK SWBZ is related only to ARM32, my issues is on ARM64. I could be easily wrong though or there is same issues on ARM64, I'm not that familiar with binutils\s ld code base.

Also I would like to keep this opened, even if this turns out to be fully binutils bug. As I think that the workaround code should be reverted when the issue is fixed in ld(whatever for ARM32/64 or both).

@ianlancetaylor ianlancetaylor changed the title cmd/link: compiler works only with gold on AArch64 in some cases cmd/link: stop requiring gold on arm64 when GNU ld is fixed Sep 26, 2017
@ianlancetaylor
Copy link
Contributor

OK, I guess.

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Sep 26, 2017
@ALTree ALTree added the NeedsFix The path to resolution is known, but the work has not been done. label Sep 26, 2019
@jfkw
Copy link

jfkw commented Aug 13, 2020

I have a situation where the gold linker will not be available on an ARM64 linux target and would like to avoid the gold dependency so I can package Go there. The referenced binutils bug https://sourceware.org/bugzilla/show_bug.cgi?id=19962 has a proposed patch:

--- a/bfd/elf32-arm.c	
+++ a/bfd/elf32-arm.c	
@@ -14112,11 +14112,15 @@ elf32_arm_adjust_dynamic_symbol (struct bfd_link_info * info,
   s = bfd_get_linker_section (dynobj, ".dynbss");
   BFD_ASSERT (s != NULL);
 
-  /* We must generate a R_ARM_COPY reloc to tell the dynamic linker to
-     copy the initial value out of the dynamic object and into the
-     runtime process image.  We need to remember the offset into the
+  /* If allowed, we must generate a R_ARM_COPY reloc to tell the dynamic
+     linker to copy the initial value out of the dynamic object and into
+     the runtime process image.  We need to remember the offset into the
      .rel(a).bss section we are going to use.  */
-  if ((h->root.u.def.section->flags & SEC_ALLOC) != 0 && h->size != 0)
+  if (info->nocopyreloc == 0
+      && (h->root.u.def.section->flags & SEC_ALLOC) != 0
+      /* PR 16177: A copy is only needed if the input section is readonly.  */
+      && (h->root.u.def.section->flags & SEC_READONLY) == 0
+      && h->size != 0)
     {
       asection *srel;

@ianlancetaylor @crawshaw Would it be possible to get some feedback from the Go team on whether that patch addresses the issue for Go? With that it might be possible to get the patch or a revised patch accepted by upstream binutils.

@ianlancetaylor
Copy link
Contributor

I don't see why that patch will help, as as far as I can see it only affects arm, not arm64.

@jfkw
Copy link

jfkw commented Mar 10, 2021

A proposed patch for aarch64 was added to https://sourceware.org/bugzilla/show_bug.cgi?id=19962 on 2020-08-24:

--- a/bfd/elfnn-aarch64.c	
+++ a/bfd/elfnn-aarch64.c	
@@ -7474,7 +7474,10 @@ elfNN_aarch64_adjust_dynamic_symbol (struct bfd_link_info *info,
       s = htab->root.sdynbss;
       srel = htab->root.srelbss;
     }
-  if ((h->root.u.def.section->flags & SEC_ALLOC) != 0 && h->size != 0)
+  if ((h->root.u.def.section->flags & SEC_ALLOC)
+      && (h->root.u.def.section->flags & SEC_READONLY)
+      && h->size != 0)
+
     {
       srel->size += RELOC_SIZE (htab);
       h->needs_copy = 1;

@jefferyto
Copy link

The GNU ld issue has been marked as fixed (according to the last comment the issue is not present in binutils 2.36 and higher). Would it be possible to revisit the gold linker requirement at this point?

The OpenWrt toolchain does not include the gold linker and I have been advising our packages to patch out Go plugins to avoid triggering this requirement (crowdsec, rclone).

@ianlancetaylor
Copy link
Contributor

I suggest that for now we change to request gold but not give an error if gold is not available. Want to test and send a patch for that? It's cmd/link/internal/ld/lib.go.

@jefferyto
Copy link

I suggest that for now we change to request gold but not give an error if gold is not available. Want to test and send a patch for that? It's cmd/link/internal/ld/lib.go.

I’ll try to find some time to work on this patch - thanks 🙂

jefferyto added a commit to jefferyto/go that referenced this issue Nov 23, 2021
COPY relocation handling on ARM/ARM64 has been fixed in recent versions
of the GNU linker. This switches to gold only if gold is available.

Fixes golang#22040.
@gopherbot
Copy link

Change https://golang.org/cl/366279 mentions this issue: cmd/link: use gold on ARM/ARM64 only if gold is available

@gopherbot
Copy link

Change https://go.dev/cl/391115 mentions this issue: cmd/link: stop forcing binutils-gold dependency on aarch64

@coyzeng
Copy link

coyzeng commented Dec 14, 2022

This PR merge plan?

@williamh
Copy link

Hi all, I am the package maintainer for Go on Gentoo Linux.
This issue has now been reported to me, and I've been asked to look into just removing the broken linker workaround code.
This is our bug.
Is there any chance of removing the code that hardcodes the linker?

@ianlancetaylor
Copy link
Contributor

Can someone investigate to see exactly when the GNU linker was fixed? There are two different patches submitted for this (https://go.dev/cl/366279, https://go.dev/cl/391115) and they both seem to have some discussion.

In particular, do you have concerns about the approach in https://go.dev/cl/366279?

@williamh
Copy link

williamh commented Feb 13, 2023

@ianlancetaylor I am not an expert in the toolchain, so hopefully someone else will join this issue and comment. The position of the people wanting me to diverge from what upstream is
doing is that they want to completely move away from gold on Gentoo.

@thesamesam
Copy link

thesamesam commented Feb 14, 2023

Thanks for commenting Ian.

On binutils:

For Gentoo, our usecase/concerns are:

  • Go currently forces use of gold on arm/arm64 for a bug not affecting Gentoo for quite some time (we keep binutils very up to date)
  • We want the linker to match the user's default is in their compiler, unless there's a good reason not to (for their benefit + making it easier to understand/reproduce bugs)
  • The "automagic" (use-if-available) patch might be a good compromise Go upstream, but not ideal downstream. We're mostly moving away from gold right now and changing behaviour based on whether a package is/isn't installed leads to confusing/reproducibility issues with bugs

My preference would be therefore be https://go.dev/cl/391115 (with or without the make.bash check, as it doesn't affect us).

On making the change at all:

@ianlancetaylor
Copy link
Contributor

@thesamesam Thanks. Just to make sure that I understand, you said:

I can reproduce the test failures when using bfd-2.40 in TestGCData with https://go.dev/cl/391115 applied to Go 1.20, so I don't think we can move forward yet.

It sounds like this means that there is still some problem using the Go linker with GNU ld 2.40 on arm64 Linux, but that using the Go linker with gold does work. Is that correct?

@thesamesam
Copy link

thesamesam commented Feb 14, 2023

@ianlancetaylor Yeah, exactly. For the situation you describe: arm Linux is OK, but arm64 Linux is not.

@gopherbot
Copy link

Change https://go.dev/cl/468655 mentions this issue: cmd/link: don't switch to gold on ARM Linux

gopherbot pushed a commit that referenced this issue Feb 17, 2023
The bug in GNU ld appears to have been fixed in GNU binutils 2.28 by
GNU binutils revision 5522f910cb539905d6adfdceab208ddfa5e84557.
(This may have been accidental as the ChangeLog for the fix makes
no reference to it; the fix is from
https://sourceware.org/bugzilla/show_bug.cgi?id=19962).

Continue using gold on arm64, at least for now, because as reported in
issue #22040 GNU ld still fails there.

For #15696
For #22040

Change-Id: I5534bb8b5680daf536a7941aba5c701e8a4138ab
Reviewed-on: https://go-review.googlesource.com/c/go/+/468655
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
johanbrandhorst pushed a commit to Pryz/go that referenced this issue Feb 22, 2023
The bug in GNU ld appears to have been fixed in GNU binutils 2.28 by
GNU binutils revision 5522f910cb539905d6adfdceab208ddfa5e84557.
(This may have been accidental as the ChangeLog for the fix makes
no reference to it; the fix is from
https://sourceware.org/bugzilla/show_bug.cgi?id=19962).

Continue using gold on arm64, at least for now, because as reported in
issue golang#22040 GNU ld still fails there.

For golang#15696
For golang#22040

Change-Id: I5534bb8b5680daf536a7941aba5c701e8a4138ab
Reviewed-on: https://go-review.googlesource.com/c/go/+/468655
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
@susematz
Copy link

@thesamesam Thanks. Just to make sure that I understand, you said:

I can reproduce the test failures when using bfd-2.40 in TestGCData with https://go.dev/cl/391115 applied to Go 1.20, so I don't think we can move forward yet.

It sounds like this means that there is still some problem using the Go linker with GNU ld 2.40 on arm64 Linux, but that using the Go linker with gold does work. Is that correct?

After much fighting with the golang toolchain and how its linking process works I finally found the BFD problem:
it's https://sourceware.org/bugzilla/show_bug.cgi?id=30437 and I have posted a fix for it. With ld.bfd so patched and the change to disable hardcoding ld.gold the golang testsuite now passes everything. (Tested on release-branch.go1.20).

@ianlancetaylor
Copy link
Contributor

@susematz Thanks. So after that patch is committed into a binutils release, we can remove the remaining case where the Go linker requires gold.

@jefferyto
Copy link

The binutils issue was marked as fixed on May 23 and a new version (2.41) was released on July 30.

jefferyto added a commit to jefferyto/openwrt-packages that referenced this issue Aug 15, 2023
Upstream has updated the Go compiler to not use gold when building for
arm, and is waiting for a fix to binutils (released in 2.41) before
doing the same for aarch64.[1]

Based on the above, it does not appear that
golang/go#49748 will be merged. This removes the
patch from that pull request.

[1]: golang/go#22040

Signed-off-by: Jeffery To <jeffery.to@gmail.com>
@ianlancetaylor
Copy link
Contributor

Thanks. Would somebody like to update and test https://go.dev/cl/391115?

@dirkmueller
Copy link

it was back up now, so I updated.

@jfkw
Copy link

jfkw commented Aug 18, 2023

Tests in all.bash pass with the updated CL https://go.dev/cl/391115/4. On SUSE and openSUSE we have applied this patch from go1.16 through go1.21 without issues.

jefferyto added a commit to jefferyto/openwrt-packages that referenced this issue Sep 22, 2023
Upstream has updated the Go compiler to not use gold when building for
arm, and is waiting for a fix to binutils (released in 2.41) before
doing the same for aarch64.[1]

Based on the above, it does not appear that
golang/go#49748 will be merged. This removes the
patch from that pull request.

[1]: golang/go#22040

Signed-off-by: Jeffery To <jeffery.to@gmail.com>
(cherry picked from commit a80af7e)
BKPepe pushed a commit to openwrt/packages that referenced this issue Sep 27, 2023
Upstream has updated the Go compiler to not use gold when building for
arm, and is waiting for a fix to binutils (released in 2.41) before
doing the same for aarch64.[1]

Based on the above, it does not appear that
golang/go#49748 will be merged. This removes the
patch from that pull request.

[1]: golang/go#22040

Signed-off-by: Jeffery To <jeffery.to@gmail.com>
(cherry picked from commit a80af7e)
smallprogram added a commit to smallprogram/packages that referenced this issue Oct 1, 2023
Upstream has updated the Go compiler to not use gold when building for
arm, and is waiting for a fix to binutils (released in 2.41) before
doing the same for aarch64.[1]

Based on the above, it does not appear that
golang/go#49748 will be merged. This removes the
patch from that pull request.

[1]: golang/go#22040

Signed-off-by: Jeffery To <jeffery.to@gmail.com>
Beginner-Go pushed a commit to coolsnowwolf/packages that referenced this issue Oct 1, 2023
Upstream has updated the Go compiler to not use gold when building for
arm, and is waiting for a fix to binutils (released in 2.41) before
doing the same for aarch64.[1]

Based on the above, it does not appear that
golang/go#49748 will be merged. This removes the
patch from that pull request.

[1]: golang/go#22040

Signed-off-by: Jeffery To <jeffery.to@gmail.com>
lu-zero pushed a commit to domo-iot/packages that referenced this issue Oct 23, 2023
Upstream has updated the Go compiler to not use gold when building for
arm, and is waiting for a fix to binutils (released in 2.41) before
doing the same for aarch64.[1]

Based on the above, it does not appear that
golang/go#49748 will be merged. This removes the
patch from that pull request.

[1]: golang/go#22040

Signed-off-by: Jeffery To <jeffery.to@gmail.com>
@zpon
Copy link

zpon commented Nov 17, 2023

Are there any progress on landing one of the proposed solutions to remove the "gold" check? Lots of projects are ending up patching the go code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

Successfully merging a pull request may close this issue.