[go] cmd/link: fix handling of visibility hidden symbols

[Cherry Mui (Gerrit)]

Cherry Mui submitted this change.

View Change

Approvals: Gopher Robot: TryBots succeeded Than McIntosh: Looks good to me, approved Cherry Mui: Run TryBots

cmd/link: fix handling of visibility hidden symbols

There is a TODO comment that checking hidden visibility is
probably not the right thing to do. I think it is indeed not. Here
we are not referencing symbols across DSO boundaries, just within
an executable binary. The hidden visibility is for references from
another DSO. So it doesn't actually matter.

This makes cgo internal linking tests work on ARM64 with newer
GCC. It failed and was disabled due to a visibility hidden symbol
in libgcc.a that we didn't handle correctly. Specifically, the
problem is that we didn't mark visibility hidden symbol references
SXREF, which caused the loader to not think it is an unresolved
external symbol, which in turn made it not loading an object file
from the libgcc.a archive which contains the actual definition.
Later stage when we try to resolve the relocation, we couldn't
resolve it. Enable the test as it works now.

Fixes #39466.

Change-Id: I2759e3ae15e7a7a1ab9a820223b688ad894509ed
Reviewed-on: https://go-review.googlesource.com/c/go/+/404296
Reviewed-by: Than McIntosh <th...@google.com>
Run-TryBot: Cherry Mui <cher...@google.com>
TryBot-Result: Gopher Robot <go...@golang.org>
---
M src/cmd/dist/test.go
M src/cmd/link/internal/amd64/asm.go
M src/cmd/link/internal/arm64/asm.go
M src/cmd/link/internal/loadelf/ldelf.go
4 files changed, 37 insertions(+), 16 deletions(-)

diff --git a/src/cmd/dist/test.go b/src/cmd/dist/test.go
index be4c552..817ea4a 100644
--- a/src/cmd/dist/test.go
+++ b/src/cmd/dist/test.go
@@ -1188,11 +1188,7 @@
 	cmd := t.addCmd(dt, "misc/cgo/test", t.goTest())
 	setEnv(cmd, "GOFLAGS", "-ldflags=-linkmode=auto")
 
-	// Skip internal linking cases on arm64 to support GCC-9.4 and above.
-	// See issue #39466.
-	skipInternalLink := goarch == "arm64" && goos != "darwin"
-
-	if t.internalLink() && !skipInternalLink {
+	if t.internalLink() {
 		cmd := t.addCmd(dt, "misc/cgo/test", t.goTest(), "-tags=internal")
 		setEnv(cmd, "GOFLAGS", "-ldflags=-linkmode=internal")
 	}
@@ -1268,7 +1264,7 @@
 
 			if t.supportedBuildmode("pie") {
 				t.addCmd(dt, "misc/cgo/test", t.goTest(), "-buildmode=pie")
-				if t.internalLink() && t.internalLinkPIE() && !skipInternalLink {
+				if t.internalLink() && t.internalLinkPIE() {
 					t.addCmd(dt, "misc/cgo/test", t.goTest(), "-buildmode=pie", "-ldflags=-linkmode=internal", "-tags=internal,internal_pie")
 				}
 				t.addCmd(dt, "misc/cgo/testtls", t.goTest(), "-buildmode=pie")
diff --git a/src/cmd/link/internal/amd64/asm.go b/src/cmd/link/internal/amd64/asm.go
index fb96049..f4832ef 100644
--- a/src/cmd/link/internal/amd64/asm.go
+++ b/src/cmd/link/internal/amd64/asm.go
@@ -88,9 +88,7 @@
 		if targType == sym.SDYNIMPORT {
 			ldr.Errorf(s, "unexpected R_X86_64_PC32 relocation for dynamic symbol %s", ldr.SymName(targ))
 		}
-		// TODO(mwhudson): the test of VisibilityHidden here probably doesn't make
-		// sense and should be removed when someone has thought about it properly.
-		if (targType == 0 || targType == sym.SXREF) && !ldr.AttrVisibilityHidden(targ) {
+		if targType == 0 || targType == sym.SXREF {
 			ldr.Errorf(s, "unknown symbol %s in pcrel", ldr.SymName(targ))
 		}
 		su := ldr.MakeSymbolUpdater(s)
diff --git a/src/cmd/link/internal/arm64/asm.go b/src/cmd/link/internal/arm64/asm.go
index 229a4d3..9937683 100644
--- a/src/cmd/link/internal/arm64/asm.go
+++ b/src/cmd/link/internal/arm64/asm.go
@@ -91,9 +91,7 @@
 		if targType == sym.SDYNIMPORT {
 			ldr.Errorf(s, "unexpected R_AARCH64_PREL32 relocation for dynamic symbol %s", ldr.SymName(targ))
 		}
-		// TODO(mwhudson): the test of VisibilityHidden here probably doesn't make
-		// sense and should be removed when someone has thought about it properly.
-		if (targType == 0 || targType == sym.SXREF) && !ldr.AttrVisibilityHidden(targ) {
+		if targType == 0 || targType == sym.SXREF {
 			ldr.Errorf(s, "unknown symbol %s in pcrel", ldr.SymName(targ))
 		}
 		su := ldr.MakeSymbolUpdater(s)
@@ -121,7 +119,7 @@
 			su.SetRelocSym(rIdx, syms.PLT)
 			su.SetRelocAdd(rIdx, r.Add()+int64(ldr.SymPlt(targ)))
 		}
-		if (targType == 0 || targType == sym.SXREF) && !ldr.AttrVisibilityHidden(targ) {
+		if targType == 0 || targType == sym.SXREF {
 			ldr.Errorf(s, "unknown symbol %s in callarm64", ldr.SymName(targ))
 		}
 		su := ldr.MakeSymbolUpdater(s)
diff --git a/src/cmd/link/internal/loadelf/ldelf.go b/src/cmd/link/internal/loadelf/ldelf.go
index d05d8e3..0381390 100644
--- a/src/cmd/link/internal/loadelf/ldelf.go
+++ b/src/cmd/link/internal/loadelf/ldelf.go
@@ -934,9 +934,7 @@
 		}
 	}
 
-	// TODO(mwhudson): the test of VisibilityHidden here probably doesn't make
-	// sense and should be removed when someone has thought about it properly.
-	if s != 0 && l.SymType(s) == 0 && !l.AttrVisibilityHidden(s) && elfsym.type_ != elf.STT_SECTION {
+	if s != 0 && l.SymType(s) == 0 && elfsym.type_ != elf.STT_SECTION {
 		sb := l.MakeSymbolUpdater(s)
 		sb.SetType(sym.SXREF)
 	}

To view, visit change 404296. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: go

Gerrit-Branch: master

Gerrit-Change-Id: I2759e3ae15e7a7a1ab9a820223b688ad894509ed

Gerrit-Change-Number: 404296

Gerrit-PatchSet: 2

Gerrit-Owner: Cherry Mui <cher...@google.com>

Gerrit-Reviewer: Cherry Mui <cher...@google.com>

Gerrit-Reviewer: Gopher Robot <go...@golang.org>

Gerrit-Reviewer: Than McIntosh <th...@google.com>

Gerrit-MessageType: merged