From 2f41edf120923000c92ed65ab501590fb1c8c548 Mon Sep 17 00:00:00 2001 From: Michael Hudson-Doyle Date: Wed, 4 May 2016 11:23:24 +1200 Subject: [PATCH] cmd/link: always read type data for dynimport symbols Consider three shared libraries: libBase.so -- defines a type T lib2.so -- references type T lib3.so -- also references type T, and something from lib2 lib2.so will contain a type symbol for T in its symbol table, but no definition. If, when linking lib3.so the linker reads the symbols from lib2.so before libBase.so, the linker didn't read the type data and later crashed. The fix is trivial but the test change is a bit messy because the order the linker reads the shared libraries in ends up depending on the order of the import statements in the file so I had to rename one of the test packages so that gofmt doesn't fix the test by accident... Fixes #15516 Change-Id: I124b058f782c900a3a54c15ed66a0d91d0cde5ce Reviewed-on: https://go-review.googlesource.com/22744 Run-TryBot: Michael Hudson-Doyle TryBot-Result: Gobot Gobot Reviewed-by: Ian Lance Taylor --- misc/cgo/testshared/shared_test.go | 76 ++++++++++--------- misc/cgo/testshared/src/dep2/dep2.go | 10 ++- misc/cgo/testshared/src/dep3/dep3.go | 22 ++++++ .../cgo/testshared/src/{dep => depBase}/asm.s | 0 .../testshared/src/{dep => depBase}/dep.go | 6 +- .../testshared/src/{dep => depBase}/gccgo.go | 2 +- .../testshared/src/{dep => depBase}/stubs.go | 2 +- misc/cgo/testshared/src/exe/exe.go | 6 +- misc/cgo/testshared/src/exe3/exe3.go | 7 ++ src/cmd/link/internal/ld/lib.go | 7 +- 10 files changed, 92 insertions(+), 46 deletions(-) create mode 100644 misc/cgo/testshared/src/dep3/dep3.go rename misc/cgo/testshared/src/{dep => depBase}/asm.s (100%) rename misc/cgo/testshared/src/{dep => depBase}/dep.go (74%) rename misc/cgo/testshared/src/{dep => depBase}/gccgo.go (73%) rename misc/cgo/testshared/src/{dep => depBase}/stubs.go (72%) create mode 100644 misc/cgo/testshared/src/exe3/exe3.go diff --git a/misc/cgo/testshared/shared_test.go b/misc/cgo/testshared/shared_test.go index 5c063963a0..34d97de526 100644 --- a/misc/cgo/testshared/shared_test.go +++ b/misc/cgo/testshared/shared_test.go @@ -135,7 +135,7 @@ func testMain(m *testing.M) (int, error) { goCmd(nil, append([]string{"install", "-buildmode=shared"}, minpkgs...)...) myContext.InstallSuffix = suffix + "_dynlink" - depP, err := myContext.Import("dep", ".", build.ImportComment) + depP, err := myContext.Import("depBase", ".", build.ImportComment) if err != nil { return 0, fmt.Errorf("import failed: %v", err) } @@ -416,11 +416,11 @@ func TestCgoPIE(t *testing.T) { // Build a GOPATH package into a shared library that links against the goroot runtime // and an executable that links against both. func TestGopathShlib(t *testing.T) { - goCmd(t, "install", "-buildmode=shared", "-linkshared", "dep") - AssertIsLinkedTo(t, filepath.Join(gopathInstallDir, "libdep.so"), soname) + goCmd(t, "install", "-buildmode=shared", "-linkshared", "depBase") + AssertIsLinkedTo(t, filepath.Join(gopathInstallDir, "libdepBase.so"), soname) goCmd(t, "install", "-linkshared", "exe") AssertIsLinkedTo(t, "./bin/exe", soname) - AssertIsLinkedTo(t, "./bin/exe", "libdep.so") + AssertIsLinkedTo(t, "./bin/exe", "libdepBase.so") AssertHasRPath(t, "./bin/exe", gorootInstallDir) AssertHasRPath(t, "./bin/exe", gopathInstallDir) // And check it runs. @@ -436,7 +436,7 @@ func testPkgListNote(t *testing.T, f *elf.File, note *note) { if isOffsetLoaded(f, note.section.Offset) { t.Errorf("package list section contained in PT_LOAD segment") } - if note.desc != "dep\n" { + if note.desc != "depBase\n" { t.Errorf("incorrect package list %q", note.desc) } } @@ -486,7 +486,7 @@ func testDepsNote(t *testing.T, f *elf.File, note *note) { if isOffsetLoaded(f, note.section.Offset) { t.Errorf("package list section contained in PT_LOAD segment") } - // libdep.so just links against the lib containing the runtime. + // libdepBase.so just links against the lib containing the runtime. if note.desc != soname { t.Errorf("incorrect dependency list %q", note.desc) } @@ -494,8 +494,8 @@ func testDepsNote(t *testing.T, f *elf.File, note *note) { // The shared library contains notes with defined contents; see above. func TestNotes(t *testing.T) { - goCmd(t, "install", "-buildmode=shared", "-linkshared", "dep") - f, err := elf.Open(filepath.Join(gopathInstallDir, "libdep.so")) + goCmd(t, "install", "-buildmode=shared", "-linkshared", "depBase") + f, err := elf.Open(filepath.Join(gopathInstallDir, "libdepBase.so")) if err != nil { t.Fatal(err) } @@ -543,16 +543,24 @@ func TestNotes(t *testing.T) { } } -// Build a GOPATH package (dep) into a shared library that links against the goroot +// Build a GOPATH package (depBase) into a shared library that links against the goroot // runtime, another package (dep2) that links against the first, and and an // executable that links against dep2. func TestTwoGopathShlibs(t *testing.T) { - goCmd(t, "install", "-buildmode=shared", "-linkshared", "dep") + goCmd(t, "install", "-buildmode=shared", "-linkshared", "depBase") goCmd(t, "install", "-buildmode=shared", "-linkshared", "dep2") goCmd(t, "install", "-linkshared", "exe2") run(t, "executable linked to GOPATH library", "./bin/exe2") } +func TestThreeGopathShlibs(t *testing.T) { + goCmd(t, "install", "-buildmode=shared", "-linkshared", "depBase") + goCmd(t, "install", "-buildmode=shared", "-linkshared", "dep2") + goCmd(t, "install", "-buildmode=shared", "-linkshared", "dep3") + goCmd(t, "install", "-linkshared", "exe3") + run(t, "executable linked to GOPATH library", "./bin/exe3") +} + // If gccgo is not available or not new enough call t.Skip. Otherwise, // return a build.Context that is set up for gccgo. func prepGccgo(t *testing.T) build.Context { @@ -586,16 +594,16 @@ func TestGoPathShlibGccgo(t *testing.T) { libgoRE := regexp.MustCompile("libgo.so.[0-9]+") - depP, err := gccgoContext.Import("dep", ".", build.ImportComment) + depP, err := gccgoContext.Import("depBase", ".", build.ImportComment) if err != nil { t.Fatalf("import failed: %v", err) } gccgoInstallDir := filepath.Join(depP.PkgTargetRoot, "shlibs") - goCmd(t, "install", "-compiler=gccgo", "-buildmode=shared", "-linkshared", "dep") - AssertIsLinkedToRegexp(t, filepath.Join(gccgoInstallDir, "libdep.so"), libgoRE) + goCmd(t, "install", "-compiler=gccgo", "-buildmode=shared", "-linkshared", "depBase") + AssertIsLinkedToRegexp(t, filepath.Join(gccgoInstallDir, "libdepBase.so"), libgoRE) goCmd(t, "install", "-compiler=gccgo", "-linkshared", "exe") AssertIsLinkedToRegexp(t, "./bin/exe", libgoRE) - AssertIsLinkedTo(t, "./bin/exe", "libdep.so") + AssertIsLinkedTo(t, "./bin/exe", "libdepBase.so") AssertHasRPath(t, "./bin/exe", gccgoInstallDir) // And check it runs. run(t, "gccgo-built", "./bin/exe") @@ -609,21 +617,21 @@ func TestTwoGopathShlibsGccgo(t *testing.T) { libgoRE := regexp.MustCompile("libgo.so.[0-9]+") - depP, err := gccgoContext.Import("dep", ".", build.ImportComment) + depP, err := gccgoContext.Import("depBase", ".", build.ImportComment) if err != nil { t.Fatalf("import failed: %v", err) } gccgoInstallDir := filepath.Join(depP.PkgTargetRoot, "shlibs") - goCmd(t, "install", "-compiler=gccgo", "-buildmode=shared", "-linkshared", "dep") + goCmd(t, "install", "-compiler=gccgo", "-buildmode=shared", "-linkshared", "depBase") goCmd(t, "install", "-compiler=gccgo", "-buildmode=shared", "-linkshared", "dep2") goCmd(t, "install", "-compiler=gccgo", "-linkshared", "exe2") - AssertIsLinkedToRegexp(t, filepath.Join(gccgoInstallDir, "libdep.so"), libgoRE) + AssertIsLinkedToRegexp(t, filepath.Join(gccgoInstallDir, "libdepBase.so"), libgoRE) AssertIsLinkedToRegexp(t, filepath.Join(gccgoInstallDir, "libdep2.so"), libgoRE) - AssertIsLinkedTo(t, filepath.Join(gccgoInstallDir, "libdep2.so"), "libdep.so") + AssertIsLinkedTo(t, filepath.Join(gccgoInstallDir, "libdep2.so"), "libdepBase.so") AssertIsLinkedToRegexp(t, "./bin/exe2", libgoRE) AssertIsLinkedTo(t, "./bin/exe2", "libdep2") - AssertIsLinkedTo(t, "./bin/exe2", "libdep.so") + AssertIsLinkedTo(t, "./bin/exe2", "libdepBase.so") // And check it runs. run(t, "gccgo-built", "./bin/exe2") @@ -690,22 +698,22 @@ func AssertNotRebuilt(t *testing.T, msg, path string) { } func TestRebuilding(t *testing.T) { - goCmd(t, "install", "-buildmode=shared", "-linkshared", "dep") + goCmd(t, "install", "-buildmode=shared", "-linkshared", "depBase") goCmd(t, "install", "-linkshared", "exe") // If the source is newer than both the .a file and the .so, both are rebuilt. resetFileStamps() - touch("src/dep/dep.go") + touch("src/depBase/dep.go") goCmd(t, "install", "-linkshared", "exe") - AssertRebuilt(t, "new source", filepath.Join(gopathInstallDir, "dep.a")) - AssertRebuilt(t, "new source", filepath.Join(gopathInstallDir, "libdep.so")) + AssertRebuilt(t, "new source", filepath.Join(gopathInstallDir, "depBase.a")) + AssertRebuilt(t, "new source", filepath.Join(gopathInstallDir, "libdepBase.so")) // If the .a file is newer than the .so, the .so is rebuilt (but not the .a) resetFileStamps() - touch(filepath.Join(gopathInstallDir, "dep.a")) + touch(filepath.Join(gopathInstallDir, "depBase.a")) goCmd(t, "install", "-linkshared", "exe") - AssertNotRebuilt(t, "new .a file", filepath.Join(gopathInstallDir, "dep.a")) - AssertRebuilt(t, "new .a file", filepath.Join(gopathInstallDir, "libdep.so")) + AssertNotRebuilt(t, "new .a file", filepath.Join(gopathInstallDir, "depBase.a")) + AssertRebuilt(t, "new .a file", filepath.Join(gopathInstallDir, "libdepBase.so")) } func appendFile(path, content string) { @@ -726,17 +734,17 @@ func appendFile(path, content string) { } func TestABIChecking(t *testing.T) { - goCmd(t, "install", "-buildmode=shared", "-linkshared", "dep") + goCmd(t, "install", "-buildmode=shared", "-linkshared", "depBase") goCmd(t, "install", "-linkshared", "exe") - // If we make an ABI-breaking change to dep and rebuild libp.so but not exe, + // If we make an ABI-breaking change to depBase and rebuild libp.so but not exe, // exe will abort with a complaint on startup. // This assumes adding an exported function breaks ABI, which is not true in // some senses but suffices for the narrow definition of ABI compatibility the // toolchain uses today. resetFileStamps() - appendFile("src/dep/dep.go", "func ABIBreak() {}\n") - goCmd(t, "install", "-buildmode=shared", "-linkshared", "dep") + appendFile("src/depBase/dep.go", "func ABIBreak() {}\n") + goCmd(t, "install", "-buildmode=shared", "-linkshared", "depBase") c := exec.Command("./bin/exe") output, err := c.CombinedOutput() if err == nil { @@ -744,7 +752,7 @@ func TestABIChecking(t *testing.T) { } scanner := bufio.NewScanner(bytes.NewReader(output)) foundMsg := false - const wantLine = "abi mismatch detected between the executable and libdep.so" + const wantLine = "abi mismatch detected between the executable and libdepBase.so" for scanner.Scan() { if scanner.Text() == wantLine { foundMsg = true @@ -763,10 +771,10 @@ func TestABIChecking(t *testing.T) { run(t, "rebuilt exe", "./bin/exe") // If we make a change which does not break ABI (such as adding an unexported - // function) and rebuild libdep.so, exe still works. + // function) and rebuild libdepBase.so, exe still works. resetFileStamps() - appendFile("src/dep/dep.go", "func noABIBreak() {}\n") - goCmd(t, "install", "-buildmode=shared", "-linkshared", "dep") + appendFile("src/depBase/dep.go", "func noABIBreak() {}\n") + goCmd(t, "install", "-buildmode=shared", "-linkshared", "depBase") run(t, "after non-ABI breaking change", "./bin/exe") } diff --git a/misc/cgo/testshared/src/dep2/dep2.go b/misc/cgo/testshared/src/dep2/dep2.go index bac1086a4a..c2c812adb9 100644 --- a/misc/cgo/testshared/src/dep2/dep2.go +++ b/misc/cgo/testshared/src/dep2/dep2.go @@ -1,11 +1,15 @@ package dep2 -import "dep" +import "depBase" var W int = 1 -var hasProg dep.HasProg +var hasProg depBase.HasProg + +type Dep2 struct { + depBase.Dep +} func G() int { - return dep.F() + 1 + return depBase.F() + 1 } diff --git a/misc/cgo/testshared/src/dep3/dep3.go b/misc/cgo/testshared/src/dep3/dep3.go new file mode 100644 index 0000000000..7b7c9dac1f --- /dev/null +++ b/misc/cgo/testshared/src/dep3/dep3.go @@ -0,0 +1,22 @@ +package dep3 + +// The point of this test file is that it references a type from +// depBase that is also referenced in dep2, but dep2 is loaded by the +// linker before depBase (because it is earlier in the import list). +// There was a bug in the linker where it would not correctly read out +// the type data in this case and later crash. + +import ( + "dep2" + "depBase" +) + +type Dep3 struct { + dep depBase.Dep + dep2 dep2.Dep2 +} + +func D3() int { + var x Dep3 + return x.dep.X + x.dep2.X +} diff --git a/misc/cgo/testshared/src/dep/asm.s b/misc/cgo/testshared/src/depBase/asm.s similarity index 100% rename from misc/cgo/testshared/src/dep/asm.s rename to misc/cgo/testshared/src/depBase/asm.s diff --git a/misc/cgo/testshared/src/dep/dep.go b/misc/cgo/testshared/src/depBase/dep.go similarity index 74% rename from misc/cgo/testshared/src/dep/dep.go rename to misc/cgo/testshared/src/depBase/dep.go index d3bed3f8ff..f9d3d7ce3a 100644 --- a/misc/cgo/testshared/src/dep/dep.go +++ b/misc/cgo/testshared/src/depBase/dep.go @@ -1,4 +1,4 @@ -package dep +package depBase var V int = 1 @@ -8,6 +8,10 @@ type HasProg struct { array [1024]*byte } +type Dep struct { + X int +} + func F() int { return V } diff --git a/misc/cgo/testshared/src/dep/gccgo.go b/misc/cgo/testshared/src/depBase/gccgo.go similarity index 73% rename from misc/cgo/testshared/src/dep/gccgo.go rename to misc/cgo/testshared/src/depBase/gccgo.go index 552ec303fa..3e2b69b50b 100644 --- a/misc/cgo/testshared/src/dep/gccgo.go +++ b/misc/cgo/testshared/src/depBase/gccgo.go @@ -1,5 +1,5 @@ //+build gccgo -package dep +package depBase func ImplementedInAsm() {} diff --git a/misc/cgo/testshared/src/dep/stubs.go b/misc/cgo/testshared/src/depBase/stubs.go similarity index 72% rename from misc/cgo/testshared/src/dep/stubs.go rename to misc/cgo/testshared/src/depBase/stubs.go index 036296a2fc..96573c12ec 100644 --- a/misc/cgo/testshared/src/dep/stubs.go +++ b/misc/cgo/testshared/src/depBase/stubs.go @@ -1,5 +1,5 @@ //+build !gccgo -package dep +package depBase func ImplementedInAsm() diff --git a/misc/cgo/testshared/src/exe/exe.go b/misc/cgo/testshared/src/exe/exe.go index f64477613a..136803fbc1 100644 --- a/misc/cgo/testshared/src/exe/exe.go +++ b/misc/cgo/testshared/src/exe/exe.go @@ -1,12 +1,12 @@ package main import ( - "dep" + "depBase" "runtime" ) func main() { - defer dep.ImplementedInAsm() + defer depBase.ImplementedInAsm() runtime.GC() - dep.V = dep.F() + 1 + depBase.V = depBase.F() + 1 } diff --git a/misc/cgo/testshared/src/exe3/exe3.go b/misc/cgo/testshared/src/exe3/exe3.go new file mode 100644 index 0000000000..643f2605f6 --- /dev/null +++ b/misc/cgo/testshared/src/exe3/exe3.go @@ -0,0 +1,7 @@ +package main + +import "dep3" + +func main() { + dep3.D3() +} diff --git a/src/cmd/link/internal/ld/lib.go b/src/cmd/link/internal/ld/lib.go index 0516ff60cf..ffad820aff 100644 --- a/src/cmd/link/internal/ld/lib.go +++ b/src/cmd/link/internal/ld/lib.go @@ -1522,9 +1522,10 @@ func ldshlibsyms(shlib string) { } lsym := Linklookup(Ctxt, elfsym.Name, 0) // Because loadlib above loads all .a files before loading any shared - // libraries, any symbols we find that duplicate symbols already - // loaded should be ignored (the symbols from the .a files "win"). - if lsym.Type != 0 { + // libraries, any non-dynimport symbols we find that duplicate symbols + // already loaded should be ignored (the symbols from the .a files + // "win"). + if lsym.Type != 0 && lsym.Type != obj.SDYNIMPORT { continue } lsym.Type = obj.SDYNIMPORT