From 8e5778ed70ec3d371615a663520a586745fb7bee Mon Sep 17 00:00:00 2001 From: Than McIntosh Date: Thu, 5 Nov 2020 14:19:47 -0500 Subject: [PATCH] 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(-) 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) + } +} -- 2.50.0