cmd/compile: special case coverage vars in pkg init order

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>

4 files changed, 132 insertions, 41 deletions

diff --git a/src/cmd/compile/internal/coverage/cover.go b/src/cmd/compile/internal/coverage/cover.go
index 65388072c7..688728d53a 100644
--- a/src/cmd/compile/internal/coverage/cover.go
+++ b/src/cmd/compile/internal/coverage/cover.go
@@ -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
diff --git a/src/cmd/compile/internal/gc/main.go b/src/cmd/compile/internal/gc/main.go
index 570f632eec..2fbf2f49d5 100644
--- a/src/cmd/compile/internal/gc/main.go
+++ b/src/cmd/compile/internal/gc/main.go
@@ -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.
diff --git a/src/cmd/compile/internal/pkginit/initorder.go b/src/cmd/compile/internal/pkginit/initorder.go
index 6290a8f314..426d2985ab 100644
--- a/src/cmd/compile/internal/pkginit/initorder.go
+++ b/src/cmd/compile/internal/pkginit/initorder.go
@@ -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
index 0000000000..37e07b71f6
--- /dev/null
+++ b/src/cmd/go/testdata/script/cover_var_init_order.txt
@@ -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()
+	}
+}