cmd/compile: fix DWARF inline debug issue with dead local vars

Fix a problem in DWARF inline debug generation relating to handling of
statically unreachable local variables. For a function such as:

    const always = true

    func HasDeadLocal() int {
      if always {
        return 9
      }
      x := new(Something)
      ...
      return x.y
    }

the variable "x" is placed onto the Dcl list for the function during
parsing, but the actual declaration node is deleted later on when
gc.Main invokes "deadcode". Later in the compile the DWARF code emits
an abstract function with "x" (since "x" was on the Dcl list at the
point of the inline), but the export data emitted does not contain
"x". This then creates clashing/inconsistant DWARF abstract function
DIEs later on if HasDeadLocal is inlined in somewhere else.

As a fix, the inliner now pruned away variables such as "x" when
creating a copy of the Dcl list as part of the inlining; this means
that both the export data generator and the DWARF emitter wind up
seeing a consistent picture.

Fixes #25459

Change-Id: I753dc4e9f9ec694340adba5f43c907ba8cc9badc
Reviewed-on: https://go-review.googlesource.com/114090
Run-TryBot: Than McIntosh <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Heschi Kreinick <[email protected]>

Author: thanm

src/cmd/compile/internal/gc/inl.go

@@ -169,7 +169,20 @@ func caninl(fn *Node) {
 		cc = 1 // this appears to yield better performance than 0.
 	}
 
