cmd/compile: tweak inliners handling of coverage counter updates

This patch fixes up a bug in the inliner's special case code for
coverage counter updates, which was not properly working for
-covermode=atomic compilations.

Updates #56044.

Change-Id: I9e309312b123121c3df02862623bdbab1f6c6a4b
Reviewed-on: https://go-review.googlesource.com/c/go/+/441858
Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: Than McIntosh <thanm@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>

diff --git a/src/cmd/compile/internal/inline/inl.go b/src/cmd/compile/internal/inline/inl.go
index fe042dd024bf356f8383937fe4ea03de32683c7d..949924517a5f06d345aa2f28e5b3ce4f5de83492 100644 (file)
--- a/src/cmd/compile/internal/inline/inl.go
+++ b/src/cmd/compile/internal/inline/inl.go
@@ -37,7 +37,6 @@ import (
        "cmd/compile/internal/typecheck"
        "cmd/compile/internal/types"
        "cmd/internal/obj"
-       "cmd/internal/objabi"
        "cmd/internal/src"
 )
 
@@ -284,6 +283,19 @@ func (v *hairyVisitor) doNode(n ir.Node) bool {
                                        break
                                }
                        }
+                       // Special case for coverage counter updates; although
+                       // these correspond to real operations, we treat them as
+                       // zero cost for the moment. This is due to the existence
+                       // of tests that are sensitive to inlining-- if the
+                       // insertion of coverage instrumentation happens to tip a
+                       // given function over the threshold and move it from
+                       // "inlinable" to "not-inlinable", this can cause changes
+                       // in allocation behavior, which can then result in test
+                       // failures (a good example is the TestAllocations in
+                       // crypto/ed25519).
+                       if isAtomicCoverageCounterUpdate(n) {
+                               break
+                       }
                }
                if n.X.Op() == ir.OMETHEXPR {
                        if meth := ir.MethodExprName(n.X); meth != nil {
@@ -485,14 +497,8 @@ func (v *hairyVisitor) doNode(n ir.Node) bool {
                // then result in test failures (a good example is the
                // TestAllocations in crypto/ed25519).
                n := n.(*ir.AssignStmt)
-               if n.X.Op() == ir.OINDEX {
-                       n := n.X.(*ir.IndexExpr)
-                       if n.X.Op() == ir.ONAME && n.X.Type().IsArray() {
-                               n := n.X.(*ir.Name)
-                               if n.Linksym().Type == objabi.SCOVERAGE_COUNTER {
-                                       return false
-                               }
-                       }
+               if n.X.Op() == ir.OINDEX && isIndexingCoverageCounter(n) {
+                       return false
                }
        }
 
@@ -1539,3 +1545,40 @@ func doList(list []ir.Node, do func(ir.Node) bool) bool {
        }
        return false
 }
+
+// isIndexingCoverageCounter returns true if the specified node 'n' is indexing
+// into a coverage counter array.
+func isIndexingCoverageCounter(n ir.Node) bool {
+       if n.Op() != ir.OINDEX {
+               return false
+       }
+       ixn := n.(*ir.IndexExpr)
+       if ixn.X.Op() != ir.ONAME || !ixn.X.Type().IsArray() {
+               return false
+       }
+       nn := ixn.X.(*ir.Name)
+       return nn.CoverageCounter()
+}
+
+// isAtomicCoverageCounterUpdate examines the specified node to
+// determine whether it represents a call to sync/atomic.AddUint32 to
+// increment a coverage counter.
+func isAtomicCoverageCounterUpdate(cn *ir.CallExpr) bool {
+       if cn.X.Op() != ir.ONAME {
+               return false
+       }
+       name := cn.X.(*ir.Name)
+       if name.Class != ir.PFUNC {
+               return false
+       }
+       fn := name.Sym().Name
+       if name.Sym().Pkg.Path != "sync/atomic" || fn != "AddUint32" {
+               return false
+       }
+       if len(cn.Args) != 2 || cn.Args[0].Op() != ir.OADDR {
+               return false
+       }
+       adn := cn.Args[0].(*ir.AddrExpr)
+       v := isIndexingCoverageCounter(adn.X)
+       return v
+}

diff --git a/src/cmd/compile/internal/test/inl_test.go b/src/cmd/compile/internal/test/inl_test.go
index fd3b489d1329d9af95363b0521941aa3c97bcba3..285052c0dab0d3f916b2944dd266870c8fe81132 100644 (file)
--- a/src/cmd/compile/internal/test/inl_test.go
+++ b/src/cmd/compile/internal/test/inl_test.go
@@ -7,6 +7,7 @@ package test
 import (
        "bufio"
        "internal/buildcfg"
+       "internal/goexperiment"
        "internal/testenv"
        "io"
        "math/bits"
@@ -332,3 +333,57 @@ func TestIntendedInlining(t *testing.T) {
                t.Errorf("%s was not inlined: %s", fullName, reason)
        }
 }
+
+func collectInlCands(msgs string) map[string]struct{} {
+       rv := make(map[string]struct{})
+       lines := strings.Split(msgs, "\n")
+       re := regexp.MustCompile(`^\S+\s+can\s+inline\s+(\S+)`)
+       for _, line := range lines {
+               m := re.FindStringSubmatch(line)
+               if m != nil {
+                       rv[m[1]] = struct{}{}
+               }
+       }
+       return rv
+}
+
+func TestIssue56044(t *testing.T) {
+       if testing.Short() {
+               t.Skipf("skipping test: too long for short mode")
+       }
+       if !goexperiment.CoverageRedesign {
+               t.Skipf("skipping new coverage tests (experiment not enabled)")
+       }
+
+       testenv.MustHaveGoBuild(t)
+
+       modes := []string{"-covermode=set", "-covermode=atomic"}
+
+       for _, mode := range modes {
+               // Build the Go runtime with "-m", capturing output.
+               args := []string{"build", "-gcflags=runtime=-m", "runtime"}
+               cmd := exec.Command(testenv.GoToolPath(t), args...)
+               b, err := cmd.CombinedOutput()
+               if err != nil {
+                       t.Fatalf("build failed (%v): %s", err, b)
+               }
+               mbase := collectInlCands(string(b))
+
+               // Redo the build with -cover, also with "-m".
+               args = []string{"build", "-gcflags=runtime=-m", mode, "runtime"}
+               cmd = exec.Command(testenv.GoToolPath(t), args...)
+               b, err = cmd.CombinedOutput()
+               if err != nil {
+                       t.Fatalf("build failed (%v): %s", err, b)
+               }
+               mcov := collectInlCands(string(b))
+
+               // Make sure that there aren't any functions that are marked
+               // as inline candidates at base but not with coverage.
+               for k := range mbase {
+                       if _, ok := mcov[k]; !ok {
+                               t.Errorf("error: did not find %s in coverage -m output", k)
+                       }
+               }
+       }
+}

diff --git a/src/runtime/coverage/emitdata_test.go b/src/runtime/coverage/emitdata_test.go
index 818a67cbdbef1ba50a35ab6defd04862d436efa8..e74db3e33266da57a5da34fe99cc70bff56085a2 100644 (file)
--- a/src/runtime/coverage/emitdata_test.go
+++ b/src/runtime/coverage/emitdata_test.go
@@ -406,6 +406,13 @@ func TestApisOnNocoverBinary(t *testing.T) {
 }
 
 func TestIssue56006EmitDataRaceCoverRunningGoroutine(t *testing.T) {
+       if testing.Short() {
+               t.Skipf("skipping test: too long for short mode")
+       }
+       if !goexperiment.CoverageRedesign {
+               t.Skipf("skipping new coverage tests (experiment not enabled)")
+       }
+
        // This test requires "go test -race -cover", meaning that we need
        // go build, go run, and "-race" support.
        testenv.MustHaveGoRun(t)