cmd/link: fix GC data reading from shared library (attempt 2)

When linking against a Go shared library, when a global variable
in the main module has a type defined in the shared library, the
linker needs to pull the GC data from the shared library to build
the GC program for the global variable. Currently, this fails
silently, as the shared library file is closed too early and the
read failed (with no error check), causing a zero GC map emitted
for the variable, which in turn causes the runtime to treat the
variable as pointerless.

For now, fix this by keeping the file open. In the future we may
want to use mmap to read from the shared library instead.

Also add error checking. And fix a (mostly harmless) mistake in
size caluculation.

Also remove an erroneous condition for ARM64. ARM64 used to have
a special case to get the addend from the relocation on the
gcdata field. That was removed, but the new code accidentally
returned 0 unconditionally. It's no longer necessary to have any
special case, since the addend is now applied directly to the
gcdata field on ARM64, like on all the other platforms.

Fixes #39927.

This is the second attempt of CL 240462. And this reverts
CL 240616.

Change-Id: I01c82422b9f67e872d833336885935bc509bc91b
Reviewed-on: https://go-review.googlesource.com/c/go/+/240621
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>

diff --git a/misc/cgo/testshared/shared_test.go b/misc/cgo/testshared/shared_test.go
index fda3d2ce76fc15a583f4c76bc7be384589ec9257..f8dabbe7a01f2ced2c840375a37253a5f10abaff 100644 (file)
--- a/misc/cgo/testshared/shared_test.go
+++ b/misc/cgo/testshared/shared_test.go
@@ -38,7 +38,15 @@ var testWork = flag.Bool("testwork", false, "if true, log and do not delete the
 
 // run runs a command and calls t.Errorf if it fails.
 func run(t *testing.T, msg string, args ...string) {
+       runWithEnv(t, msg, nil, args...)
+}
+
+// runWithEnv runs a command under the given environment and calls t.Errorf if it fails.
+func runWithEnv(t *testing.T, msg string, env []string, args ...string) {
        c := exec.Command(args[0], args[1:]...)
+       if len(env) != 0 {
+               c.Env = append(os.Environ(), env...)
+       }
        if output, err := c.CombinedOutput(); err != nil {
                t.Errorf("executing %s (%s) failed %s:\n%s", strings.Join(args, " "), msg, err, output)
        }
@@ -1034,3 +1042,11 @@ func TestGeneratedHash(t *testing.T) {
 func TestPackageOrder(t *testing.T) {
        goCmd(t, "install", "-buildmode=shared", "-linkshared", "./issue39777/a", "./issue39777/b")
 }
+
+// Test that GC data are generated correctly by the linker when it needs a type defined in
+// a shared library. See issue 39927.
+func TestGCData(t *testing.T) {
+       goCmd(t, "install", "-buildmode=shared", "-linkshared", "./gcdata/p")
+       goCmd(t, "build", "-linkshared", "./gcdata/main")
+       runWithEnv(t, "running gcdata/main", []string{"GODEBUG=clobberfree=1"}, "./main")
+}

diff --git a/misc/cgo/testshared/testdata/gcdata/main/main.go b/misc/cgo/testshared/testdata/gcdata/main/main.go
new file mode 100644 (file)
index 0000000..394862f
--- /dev/null
+++ b/misc/cgo/testshared/testdata/gcdata/main/main.go
@@ -0,0 +1,37 @@
+// Copyright 2020 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Test that GC data is generated correctly for global
+// variables with types defined in a shared library.
+// See issue 39927.
+
+// This test run under GODEBUG=clobberfree=1. The check
+// *x[i] == 12345 depends on this debug mode to clobber
+// the value if the object is freed prematurely.
+
+package main
+
+import (
+       "fmt"
+       "runtime"
+       "testshared/gcdata/p"
+)
+
+var x p.T
+
+func main() {
+       for i := range x {
+               x[i] = new(int)
+               *x[i] = 12345
+       }
+       runtime.GC()
+       runtime.GC()
+       runtime.GC()
+       for i := range x {
+               if *x[i] != 12345 {
+                       fmt.Printf("x[%d] == %d, want 12345\n", i, *x[i])
+                       panic("FAIL")
+               }
+       }
+}

diff --git a/misc/cgo/testshared/testdata/gcdata/p/p.go b/misc/cgo/testshared/testdata/gcdata/p/p.go
new file mode 100644 (file)
index 0000000..1fee754
--- /dev/null
+++ b/misc/cgo/testshared/testdata/gcdata/p/p.go
@@ -0,0 +1,7 @@
+// Copyright 2020 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package p
+
+type T [10]*int

diff --git a/src/cmd/link/internal/ld/decodesym.go b/src/cmd/link/internal/ld/decodesym.go
index e9c87efe3796b75543f85a988e3a6d3dac55e208..21bcc3a726a74d7be29bd2a62e52d70672ac9466 100644 (file)
--- a/src/cmd/link/internal/ld/decodesym.go
+++ b/src/cmd/link/internal/ld/decodesym.go
@@ -10,6 +10,7 @@ import (
        "cmd/link/internal/loader"
        "cmd/link/internal/sym"
        "debug/elf"
+       "log"
 )
 
 // Decoding the type.* symbols.         This has to be in sync with
@@ -229,8 +230,15 @@ func decodetypeGcmask(ctxt *Link, s loader.Sym) []byte {
                ptrdata := decodetypePtrdata(ctxt.Arch, symData)
                sect := findShlibSection(ctxt, ctxt.loader.SymPkg(s), addr)
                if sect != nil {
-                       r := make([]byte, ptrdata/int64(ctxt.Arch.PtrSize))
-                       sect.ReadAt(r, int64(addr-sect.Addr))
+                       bits := ptrdata / int64(ctxt.Arch.PtrSize)
+                       r := make([]byte, (bits+7)/8)
+                       // ldshlibsyms avoids closing the ELF file so sect.ReadAt works.
+                       // If we remove this read (and the ones in decodetypeGcprog), we
+                       // can close the file.
+                       _, err := sect.ReadAt(r, int64(addr-sect.Addr))
+                       if err != nil {
+                               log.Fatal(err)
+                       }
                        return r
                }
                Exitf("cannot find gcmask for %s", ctxt.loader.SymName(s))
@@ -251,9 +259,15 @@ func decodetypeGcprog(ctxt *Link, s loader.Sym) []byte {
                        // A gcprog is a 4-byte uint32 indicating length, followed by
                        // the actual program.
                        progsize := make([]byte, 4)
-                       sect.ReadAt(progsize, int64(addr-sect.Addr))
+                       _, err := sect.ReadAt(progsize, int64(addr-sect.Addr))
+                       if err != nil {
+                               log.Fatal(err)
+                       }
                        progbytes := make([]byte, ctxt.Arch.ByteOrder.Uint32(progsize))
-                       sect.ReadAt(progbytes, int64(addr-sect.Addr+4))
+                       _, err = sect.ReadAt(progbytes, int64(addr-sect.Addr+4))
+                       if err != nil {
+                               log.Fatal(err)
+                       }
                        return append(progsize, progbytes...)
                }
                Exitf("cannot find gcmask for %s", ctxt.loader.SymName(s))
@@ -268,7 +282,7 @@ func decodetypeGcprog(ctxt *Link, s loader.Sym) []byte {
 func findShlibSection(ctxt *Link, path string, addr uint64) *elf.Section {
        for _, shlib := range ctxt.Shlibs {
                if shlib.Path == path {
-                       for _, sect := range shlib.File.Sections {
+                       for _, sect := range shlib.File.Sections[1:] { // skip the NULL section
                                if sect.Addr <= addr && addr <= sect.Addr+sect.Size {
                                        return sect
                                }
@@ -279,8 +293,5 @@ func findShlibSection(ctxt *Link, path string, addr uint64) *elf.Section {
 }
 
 func decodetypeGcprogShlib(ctxt *Link, data []byte) uint64 {
-       if ctxt.Arch.Family == sys.ARM64 {
-               return 0
-       }
        return decodeInuxi(ctxt.Arch, data[2*int32(ctxt.Arch.PtrSize)+8+1*int32(ctxt.Arch.PtrSize):], ctxt.Arch.PtrSize)
 }

diff --git a/src/cmd/link/internal/ld/lib.go b/src/cmd/link/internal/ld/lib.go
index 7b6fd9594df5651cba5f2a34963fe4660c543ea4..b0a9613e4f478fda55a44b3c5dad81432a30ae33 100644 (file)
--- a/src/cmd/link/internal/ld/lib.go
+++ b/src/cmd/link/internal/ld/lib.go
@@ -2126,7 +2126,9 @@ func ldshlibsyms(ctxt *Link, shlib string) {
                Errorf(nil, "cannot open shared library: %s", libpath)
                return
        }
-       defer f.Close()
+       // Keep the file open as decodetypeGcprog needs to read from it.
+       // TODO: fix. Maybe mmap the file.
+       //defer f.Close()
 
        hash, err := readnote(f, ELF_NOTE_GO_NAME, ELF_NOTE_GOABIHASH_TAG)
        if err != nil {