-	visitor := hairyVisitor{budget: inlineMaxBudget, extraCallCost: cc}
+	// At this point in the game the function we're looking at may
+	// have "stale" autos, vars that still appear in the Dcl list, but
+	// which no longer have any uses in the function body (due to
+	// elimination by deadcode). We'd like to exclude these dead vars
+	// when creating the "Inline.Dcl" field below; to accomplish this,
+	// the hairyVisitor below builds up a map of used/referenced
+	// locals, and we use this map to produce a pruned Inline.Dcl
+	// list. See issue 25249 for more context.
+
+	visitor := hairyVisitor{
+		budget:        inlineMaxBudget,
+		extraCallCost: cc,
+		usedLocals:    make(map[*Node]bool),
+	}
 	if visitor.visitList(fn.Nbody) {
 		reason = visitor.reason
 		return
@@ -181,7 +194,7 @@ func caninl(fn *Node) {
 
 	n.Func.Inl = &Inline{
 		Cost: inlineMaxBudget - visitor.budget,
-		Dcl:  inlcopylist(n.Name.Defn.Func.Dcl),
+		Dcl:  inlcopylist(pruneUnusedAutos(n.Name.Defn.Func.Dcl, &visitor)),
 		Body: inlcopylist(fn.Nbody.Slice()),
 	}
 
@@ -245,6 +258,7 @@ type hairyVisitor struct {
 	budget        int32
 	reason        string
 	extraCallCost int32
+	usedLocals    map[*Node]bool
 }
 
 // Look for anything we want to punt on.
@@ -374,6 +388,12 @@ func (v *hairyVisitor) visit(n *Node) bool {
 			return v.visitList(n.Ninit) || v.visitList(n.Nbody) ||
 				v.visitList(n.Rlist)
 		}
+
+	case ONAME:
+		if n.Class() == PAUTO {
+			v.usedLocals[n] = true
+		}
+
 	}
 
 	v.budget--
@@ -1250,3 +1270,16 @@ func (subst *inlsubst) updatedPos(xpos src.XPos) src.XPos {
 	pos.SetBase(newbase)
 	return Ctxt.PosTable.XPos(pos)
 }
+
+func pruneUnusedAutos(ll []*Node, vis *hairyVisitor) []*Node {
+	s := make([]*Node, 0, len(ll))
+	for _, n := range ll {
+		if n.Class() == PAUTO {
+			if _, found := vis.usedLocals[n]; !found {
+				continue
+			}
+		}
+		s = append(s, n)
+	}
+	return s
+}

src/cmd/link/internal/ld/dwarf_test.go

@@ -22,9 +22,9 @@ import (
 )
 
 const (
-	NoOpt        = "-gcflags=-l -N"
-	OptInl4      = "-gcflags=all=-l=4"
-	OptInl4DwLoc = "-gcflags=all=-l=4 -dwarflocationlists"
+	DefaultOpt = "-gcflags="
+	NoOpt      = "-gcflags=-l -N"
+	OptInl4    = "-gcflags=all=-l=4"
 )
 
 func TestRuntimeTypesPresent(t *testing.T) {
@@ -111,6 +111,38 @@ func gobuild(t *testing.T, dir string, testfile string, gcflags string) *builtFi
 	return &builtFile{f, dst}
 }
 
+func envWithGoPathSet(gp string) []string {
+	env := os.Environ()
+	for i := 0; i < len(env); i++ {
+		if strings.HasPrefix(env[i], "GOPATH=") {
+			env[i] = "GOPATH=" + gp
+			return env
+		}
+	}
+	env = append(env, "GOPATH="+gp)
+	return env
+}
+
+// Similar to gobuild() above, but runs off a separate GOPATH environment
+
+func gobuildTestdata(t *testing.T, tdir string, gopathdir string, packtobuild string, gcflags string) *builtFile {
+	dst := filepath.Join(tdir, "out.exe")
+
+	// Run a build with an updated GOPATH
+	cmd := exec.Command(testenv.GoToolPath(t), "build", gcflags, "-o", dst, packtobuild)
+	cmd.Env = envWithGoPathSet(gopathdir)
+	if b, err := cmd.CombinedOutput(); err != nil {
+		t.Logf("build: %s\n", b)
+		t.Fatalf("build error: %v", err)
+	}
+
+	f, err := objfilepkg.Open(dst)
+	if err != nil {
+		t.Fatal(err)
+	}
+	return &builtFile{f, dst}
+}
+
 func TestEmbeddedStructMarker(t *testing.T) {
 	testenv.MustHaveGoBuild(t)
 
@@ -684,37 +716,16 @@ func main() {
 	}
 }
 
-func abstractOriginSanity(t *testing.T, flags string) {
-	// Nothing special about net/http here, this is just a convenient
-	// way to pull in a lot of code.
-	const prog = `
-package main
-
-import (
-	"net/http"
-	"net/http/httptest"
-)
-
-type statusHandler int
+func abstractOriginSanity(t *testing.T, gopathdir string, flags string) {
 
-func (h *statusHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
-	w.WriteHeader(int(*h))
-}
-
-func main() {
-	status := statusHandler(http.StatusNotFound)
-	s := httptest.NewServer(&status)
-	defer s.Close()
-}
-`
 	dir, err := ioutil.TempDir("", "TestAbstractOriginSanity")
 	if err != nil {
 		t.Fatalf("could not create directory: %v", err)
 	}
 	defer os.RemoveAll(dir)
 
 	// Build with inlining, to exercise DWARF inlining support.
-	f := gobuild(t, dir, prog, flags)
+	f := gobuildTestdata(t, dir, gopathdir, "main", flags)
 
 	d, err := f.DWARF()
 	if err != nil {
@@ -790,10 +801,15 @@ func TestAbstractOriginSanity(t *testing.T) {
 		t.Skip("skipping on solaris and darwin, pending resolution of issue #23168")
 	}
 
-	abstractOriginSanity(t, OptInl4)
+	if wd, err := os.Getwd(); err == nil {
+		gopathdir := filepath.Join(wd, "testdata", "httptest")
+		abstractOriginSanity(t, gopathdir, OptInl4)
+	} else {
+		t.Fatalf("os.Getwd() failed %v", err)
+	}
 }
 
-func TestAbstractOriginSanityWithLocationLists(t *testing.T) {
+func TestAbstractOriginSanityIssue25459(t *testing.T) {
 	testenv.MustHaveGoBuild(t)
 
 	if runtime.GOOS == "plan9" {
@@ -806,7 +822,12 @@ func TestAbstractOriginSanityWithLocationLists(t *testing.T) {
 		t.Skip("skipping on not-amd64 not-x86; location lists not supported")
 	}
 
-	abstractOriginSanity(t, OptInl4DwLoc)
+	if wd, err := os.Getwd(); err == nil {
+		gopathdir := filepath.Join(wd, "testdata", "issue25459")
+		abstractOriginSanity(t, gopathdir, DefaultOpt)
+	} else {
+		t.Fatalf("os.Getwd() failed %v", err)
+	}
 }
 
 func TestRuntimeTypeAttr(t *testing.T) {

src/cmd/link/internal/ld/testdata/httptest/src/main/main.go

@@ -0,0 +1,22 @@
+// A small test program that uses the net/http package. There is
+// nothing special about net/http here, this is just a convenient way
+// to pull in a lot of code.
+
+package main
+
+import (
+	"net/http"
+	"net/http/httptest"
+)
+
+type statusHandler int
+
+func (h *statusHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+	w.WriteHeader(int(*h))
+}
+
+func main() {
+	status := statusHandler(http.StatusNotFound)
+	s := httptest.NewServer(&status)
+	defer s.Close()
+}

src/cmd/link/internal/ld/testdata/issue25459/src/a/a.go

@@ -0,0 +1,27 @@
+package a
+
+const Always = true
+
+var Count int
+
+type FuncReturningInt func() int
+
+var PointerToConstIf FuncReturningInt
+
+func ConstIf() int {
+	if Always {
+		return 1
+	}
+	var imdead [4]int
+	imdead[Count] = 1
+	return imdead[0]
+}
+
+func CallConstIf() int {
+	Count += 3
+	return ConstIf()
+}
+
+func Another() {
+	defer func() { PointerToConstIf = ConstIf; Count += 1 }()
+}

src/cmd/link/internal/ld/testdata/issue25459/src/main/main.go

@@ -0,0 +1,10 @@
+package main
+
+import "a"
+
+var Glob int
+
+func main() {
+	a.Another()
+	Glob += a.ConstIf() + a.CallConstIf()
+}