]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/compile: special case coverage vars in pkg init order
authorThan McIntosh <thanm@google.com>
Tue, 18 Oct 2022 15:28:52 +0000 (11:28 -0400)
committerThan McIntosh <thanm@google.com>
Tue, 18 Oct 2022 18:14:15 +0000 (18:14 +0000)
When computing package initialization order, special case the counter
variables inserted by "cmd/cover" for coverage instrumentation, since
their presence can perturb the order in which variables are
initialized in ways that are user-visible and incorrect with respect
to the original (uninstrumented) program.

Fixes #56293.

Change-Id: Ieec9239ded4f8e2503ff9bbe0fe171afb771b335
Reviewed-on: https://go-review.googlesource.com/c/go/+/443715
Run-TryBot: Than McIntosh <thanm@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: David Chase <drchase@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

src/cmd/compile/internal/coverage/cover.go
src/cmd/compile/internal/gc/main.go
src/cmd/compile/internal/pkginit/initorder.go
src/cmd/go/testdata/script/cover_var_init_order.txt [new file with mode: 0644]

index 65388072c7c65ccf5e8b70ec35598f3ce2c9f89c..688728d53a23dd8642fb486c1cdb8f2345309fe2 100644 (file)
@@ -4,6 +4,13 @@
 
 package coverage
 
