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 <thanm@google.com>
Run-TryBot: Than McIntosh <thanm@google.com>
Reviewed-by: Jeremy Faller <jeremy@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Go Bot <gobot@golang.org>

diff --git a/src/cmd/link/internal/loader/loader.go b/src/cmd/link/internal/loader/loader.go
index d861efcb13a3b303b0bda1836a0ad86be7c7c122..971cc432ff54b10921202e6a96df60494131a039 100644 (file)
--- 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 204410e9768135682b169b8ec281ae47c76b0ffc..158c6707391bb9a3654b8a087c8bcd2b991d5fd3 100644 (file)
--- 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)
+       }
+}