From 9b2df72b63ff977004756e9b847f926b4fb8d8a8 Mon Sep 17 00:00:00 2001 From: Cherry Zhang Date: Thu, 10 Sep 2020 11:07:48 -0400 Subject: cmd/link: add copyright header Change-Id: I44f57019bb8e659d4aa3da8b13e8bd9a20b9d2e1 Reviewed-on: https://go-review.googlesource.com/c/go/+/253920 Reviewed-by: Than McIntosh --- src/cmd/link/link_test.go | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'src/cmd/link/link_test.go') diff --git a/src/cmd/link/link_test.go b/src/cmd/link/link_test.go index 72ff01c932..98798be465 100644 --- a/src/cmd/link/link_test.go +++ b/src/cmd/link/link_test.go @@ -1,3 +1,7 @@ +// Copyright 2016 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 main import ( -- cgit v1.3 From 1ed4f12f4a6b9d783cf9a6fc3a292a433b8539c6 Mon Sep 17 00:00:00 2001 From: Cherry Zhang Date: Thu, 10 Sep 2020 11:14:27 -0400 Subject: cmd/link: add a test to test RODATA is indeed read-only Updates #38830. Change-Id: Ie1f6ccef40a773f038aac587dfc26bf70a1a8536 Reviewed-on: https://go-review.googlesource.com/c/go/+/253921 Run-TryBot: Cherry Zhang Reviewed-by: Than McIntosh TryBot-Result: Gobot Gobot --- src/cmd/link/link_test.go | 14 ++++++++++++++ src/cmd/link/testdata/testRO/x.go | 22 ++++++++++++++++++++++ 2 files changed, 36 insertions(+) create mode 100644 src/cmd/link/testdata/testRO/x.go (limited to 'src/cmd/link/link_test.go') diff --git a/src/cmd/link/link_test.go b/src/cmd/link/link_test.go index 98798be465..4e60996d8e 100644 --- a/src/cmd/link/link_test.go +++ b/src/cmd/link/link_test.go @@ -800,3 +800,17 @@ func TestContentAddressableSymbols(t *testing.T) { t.Errorf("command %s failed: %v\n%s", cmd, err, out) } } + +func TestReadOnly(t *testing.T) { + // Test that read-only data is indeed read-only. + testenv.MustHaveGoBuild(t) + + t.Parallel() + + src := filepath.Join("testdata", "testRO", "x.go") + cmd := exec.Command(testenv.GoToolPath(t), "run", src) + out, err := cmd.CombinedOutput() + if err == nil { + t.Errorf("running test program did not fail. output:\n%s", out) + } +} diff --git a/src/cmd/link/testdata/testRO/x.go b/src/cmd/link/testdata/testRO/x.go new file mode 100644 index 0000000000..d77db6d563 --- /dev/null +++ b/src/cmd/link/testdata/testRO/x.go @@ -0,0 +1,22 @@ +// 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 read-only data is indeed read-only. This +// program attempts to modify read-only data, and it +// should fail. + +package main + +import "unsafe" + +var s = "hello" + +func main() { + println(s) + *(*struct { + p *byte + l int + })(unsafe.Pointer(&s)).p = 'H' + println(s) +} -- cgit v1.3 From 7e9369a517d9ebf867748719948d8cbccec3bc57 Mon Sep 17 00:00:00 2001 From: Jay Conrod Date: Mon, 21 Sep 2020 12:10:16 -0400 Subject: cmd/link: add go.mod to TestFuncAlign Fixes #41531 Change-Id: I8b4f0d5b7094e56787998d244d8a4c03becb8452 Reviewed-on: https://go-review.googlesource.com/c/go/+/256302 Trust: Jay Conrod Reviewed-by: Bryan C. Mills --- src/cmd/link/link_test.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) (limited to 'src/cmd/link/link_test.go') diff --git a/src/cmd/link/link_test.go b/src/cmd/link/link_test.go index 4e60996d8e..b7611f207c 100644 --- a/src/cmd/link/link_test.go +++ b/src/cmd/link/link_test.go @@ -595,7 +595,12 @@ func TestFuncAlign(t *testing.T) { } defer os.RemoveAll(tmpdir) - src := filepath.Join(tmpdir, "falign.go") + src := filepath.Join(tmpdir, "go.mod") + err = ioutil.WriteFile(src, []byte("module cmd/link/TestFuncAlign/falign"), 0666) + if err != nil { + t.Fatal(err) + } + src = filepath.Join(tmpdir, "falign.go") err = ioutil.WriteFile(src, []byte(testFuncAlignSrc), 0666) if err != nil { t.Fatal(err) -- cgit v1.3 From f8df205e74d5122c43f41923280451641e566ee2 Mon Sep 17 00:00:00 2001 From: Cherry Zhang Date: Wed, 7 Oct 2020 18:29:51 -0400 Subject: all: enable more tests on macOS/ARM64 On macOS, we can do "go build", can exec, and have the source tree available, so we can enable more tests. Skip ones that don't work. Most of them are due to that it requires external linking (for now) and some tests don't work with external linking (e.g. runtime deadlock detection). For them, helper functions CanInternalLink/MustInternalLink are introduced. I still want to have internal linking implemented, but it is still a good idea to identify which tests don't work with external linking. Updates #38485. Change-Id: I6b14697573cf3f371daf54b9ddd792acf232f2f2 Reviewed-on: https://go-review.googlesource.com/c/go/+/260719 Trust: Cherry Zhang Run-TryBot: Cherry Zhang TryBot-Result: Go Bot Reviewed-by: Brad Fitzpatrick Reviewed-by: Than McIntosh --- src/cmd/go/go_test.go | 9 ++++--- src/cmd/internal/sys/supported.go | 1 + src/cmd/internal/sys/supported_test.go | 18 ++++++++++++++ src/cmd/link/internal/ld/dwarf_test.go | 17 +++++++++++++ src/cmd/link/internal/ld/ld_test.go | 7 +++++- src/cmd/link/link_test.go | 1 + src/cmd/nm/nm_cgo_test.go | 5 ++++ src/cmd/nm/nm_test.go | 3 +++ src/internal/cpu/cpu_test.go | 9 +++++++ src/internal/testenv/testenv.go | 44 +++++++++++++++++++++++----------- src/os/exec/exec_test.go | 4 ++++ src/runtime/crash_test.go | 21 ++++++++++++++++ src/runtime/time_test.go | 4 ++++ 13 files changed, 123 insertions(+), 20 deletions(-) create mode 100644 src/cmd/internal/sys/supported_test.go (limited to 'src/cmd/link/link_test.go') diff --git a/src/cmd/go/go_test.go b/src/cmd/go/go_test.go index 66a52c86ad..093ea2ffa1 100644 --- a/src/cmd/go/go_test.go +++ b/src/cmd/go/go_test.go @@ -58,11 +58,10 @@ func init() { switch runtime.GOOS { case "android", "js": canRun = false - case "darwin", "ios": - switch runtime.GOARCH { - case "arm64": - canRun = false - } + case "darwin": + // nothing to do + case "ios": + canRun = false case "linux": switch runtime.GOARCH { case "arm": diff --git a/src/cmd/internal/sys/supported.go b/src/cmd/internal/sys/supported.go index 8d87e95655..41e5ec1432 100644 --- a/src/cmd/internal/sys/supported.go +++ b/src/cmd/internal/sys/supported.go @@ -32,6 +32,7 @@ func MSanSupported(goos, goarch string) bool { } // MustLinkExternal reports whether goos/goarch requires external linking. +// (This is the opposite of internal/testenv.CanInternalLink. Keep them in sync.) func MustLinkExternal(goos, goarch string) bool { switch goos { case "android": diff --git a/src/cmd/internal/sys/supported_test.go b/src/cmd/internal/sys/supported_test.go new file mode 100644 index 0000000000..1217814af5 --- /dev/null +++ b/src/cmd/internal/sys/supported_test.go @@ -0,0 +1,18 @@ +// 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 sys + +import ( + "internal/testenv" + "runtime" + "testing" +) + +func TestMustLinkExternalMatchesTestenv(t *testing.T) { + // MustLinkExternal and testenv.CanInternalLink are the exact opposite. + if b := MustLinkExternal(runtime.GOOS, runtime.GOARCH); b != !testenv.CanInternalLink() { + t.Fatalf("MustLinkExternal() == %v, testenv.CanInternalLink() == %v, don't match", b, testenv.CanInternalLink()) + } +} diff --git a/src/cmd/link/internal/ld/dwarf_test.go b/src/cmd/link/internal/ld/dwarf_test.go index 22948521f5..a66506d392 100644 --- a/src/cmd/link/internal/ld/dwarf_test.go +++ b/src/cmd/link/internal/ld/dwarf_test.go @@ -238,6 +238,10 @@ func TestSizes(t *testing.T) { if runtime.GOOS == "plan9" { t.Skip("skipping on plan9; no DWARF symbol table in executables") } + + // External linking may bring in C symbols with unknown size. Skip. + testenv.MustInternalLink(t) + t.Parallel() // DWARF sizes should never be -1. @@ -919,6 +923,7 @@ func TestAbstractOriginSanityIssue26237(t *testing.T) { func TestRuntimeTypeAttrInternal(t *testing.T) { testenv.MustHaveGoBuild(t) + testenv.MustInternalLink(t) if runtime.GOOS == "plan9" { t.Skip("skipping on plan9; no DWARF symbol table in executables") @@ -1018,6 +1023,9 @@ func main() { t.Fatalf("*main.X DIE had no runtime type attr. DIE: %v", dies[0]) } + if runtime.GOOS == "darwin" && runtime.GOARCH == "arm64" { + return // everything is PIE on ARM64, addresses are relocated + } if rtAttr.(uint64)+types.Addr != addr { t.Errorf("DWARF type offset was %#x+%#x, but test program said %#x", rtAttr.(uint64), types.Addr, addr) } @@ -1203,6 +1211,15 @@ func main() { } } + // When external linking, we put all symbols in the symbol table (so the + // external linker can find them). Skip the symbol table check. + // TODO: maybe there is some way to tell the external linker not to put + // those symbols in the executable's symbol table? Prefix the symbol name + // with "." or "L" to pretend it is a label? + if !testenv.CanInternalLink() { + return + } + syms, err := f.Symbols() if err != nil { t.Fatalf("error reading symbols: %v", err) diff --git a/src/cmd/link/internal/ld/ld_test.go b/src/cmd/link/internal/ld/ld_test.go index 4367c1028e..cdfaadb17d 100644 --- a/src/cmd/link/internal/ld/ld_test.go +++ b/src/cmd/link/internal/ld/ld_test.go @@ -18,8 +18,13 @@ import ( ) func TestUndefinedRelocErrors(t *testing.T) { - t.Parallel() testenv.MustHaveGoBuild(t) + + // When external linking, symbols may be defined externally, so we allow + // undefined symbols and let external linker resolve. Skip the test. + testenv.MustInternalLink(t) + + t.Parallel() dir, err := ioutil.TempDir("", "go-build") if err != nil { t.Fatal(err) diff --git a/src/cmd/link/link_test.go b/src/cmd/link/link_test.go index b7611f207c..6729568766 100644 --- a/src/cmd/link/link_test.go +++ b/src/cmd/link/link_test.go @@ -181,6 +181,7 @@ main.x: relocation target main.zero not defined func TestIssue33979(t *testing.T) { testenv.MustHaveGoBuild(t) testenv.MustHaveCGO(t) + testenv.MustInternalLink(t) // Skip test on platforms that do not support cgo internal linking. switch runtime.GOARCH { diff --git a/src/cmd/nm/nm_cgo_test.go b/src/cmd/nm/nm_cgo_test.go index 9a257e0ed2..58f2c24908 100644 --- a/src/cmd/nm/nm_cgo_test.go +++ b/src/cmd/nm/nm_cgo_test.go @@ -15,6 +15,11 @@ func canInternalLink() bool { switch runtime.GOOS { case "aix": return false + case "darwin": + switch runtime.GOARCH { + case "arm64": + return false + } case "dragonfly": return false case "freebsd": diff --git a/src/cmd/nm/nm_test.go b/src/cmd/nm/nm_test.go index 413a4eb06f..382446e9fe 100644 --- a/src/cmd/nm/nm_test.go +++ b/src/cmd/nm/nm_test.go @@ -173,6 +173,9 @@ func testGoExec(t *testing.T, iscgo, isexternallinker bool) { if runtime.GOOS == "windows" { return true } + if runtime.GOOS == "darwin" && runtime.GOARCH == "arm64" { + return true // On darwin/arm64 everything is PIE + } return false } diff --git a/src/internal/cpu/cpu_test.go b/src/internal/cpu/cpu_test.go index e09bd2d8b9..919bbd5ed7 100644 --- a/src/internal/cpu/cpu_test.go +++ b/src/internal/cpu/cpu_test.go @@ -15,6 +15,7 @@ import ( ) func TestMinimalFeatures(t *testing.T) { + // TODO: maybe do MustSupportFeatureDectection(t) ? if runtime.GOARCH == "arm64" { switch runtime.GOOS { case "linux", "android": @@ -36,6 +37,13 @@ func MustHaveDebugOptionsSupport(t *testing.T) { } } +func MustSupportFeatureDectection(t *testing.T) { + if runtime.GOOS == "darwin" && runtime.GOARCH == "arm64" { + t.Skipf("CPU feature detection is not supported on %s/%s", runtime.GOOS, runtime.GOARCH) + } + // TODO: maybe there are other platforms? +} + func runDebugOptionsTest(t *testing.T, test string, options string) { MustHaveDebugOptionsSupport(t) @@ -58,6 +66,7 @@ func runDebugOptionsTest(t *testing.T, test string, options string) { } func TestDisableAllCapabilities(t *testing.T) { + MustSupportFeatureDectection(t) runDebugOptionsTest(t, "TestAllCapabilitiesDisabled", "cpu.all=off") } diff --git a/src/internal/testenv/testenv.go b/src/internal/testenv/testenv.go index cfb033b2a2..0ee6355ee3 100644 --- a/src/internal/testenv/testenv.go +++ b/src/internal/testenv/testenv.go @@ -43,12 +43,8 @@ func HasGoBuild() bool { return false } switch runtime.GOOS { - case "android", "js": + case "android", "js", "ios": return false - case "darwin", "ios": - if runtime.GOARCH == "arm64" { - return false - } } return true } @@ -122,12 +118,8 @@ func GoTool() (string, error) { // using os.StartProcess or (more commonly) exec.Command. func HasExec() bool { switch runtime.GOOS { - case "js": + case "js", "ios": return false - case "darwin", "ios": - if runtime.GOARCH == "arm64" { - return false - } } return true } @@ -135,10 +127,8 @@ func HasExec() bool { // HasSrc reports whether the entire source tree is available under GOROOT. func HasSrc() bool { switch runtime.GOOS { - case "darwin", "ios": - if runtime.GOARCH == "arm64" { - return false - } + case "ios": + return false } return true } @@ -202,6 +192,32 @@ func MustHaveCGO(t testing.TB) { } } +// CanInternalLink reports whether the current system can link programs with +// internal linking. +// (This is the opposite of cmd/internal/sys.MustLinkExternal. Keep them in sync.) +func CanInternalLink() bool { + switch runtime.GOOS { + case "android": + if runtime.GOARCH != "arm64" { + return false + } + case "darwin", "ios": + if runtime.GOARCH == "arm64" { + return false + } + } + return true +} + +// MustInternalLink checks that the current system can link programs with internal +// linking. +// If not, MustInternalLink calls t.Skip with an explanation. +func MustInternalLink(t testing.TB) { + if !CanInternalLink() { + t.Skipf("skipping test: internal linking on %s/%s is not supported", runtime.GOOS, runtime.GOARCH) + } +} + // HasSymlink reports whether the current system can use os.Symlink. func HasSymlink() bool { ok, _ := hasSymlink() diff --git a/src/os/exec/exec_test.go b/src/os/exec/exec_test.go index dafbc64a17..9746722980 100644 --- a/src/os/exec/exec_test.go +++ b/src/os/exec/exec_test.go @@ -605,6 +605,10 @@ func TestExtraFiles(t *testing.T) { testenv.MustHaveExec(t) testenv.MustHaveGoBuild(t) + // This test runs with cgo disabled. External linking needs cgo, so + // it doesn't work if external linking is required. + testenv.MustInternalLink(t) + if runtime.GOOS == "windows" { t.Skipf("skipping test on %q", runtime.GOOS) } diff --git a/src/runtime/crash_test.go b/src/runtime/crash_test.go index eae4f538c1..5e22b7593e 100644 --- a/src/runtime/crash_test.go +++ b/src/runtime/crash_test.go @@ -181,6 +181,9 @@ func TestCrashHandler(t *testing.T) { } func testDeadlock(t *testing.T, name string) { + // External linking brings in cgo, causing deadlock detection not working. + testenv.MustInternalLink(t) + output := runTestProg(t, "testprog", name) want := "fatal error: all goroutines are asleep - deadlock!\n" if !strings.HasPrefix(output, want) { @@ -205,6 +208,9 @@ func TestLockedDeadlock2(t *testing.T) { } func TestGoexitDeadlock(t *testing.T) { + // External linking brings in cgo, causing deadlock detection not working. + testenv.MustInternalLink(t) + output := runTestProg(t, "testprog", "GoexitDeadlock") want := "no goroutines (main called runtime.Goexit) - deadlock!" if !strings.Contains(output, want) { @@ -290,6 +296,9 @@ func TestRecursivePanic4(t *testing.T) { } func TestGoexitCrash(t *testing.T) { + // External linking brings in cgo, causing deadlock detection not working. + testenv.MustInternalLink(t) + output := runTestProg(t, "testprog", "GoexitExit") want := "no goroutines (main called runtime.Goexit) - deadlock!" if !strings.Contains(output, want) { @@ -348,6 +357,9 @@ func TestBreakpoint(t *testing.T) { } func TestGoexitInPanic(t *testing.T) { + // External linking brings in cgo, causing deadlock detection not working. + testenv.MustInternalLink(t) + // see issue 8774: this code used to trigger an infinite recursion output := runTestProg(t, "testprog", "GoexitInPanic") want := "fatal error: no goroutines (main called runtime.Goexit) - deadlock!" @@ -412,6 +424,9 @@ func TestPanicAfterGoexit(t *testing.T) { } func TestRecoveredPanicAfterGoexit(t *testing.T) { + // External linking brings in cgo, causing deadlock detection not working. + testenv.MustInternalLink(t) + output := runTestProg(t, "testprog", "RecoveredPanicAfterGoexit") want := "fatal error: no goroutines (main called runtime.Goexit) - deadlock!" if !strings.HasPrefix(output, want) { @@ -420,6 +435,9 @@ func TestRecoveredPanicAfterGoexit(t *testing.T) { } func TestRecoverBeforePanicAfterGoexit(t *testing.T) { + // External linking brings in cgo, causing deadlock detection not working. + testenv.MustInternalLink(t) + t.Parallel() output := runTestProg(t, "testprog", "RecoverBeforePanicAfterGoexit") want := "fatal error: no goroutines (main called runtime.Goexit) - deadlock!" @@ -429,6 +447,9 @@ func TestRecoverBeforePanicAfterGoexit(t *testing.T) { } func TestRecoverBeforePanicAfterGoexit2(t *testing.T) { + // External linking brings in cgo, causing deadlock detection not working. + testenv.MustInternalLink(t) + t.Parallel() output := runTestProg(t, "testprog", "RecoverBeforePanicAfterGoexit2") want := "fatal error: no goroutines (main called runtime.Goexit) - deadlock!" diff --git a/src/runtime/time_test.go b/src/runtime/time_test.go index a8dab7db8e..afd9af2af4 100644 --- a/src/runtime/time_test.go +++ b/src/runtime/time_test.go @@ -20,6 +20,10 @@ func TestFakeTime(t *testing.T) { t.Skip("faketime not supported on windows") } + // Faketime is advanced in checkdead. External linking brings in cgo, + // causing checkdead not working. + testenv.MustInternalLink(t) + t.Parallel() exe, err := buildTestProg(t, "testfaketime", "-tags=faketime") -- cgit v1.3 From 5faa8286512db8b11ba3f16c447dbf41f289b47a Mon Sep 17 00:00:00 2001 From: Cherry Zhang Date: Fri, 16 Oct 2020 20:25:54 -0400 Subject: cmd/link: use GOOS=ios for TestBuildForTvOS Updates #38485. Fix darwin-amd64-10_15 build. Change-Id: I1833c23788acafc9530bb91fb6182fc5cb44f6cd Reviewed-on: https://go-review.googlesource.com/c/go/+/263265 Trust: Cherry Zhang Run-TryBot: Cherry Zhang Reviewed-by: Ian Lance Taylor --- src/cmd/link/link_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/cmd/link/link_test.go') diff --git a/src/cmd/link/link_test.go b/src/cmd/link/link_test.go index 6729568766..968da4837d 100644 --- a/src/cmd/link/link_test.go +++ b/src/cmd/link/link_test.go @@ -309,7 +309,7 @@ func TestBuildForTvOS(t *testing.T) { cmd := exec.Command(testenv.GoToolPath(t), "build", "-buildmode=c-archive", "-o", ar, lib) cmd.Env = append(os.Environ(), "CGO_ENABLED=1", - "GOOS=darwin", + "GOOS=ios", "GOARCH=arm64", "CC="+strings.Join(CC, " "), "CGO_CFLAGS=", // ensure CGO_CFLAGS does not contain any flags. Issue #35459 -- cgit v1.3 From 2ad44158af373f68c1aef528738a5baade77d316 Mon Sep 17 00:00:00 2001 From: Cherry Zhang Date: Thu, 22 Oct 2020 10:05:44 -0400 Subject: cmd/internal/obj: use correct symbol size for Hashed64 classification Use sym.Size, instead of len(sym.P), to decide whether a content-addressable symbol is "short" and hashed as Hashed64. So we don't dedup a small symbol with a gigantic almost-zero symbol. Fixes #42140. Change-Id: Ic65869e1eaf51947517b3ece49c8b0be1b94bb75 Reviewed-on: https://go-review.googlesource.com/c/go/+/264337 Trust: Cherry Zhang Trust: Cuong Manh Le Run-TryBot: Cherry Zhang TryBot-Result: Go Bot Reviewed-by: Cuong Manh Le Reviewed-by: Than McIntosh --- src/cmd/internal/obj/sym.go | 2 +- src/cmd/link/link_test.go | 53 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) (limited to 'src/cmd/link/link_test.go') diff --git a/src/cmd/internal/obj/sym.go b/src/cmd/internal/obj/sym.go index 0182773f8e..4515bdd0d3 100644 --- a/src/cmd/internal/obj/sym.go +++ b/src/cmd/internal/obj/sym.go @@ -205,7 +205,7 @@ func (ctxt *Link) NumberSyms() { // if Pkgpath is unknown, cannot hash symbols with relocations, as it // may reference named symbols whose names are not fully expanded. if s.ContentAddressable() && (ctxt.Pkgpath != "" || len(s.R) == 0) { - if len(s.P) <= 8 && len(s.R) == 0 && !strings.HasPrefix(s.Name, "type.") { + if s.Size <= 8 && len(s.R) == 0 && !strings.HasPrefix(s.Name, "type.") { // We can use short hash only for symbols without relocations. // Don't use short hash for type symbols, as they need special handling. s.PkgIdx = goobj.PkgIdxHashed64 diff --git a/src/cmd/link/link_test.go b/src/cmd/link/link_test.go index 968da4837d..204410e976 100644 --- a/src/cmd/link/link_test.go +++ b/src/cmd/link/link_test.go @@ -820,3 +820,56 @@ func TestReadOnly(t *testing.T) { t.Errorf("running test program did not fail. output:\n%s", out) } } + +const testIssue38554Src = ` +package main + +type T [10<<20]byte + +//go:noinline +func f() T { + return T{} // compiler will make a large stmp symbol, but not used. +} + +func main() { + x := f() + println(x[1]) +} +` + +func TestIssue38554(t *testing.T) { + testenv.MustHaveGoBuild(t) + + t.Parallel() + + tmpdir, err := ioutil.TempDir("", "TestIssue38554") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(tmpdir) + + src := filepath.Join(tmpdir, "x.go") + err = ioutil.WriteFile(src, []byte(testIssue38554Src), 0666) + if err != nil { + t.Fatalf("failed to write source file: %v", err) + } + exe := filepath.Join(tmpdir, "x.exe") + cmd := exec.Command(testenv.GoToolPath(t), "build", "-o", exe, src) + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("build failed: %v\n%s", err, out) + } + + fi, err := os.Stat(exe) + if err != nil { + t.Fatalf("failed to stat output file: %v", err) + } + + // The test program is not much different from a helloworld, which is + // typically a little over 1 MB. We allow 5 MB. If the bad stmp is live, + // it will be over 10 MB. + const want = 5 << 20 + if got := fi.Size(); got > want { + t.Errorf("binary too big: got %d, want < %d", got, want) + } +} -- cgit v1.3 From 8e5778ed70ec3d371615a663520a586745fb7bee Mon Sep 17 00:00:00 2001 From: Than McIntosh Date: Thu, 5 Nov 2020 14:19:47 -0500 Subject: cmd/link: report error if builtin referenced but not defined When the compiler refers to a runtime builtin, it emits an indexed symbol reference in the object file via predetermined/preassigned ID within the PkgIdxBuiltin pseudo-package. At link time when the loader encounters these references, it redirects them to the corresponding defined symbol in the runtime package. This redirection process currently assumes that if a runtime builtin is referenced, we'll always have a definition for it. This assumption holds in most cases, however for the builtins "runtime.racefuncenter" and "runtime.racefuncexit", we'll only see definitions if the runtime package we're linking against was built with "-race". In the bug in question, build passes "-gcflags=-race" during compilation of the main package, but doesn't pass "-race" directly to 'go build', and as a result the final link combines a race-instrumented main with a non-race runtime; this results in R_CALL relocations with zero-valued target symbols, resulting in a panic during stack checking. This patch changes the loader's resolve method to detect situations where we're asking for builtin "runtime.X", but the runtime package read in doesn't contain a definition for X. Fixes #42396. Change-Id: Iafd38bd3b0f7f462868d120ccd4d7d1b88b27436 Reviewed-on: https://go-review.googlesource.com/c/go/+/267881 Trust: Than McIntosh Run-TryBot: Than McIntosh Reviewed-by: Jeremy Faller Reviewed-by: Cherry Zhang TryBot-Result: Go Bot --- src/cmd/link/internal/loader/loader.go | 29 ++++++++++++++++++- src/cmd/link/link_test.go | 52 ++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 1 deletion(-) (limited to 'src/cmd/link/link_test.go') diff --git a/src/cmd/link/internal/loader/loader.go b/src/cmd/link/internal/loader/loader.go index d861efcb13..971cc432ff 100644 --- a/src/cmd/link/internal/loader/loader.go +++ b/src/cmd/link/internal/loader/loader.go @@ -633,7 +633,11 @@ func (l *Loader) resolve(r *oReader, s goobj.SymRef) Sym { i := int(s.SymIdx) + r.ndef + r.nhashed64def + r.nhasheddef return r.syms[i] case goobj.PkgIdxBuiltin: - return l.builtinSyms[s.SymIdx] + if bi := l.builtinSyms[s.SymIdx]; bi != 0 { + return bi + } + l.reportMissingBuiltin(int(s.SymIdx), r.unit.Lib.Pkg) + return 0 case goobj.PkgIdxSelf: rr = r default: @@ -642,6 +646,29 @@ func (l *Loader) resolve(r *oReader, s goobj.SymRef) Sym { return l.toGlobal(rr, s.SymIdx) } +// reportMissingBuiltin issues an error in the case where we have a +// relocation against a runtime builtin whose definition is not found +// when the runtime package is built. The canonical example is +// "runtime.racefuncenter" -- currently if you do something like +// +// go build -gcflags=-race myprogram.go +// +// the compiler will insert calls to the builtin runtime.racefuncenter, +// but the version of the runtime used for linkage won't actually contain +// definitions of that symbol. See issue #42396 for details. +// +// As currently implemented, this is a fatal error. This has drawbacks +// in that if there are multiple missing builtins, the error will only +// cite the first one. On the plus side, terminating the link here has +// advantages in that we won't run the risk of panics or crashes later +// on in the linker due to R_CALL relocations with 0-valued target +// symbols. +func (l *Loader) reportMissingBuiltin(bsym int, reflib string) { + bname, _ := goobj.BuiltinName(bsym) + log.Fatalf("reference to undefined builtin %q from package %q", + bname, reflib) +} + // Look up a symbol by name, return global index, or 0 if not found. // This is more like Syms.ROLookup than Lookup -- it doesn't create // new symbol. diff --git a/src/cmd/link/link_test.go b/src/cmd/link/link_test.go index 204410e976..158c670739 100644 --- a/src/cmd/link/link_test.go +++ b/src/cmd/link/link_test.go @@ -7,6 +7,7 @@ package main import ( "bufio" "bytes" + "cmd/internal/sys" "debug/macho" "internal/testenv" "io/ioutil" @@ -873,3 +874,54 @@ func TestIssue38554(t *testing.T) { t.Errorf("binary too big: got %d, want < %d", got, want) } } + +const testIssue42396src = ` +package main + +//go:noinline +//go:nosplit +func callee(x int) { +} + +func main() { + callee(9) +} +` + +func TestIssue42396(t *testing.T) { + testenv.MustHaveGoBuild(t) + + if !sys.RaceDetectorSupported(runtime.GOOS, runtime.GOARCH) { + t.Skip("no race detector support") + } + + t.Parallel() + + tmpdir, err := ioutil.TempDir("", "TestIssue42396") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(tmpdir) + + src := filepath.Join(tmpdir, "main.go") + err = ioutil.WriteFile(src, []byte(testIssue42396src), 0666) + if err != nil { + t.Fatalf("failed to write source file: %v", err) + } + exe := filepath.Join(tmpdir, "main.exe") + cmd := exec.Command(testenv.GoToolPath(t), "build", "-gcflags=-race", "-o", exe, src) + out, err := cmd.CombinedOutput() + if err == nil { + t.Fatalf("build unexpectedly succeeded") + } + + // Check to make sure that we see a reasonable error message + // and not a panic. + if strings.Contains(string(out), "panic:") { + t.Fatalf("build should not fail with panic:\n%s", out) + } + const want = "reference to undefined builtin" + if !strings.Contains(string(out), want) { + t.Fatalf("error message incorrect: expected it to contain %q but instead got:\n%s\n", want, out) + } +} -- cgit v1.3