+// This package contains support routines for coverage "fixup" in the
+// compiler, which happens when compiling a package whose source code
+// has been run through "cmd/cover" to add instrumentation. The two
+// important entry points are FixupVars (called prior to package init
+// generation) and FixupInit (called following package init
+// generation).
+
 import (
        "cmd/compile/internal/base"
        "cmd/compile/internal/ir"
@@ -15,30 +22,23 @@ import (
        "strings"
 )
 
-// Fixup is the main entry point for coverage compiler fixup. It
-// collects and reclassifies the variables mentioned in the
-// -coveragecfg file, then adds calls to the pkg init function as
-// appropriate to register the proper variables with the runtime.
-func Fixup() {
-       metavar, pkgIdVar, initfn, covermode, covergran :=
-               fixupMetaAndCounterVariables()
-       hashv, len := metaHashAndLen()
-       if covermode != coverage.CtrModeTestMain {
-               registerMeta(metavar, initfn, hashv, len,
-                       pkgIdVar, covermode, covergran)
-       }
-       if base.Ctxt.Pkgpath == "main" {
-               addInitHookCall(initfn, covermode)
-       }
+// Names records state information collected in the first fixup
+// phase so that it can be passed to the second fixup phase.
+type Names struct {
+       MetaVar     *ir.Name
+       PkgIdVar    *ir.Name
+       InitFn      *ir.Func
+       CounterMode coverage.CounterMode
+       CounterGran coverage.CounterGranularity
 }
 
-// fixupMetaAndCounterVariables collects and returns the package ID
-// and meta-data variables being used for this "-cover" build, along
-// with the init function for the package and the coverage mode. It
-// also reclassifies certain variables (for example, tagging coverage
-// counter variables with flags so that they can be handled properly
-// downstream).
-func fixupMetaAndCounterVariables() (*ir.Name, *ir.Name, *ir.Func, coverage.CounterMode, coverage.CounterGranularity) {
+// FixupVars is the first of two entry points for coverage compiler
+// fixup. It collects and returns the package ID and meta-data
+// variables being used for this "-cover" build, along with the
+// coverage counter mode and granularity. It also reclassifies selected
+// variables (for example, tagging coverage counter variables with
+// flags so that they can be handled properly downstream).
+func FixupVars() Names {
        metaVarName := base.Flag.Cfg.CoverageInfo.MetaVar
        pkgIdVarName := base.Flag.Cfg.CoverageInfo.PkgIdVar
        counterMode := base.Flag.Cfg.CoverageInfo.CounterMode
@@ -46,7 +46,6 @@ func fixupMetaAndCounterVariables() (*ir.Name, *ir.Name, *ir.Func, coverage.Coun
        counterPrefix := base.Flag.Cfg.CoverageInfo.CounterPrefix
        var metavar *ir.Name
        var pkgidvar *ir.Name
-       var initfn *ir.Func
 
        ckTypSanity := func(nm *ir.Name, tag string) {
                if nm.Type() == nil || nm.Type().HasPointers() {
@@ -55,13 +54,6 @@ func fixupMetaAndCounterVariables() (*ir.Name, *ir.Name, *ir.Func, coverage.Coun
        }
 
        for _, n := range typecheck.Target.Decls {
-               if fn, ok := n.(*ir.Func); ok && ir.FuncName(fn) == "init" {
-                       if initfn != nil {
-                               panic("unexpected")
-                       }
-                       initfn = fn
-                       continue
-               }
                as, ok := n.(*ir.AssignStmt)
                if !ok {
                        continue
@@ -108,7 +100,35 @@ func fixupMetaAndCounterVariables() (*ir.Name, *ir.Name, *ir.Func, coverage.Coun
                        counterGran)
        }
 
-       return metavar, pkgidvar, initfn, cm, cg
+       return Names{
+               MetaVar:     metavar,
+               PkgIdVar:    pkgidvar,
+               CounterMode: cm,
+               CounterGran: cg,
+       }
+}
+
+// FixupInit is the second main entry point for coverage compiler
+// fixup. It adds calls to the pkg init function as appropriate to
+// register coverage-related variables with the runtime.
+func FixupInit(cnames Names) {
+       for _, n := range typecheck.Target.Decls {
+               if fn, ok := n.(*ir.Func); ok && ir.FuncName(fn) == "init" {
+                       cnames.InitFn = fn
+                       break
+               }
+       }
+       if cnames.InitFn == nil {
+               panic("unexpected (no init func for -cover build)")
+       }
+
+       hashv, len := metaHashAndLen()
+       if cnames.CounterMode != coverage.CtrModeTestMain {
+               registerMeta(cnames, hashv, len)
+       }
+       if base.Ctxt.Pkgpath == "main" {
+               addInitHookCall(cnames.InitFn, cnames.CounterMode)
+       }
 }
 
 func metaHashAndLen() ([16]byte, int) {
@@ -132,19 +152,19 @@ func metaHashAndLen() ([16]byte, int) {
        return hv, base.Flag.Cfg.CoverageInfo.MetaLen
 }
 
-func registerMeta(mdname *ir.Name, initfn *ir.Func, hash [16]byte, mdlen int, pkgIdVar *ir.Name, cmode coverage.CounterMode, cgran coverage.CounterGranularity) {
+func registerMeta(cnames Names, hashv [16]byte, mdlen int) {
        // Materialize expression for hash (an array literal)
-       pos := initfn.Pos()
+       pos := cnames.InitFn.Pos()
        elist := make([]ir.Node, 0, 16)
        for i := 0; i < 16; i++ {
-               elem := ir.NewInt(int64(hash[i]))
+               elem := ir.NewInt(int64(hashv[i]))
                elist = append(elist, elem)
        }
        ht := types.NewArray(types.Types[types.TUINT8], 16)
        hashx := ir.NewCompLitExpr(pos, ir.OCOMPLIT, ht, elist)
 
        // Materalize expression corresponding to address of the meta-data symbol.
-       mdax := typecheck.NodAddr(mdname)
+       mdax := typecheck.NodAddr(cnames.MetaVar)
        mdauspx := typecheck.ConvNop(mdax, types.Types[types.TUNSAFEPTR])
 
        // Materialize expression for length.
@@ -157,20 +177,20 @@ func registerMeta(mdname *ir.Name, initfn *ir.Func, hash [16]byte, mdlen int, pk
        fn := typecheck.LookupRuntime("addCovMeta")
        pkid := coverage.HardCodedPkgID(base.Ctxt.Pkgpath)
        pkIdNode := ir.NewInt(int64(pkid))
-       cmodeNode := ir.NewInt(int64(cmode))
-       cgranNode := ir.NewInt(int64(cgran))
+       cmodeNode := ir.NewInt(int64(cnames.CounterMode))
+       cgranNode := ir.NewInt(int64(cnames.CounterGran))
        pkPathNode := ir.NewString(base.Ctxt.Pkgpath)
        callx := typecheck.Call(pos, fn, []ir.Node{mdauspx, lenx, hashx,
                pkPathNode, pkIdNode, cmodeNode, cgranNode}, false)
        assign := callx
        if pkid == coverage.NotHardCoded {
-               assign = typecheck.Stmt(ir.NewAssignStmt(pos, pkgIdVar, callx))
+               assign = typecheck.Stmt(ir.NewAssignStmt(pos, cnames.PkgIdVar, callx))
        }
 
        // Tack the call onto the start of our init function. We do this
        // early in the init since it's possible that instrumented function
        // bodies (with counter updates) might be inlined into init.
-       initfn.Body.Prepend(assign)
+       cnames.InitFn.Body.Prepend(assign)
 }
 
 // addInitHookCall generates a call to runtime/coverage.initHook() and
index 570f632eec8218e3d6b1b2d317e8e27929bbdb26..2fbf2f49d50e5ec1f1dbdfc8ccf7158e11911302 100644 (file)
@@ -203,6 +203,12 @@ func Main(archInit func(*ssagen.ArchInfo)) {
        // because it generates itabs for initializing global variables.
        ssagen.InitConfig()
 
+       // First part of coverage fixup (if applicable).
+       var cnames coverage.Names
+       if base.Flag.Cfg.CoverageInfo != nil {
+               cnames = coverage.FixupVars()
+       }
+
        // Create "init" function for package-scope variable initialization
        // statements, if any.
        //
@@ -212,9 +218,10 @@ func Main(archInit func(*ssagen.ArchInfo)) {
        // removal can skew the results (e.g., #43444).
        pkginit.MakeInit()
 
-       // Fix up init routines if building for code coverage.
+       // Second part of code coverage fixup (init func modification),
+       // if applicable.
        if base.Flag.Cfg.CoverageInfo != nil {
-               coverage.Fixup()
+               coverage.FixupInit(cnames)
        }
 
        // Eliminate some obviously dead code.
index 6290a8f314b7a34c40a82683bd925b6d461e2f31..426d2985ab3f0305955ec1786927dd0c6a5d7133 100644 (file)
@@ -320,6 +320,15 @@ func (d *initDeps) foundDep(n *ir.Name) {
                return
        }
 
+       // Treat coverage counter variables effectively as invisible with
+       // respect to init order. If we don't do this, then the
+       // instrumentation vars can perturb the order of initialization
+       // away from the order of the original uninstrumented program.
+       // See issue #56293 for more details.
+       if n.CoverageCounter() || n.CoverageAuxVar() {
+               return
+       }
+
        if d.seen.Has(n) {
                return
        }
diff --git a/src/cmd/go/testdata/script/cover_var_init_order.txt b/src/cmd/go/testdata/script/cover_var_init_order.txt
new file mode 100644 (file)
index 0000000..37e07b7
--- /dev/null
@@ -0,0 +1,55 @@
+# This test verifies that issue 56293 has been fixed, and that the
+# insertion of coverage instrumentation doesn't perturb package
+# initialization order.
+
+[short] skip
+
+# Skip if new coverage is turned off.
+[!GOEXPERIMENT:coverageredesign] skip
+
+go test -cover example
+
+-- go.mod --
+module example
+
+go 1.20
+
+-- m.go --
+
+package main
+
+import (
+       "flag"
+)
+
+var (
+       fooFlag = flag.String("foo", "", "this should be ok")
+       foo     = flag.Lookup("foo")
+
+       barFlag = flag.String("bar", "", "this should be also ok, but is "+notOK()+".")
+       bar     = flag.Lookup("bar")
+)
+
+func notOK() string {
+       return "not OK"
+}
+
+-- m_test.go --
+
+package main
+
+import (
+       "testing"
+)
+
+func TestFoo(t *testing.T) {
+       if foo == nil {
+               t.Fatal()
+       }
+}
+
+func TestBar(t *testing.T) {
+       if bar == nil {
+               t.Fatal()
+       }
+}