cmd/compile: experimental loop iterator capture semantics change

Adds:
GOEXPERIMENT=loopvar (expected way of invoking)
-d=loopvar={-1,0,1,2,11,12} (for per-package control and/or logging)
-d=loopvarhash=... (for hash debugging)

loopvar=11,12 are for testing, benchmarking, and debugging.

If enabled,for loops of the form `for x,y := range thing`, if x and/or
y are addressed or captured by a closure, are transformed by renaming
x/y to a temporary and prepending an assignment to the body of the
loop x := tmp_x.  This changes the loop semantics by making each
iteration's instance of x be distinct from the others (currently they
are all aliased, and when this matters, it is almost always a bug).

3-range with captured iteration variables are also transformed,
though it is a more complex transformation.

"Optimized" to do a simpler transformation for
3-clause for where the increment is empty.

(Prior optimization of address-taking under Return disabled, because
it was incorrect; returns can have loops for children.  Restored in
a later CL.)

Includes support for -d=loopvarhash=<binary string> intended for use
with hash search and GOCOMPILEDEBUG=loopvarhash=<binary string>
(use `gossahash -e loopvarhash command-that-fails`).

Minor feature upgrades to hash-triggered features; clients can specify
that file-position hashes use only the most-inline position, and/or that
they use only the basenames of source files (not the full directory path).
Most-inlined is the right choice for debugging loop-iteration change
once the semantics are linked to the package across inlining; basename-only
makes it tractable to write tests (which, otherwise, depend on the full
pathname of the source file and thus vary).

Updates #57969.

Change-Id: I180a51a3f8d4173f6210c861f10de23de8a1b1db
Reviewed-on: https://go-review.googlesource.com/c/go/+/411904
Reviewed-by: Matthew Dempsky <[email protected]>
Run-TryBot: David Chase <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>

Author: dr2chase

src/cmd/compile/internal/base/debug.go

@@ -33,6 +33,8 @@ type DebugFlags struct {
 	InlStaticInit         int    `help:"allow static initialization of inlined calls" concurrent:"ok"`
 	InterfaceCycles       int    `help:"allow anonymous interface cycles"`
 	Libfuzzer             int    `help:"enable coverage instrumentation for libfuzzer"`
+	LoopVar               int    `help:"shared (0, default), 1 (private loop variables), 2, private + log"`
+	LoopVarHash           string `help:"for debugging changes in loop behavior. Overrides experiment and loopvar flag."`
 	LocationLists         int    `help:"print information about DWARF location list creation"`
 	Nil                   int    `help:"print information about nil checks"`
 	NoOpenDefer           int    `help:"disable open-coded defers" concurrent:"ok"`

src/cmd/compile/internal/base/flag.go

@@ -186,7 +186,61 @@ func ParseFlags() {
 	}
 
 	if Debug.Gossahash != "" {
-		hashDebug = NewHashDebug("gosshash", Debug.Gossahash, nil)
+		hashDebug = NewHashDebug("gossahash", Debug.Gossahash, nil)
+	}
+
+	// Three inputs govern loop iteration variable rewriting, hash, experiment, flag.
+	// The loop variable rewriting is:
+	// IF non-empty hash, then hash determines behavior (function+line match) (*)
+	// ELSE IF experiment and flag==0, then experiment (set flag=1)
+	// ELSE flag (note that build sets flag per-package), with behaviors:
+	//  -1 => no change to behavior.
+	//   0 => no change to behavior (unless non-empty hash, see above)
+	//   1 => apply change to likely-iteration-variable-escaping loops
+	//   2 => apply change, log results
+	//   11 => apply change EVERYWHERE, do not log results (for debugging/benchmarking)
+	//   12 => apply change EVERYWHERE, log results (for debugging/benchmarking)
+	//
+	// The expected uses of the these inputs are, in believed most-likely to least likely:
+	//  GOEXPERIMENT=loopvar -- apply change to entire application
+	//  -gcflags=some_package=-d=loopvar=1 -- apply change to some_package (**)
+	//  -gcflags=some_package=-d=loopvar=2 -- apply change to some_package, log it
+	//  GOEXPERIMENT=loopvar -gcflags=some_package=-d=loopvar=-1 -- apply change to all but one package
+	//  GOCOMPILEDEBUG=loopvarhash=... -- search for failure cause
+	//
+	//  (*) For debugging purposes, providing loopvar flag >= 11 will expand the hash-eligible set of loops to all.
+	// (**) Currently this applies to all code in the compilation of some_package, including
+	//     inlines from other packages that may have been compiled w/o the change.
+
+	if Debug.LoopVarHash != "" {
+		// This first little bit controls the inputs for debug-hash-matching.
+		basenameOnly := false
+		mostInlineOnly := true
+		if strings.HasPrefix(Debug.LoopVarHash, "FS") {
+			// Magic handshake for testing, use file suffixes only when hashing on a position.
+			// i.e., rather than /tmp/asdfasdfasdf/go-test-whatever/foo_test.go,
+			// hash only on "foo_test.go", so that it will be the same hash across all runs.
+			Debug.LoopVarHash = Debug.LoopVarHash[2:]
+			basenameOnly = true
+		}
+		if strings.HasPrefix(Debug.LoopVarHash, "IL") {
+			// When hash-searching on a position that is an inline site, default is to use the
+			// most-inlined position only.  This makes the hash faster, plus there's no point
+			// reporting a problem with all the inlining; there's only one copy of the source.
+			// However, if for some reason you wanted it per-site, you can get this.  (The default
+			// hash-search behavior for compiler debugging is at an inline site.)
+			Debug.LoopVarHash = Debug.LoopVarHash[2:]
+			mostInlineOnly = false
+		}
+		// end of testing trickiness
+		LoopVarHash = NewHashDebug("loopvarhash", Debug.LoopVarHash, nil)
+		if Debug.LoopVar < 11 { // >= 11 means all loops are rewrite-eligible
+			Debug.LoopVar = 1 // 1 means those loops that syntactically escape their dcl vars are eligible.
+		}
+		LoopVarHash.SetInlineSuffixOnly(mostInlineOnly)
+		LoopVarHash.SetFileSuffixOnly(basenameOnly)
+	} else if buildcfg.Experiment.LoopVar && Debug.LoopVar == 0 {
+		Debug.LoopVar = 1
 	}
 
 	if Debug.Fmahash != "" {

src/cmd/compile/internal/base/hashdebug.go

@@ -12,6 +12,7 @@ import (
 	"fmt"
 	"io"
 	"os"
+	"path/filepath"
 	"strconv"
 	"strings"
 	"sync"
@@ -34,16 +35,38 @@ type HashDebug struct {
 	name string     // base name of the flag/variable.
 	// what file (if any) receives the yes/no logging?
 	// default is os.Stdout
-	logfile  writeSyncer
-	posTmp   []src.Pos
-	bytesTmp bytes.Buffer
-	matches  []hashAndMask // A hash matches if one of these matches.
-	yes, no  bool
+	logfile          writeSyncer
+	posTmp           []src.Pos
+	bytesTmp         bytes.Buffer
+	matches          []hashAndMask // A hash matches if one of these matches.
+	yes, no          bool
+	fileSuffixOnly   bool // for Pos hashes, remove the directory prefix.
+	inlineSuffixOnly bool // for Pos hashes, remove all but the most inline position.
+}
+
+// SetFileSuffixOnly controls whether hashing and reporting use the entire
+// file path name, just the basename.  This makes hashing more consistent,
+// at the expense of being able to certainly locate the file.
+func (d *HashDebug) SetFileSuffixOnly(b bool) *HashDebug {
+	d.fileSuffixOnly = b
+	return d
+}
+
+// SetInlineSuffixOnly controls whether hashing and reporting use the entire
+// inline position, or just the most-inline suffix.  Compiler debugging tends
+// to want the whole inlining, debugging user problems (loopvarhash, e.g.)
+// typically does not need to see the entire inline tree, there is just one
+// copy of the source code.
+func (d *HashDebug) SetInlineSuffixOnly(b bool) *HashDebug {
+	d.inlineSuffixOnly = b
+	return d
 }
 
 // The default compiler-debugging HashDebug, for "-d=gossahash=..."
 var hashDebug *HashDebug
-var FmaHash *HashDebug
+
+var FmaHash *HashDebug     // for debugging fused-multiply-add floating point changes
+var LoopVarHash *HashDebug // for debugging shared/private loop variable changes
 
 // DebugHashMatch reports whether debug variable Gossahash
 //
@@ -56,10 +79,10 @@ var FmaHash *HashDebug
 //  4. is a suffix of the sha1 hash of pkgAndName (returns true)
 //
 //  5. OR
-//     if the value is in the regular language "[01]+(;[01]+)+"
+//     if the value is in the regular language "[01]+(/[01]+)+"
 //     test the [01]+ substrings after in order returning true
 //     for the first one that suffix-matches. The substrings AFTER
-//     the first semicolon are numbered 0,1, etc and are named
+//     the first slash are numbered 0,1, etc and are named
 //     fmt.Sprintf("%s%d", varname, number)
 //     Clause 5 is not really intended for human use and only
 //     matters for failures that require multiple triggers.
@@ -235,13 +258,20 @@ func (d *HashDebug) DebugHashMatchParam(pkgAndName string, param uint64) bool {
 // package name and path. The output trigger string is prefixed with "POS=" so
 // that tools processing the output can reliably tell the difference. The mutex
 // locking is also more frequent and more granular.
+// Note that the default answer for no environment variable (d == nil)
+// is "yes", do the thing.
 func (d *HashDebug) DebugHashMatchPos(ctxt *obj.Link, pos src.XPos) bool {
 	if d == nil {
 		return true
 	}
 	if d.no {
 		return false
 	}
+	// Written this way to make inlining likely.
+	return d.debugHashMatchPos(ctxt, pos)
+}
+
+func (d *HashDebug) debugHashMatchPos(ctxt *obj.Link, pos src.XPos) bool {
 	d.mu.Lock()
 	defer d.mu.Unlock()
 
@@ -278,9 +308,17 @@ func (d *HashDebug) bytesForPos(ctxt *obj.Link, pos src.XPos) []byte {
 	// Reverse posTmp to put outermost first.
 	b := &d.bytesTmp
 	b.Reset()
-	for i := len(d.posTmp) - 1; i >= 0; i-- {
+	start := len(d.posTmp) - 1
+	if d.inlineSuffixOnly {
+		start = 0
+	}
+	for i := start; i >= 0; i-- {
 		p := &d.posTmp[i]
-		fmt.Fprintf(b, "%s:%d:%d", p.Filename(), p.Line(), p.Col())
+		f := p.Filename()
+		if d.fileSuffixOnly {
+			f = filepath.Base(f)
+		}
+		fmt.Fprintf(b, "%s:%d:%d", f, p.Line(), p.Col())
 		if i != 0 {
 			b.WriteByte(';')
 		}

src/cmd/compile/internal/escape/stmt.go

@@ -64,7 +64,7 @@ func (e *escape) stmt(n ir.Node) {
 			}
 			e.loopDepth++
 		default:
-			base.Fatalf("label missing tag")
+			base.Fatalf("label %v missing tag", n.Label)
 		}
 		delete(e.labels, n.Label)
 

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

@@ -16,6 +16,7 @@ import (
 	"cmd/compile/internal/inline"
 	"cmd/compile/internal/ir"
 	"cmd/compile/internal/logopt"
+	"cmd/compile/internal/loopvar"
 	"cmd/compile/internal/noder"
 	"cmd/compile/internal/pgo"
 	"cmd/compile/internal/pkginit"
@@ -265,10 +266,12 @@ func Main(archInit func(*ssagen.ArchInfo)) {
 	}
 	noder.MakeWrappers(typecheck.Target) // must happen after inlining
 
-	// Devirtualize.
+	// Devirtualize and get variable capture right in for loops
+	var transformed []*ir.Name
 	for _, n := range typecheck.Target.Decls {
 		if n.Op() == ir.ODCLFUNC {
 			devirtualize.Func(n.(*ir.Func))
+			transformed = append(transformed, loopvar.ForCapture(n.(*ir.Func))...)
 		}
 	}
 	ir.CurFunc = nil
@@ -293,6 +296,46 @@ func Main(archInit func(*ssagen.ArchInfo)) {
 	base.Timer.Start("fe", "escapes")
 	escape.Funcs(typecheck.Target.Decls)
 
+	if 2 <= base.Debug.LoopVar && base.Debug.LoopVar != 11 || logopt.Enabled() { // 11 is do them all, quietly, 12 includes debugging.
+		fileToPosBase := make(map[string]*src.PosBase) // used to remove inline context for innermost reporting.
+		for _, n := range transformed {
+			pos := n.Pos()
+			if logopt.Enabled() {
+				// For automated checking of coverage of this transformation, include this in the JSON information.
+				if n.Esc() == ir.EscHeap {
+					logopt.LogOpt(pos, "transform-escape", "loopvar", ir.FuncName(n.Curfn))
+				} else {
+					logopt.LogOpt(pos, "transform-noescape", "loopvar", ir.FuncName(n.Curfn))
+				}
+			}
+			inner := base.Ctxt.InnermostPos(pos)
+			outer := base.Ctxt.OutermostPos(pos)
+			if inner == outer {
+				if n.Esc() == ir.EscHeap {
+					base.WarnfAt(pos, "transformed loop variable %v escapes", n)
+				} else {
+					base.WarnfAt(pos, "transformed loop variable %v does not escape", n)
+				}
+			} else {
+				// Report the problem at the line where it actually occurred.
+				afn := inner.AbsFilename()
+				pb, ok := fileToPosBase[afn]
+				if !ok {
+					pb = src.NewFileBase(inner.Filename(), afn)
+					fileToPosBase[afn] = pb
+				}
+				inner.SetBase(pb) // rebasing w/o inline context makes it print correctly in WarnfAt; otherwise it prints as outer.
+				innerXPos := base.Ctxt.PosTable.XPos(inner)
+
+				if n.Esc() == ir.EscHeap {
+					base.WarnfAt(innerXPos, "transformed loop variable %v escapes (loop inlined into %s:%d)", n, outer.Filename(), outer.Line())
+				} else {
+					base.WarnfAt(innerXPos, "transformed loop variable %v does not escape (loop inlined into %s:%d)", n, outer.Filename(), outer.Line())
+				}
+			}
+		}
+	}
+
 	// Collect information for go:nowritebarrierrec
 	// checking. This must happen before transforming closures during Walk
 	// We'll do the final check after write barriers are

src/cmd/compile/internal/ir/stmt.go

@@ -163,6 +163,15 @@ func NewBranchStmt(pos src.XPos, op Op, label *types.Sym) *BranchStmt {
 	return n
 }
 
+func (n *BranchStmt) SetOp(op Op) {
+	switch op {
+	default:
+		panic(n.no("SetOp " + op.String()))
+	case OBREAK, OCONTINUE, OFALL, OGOTO:
+		n.op = op
+	}
+}
+
 func (n *BranchStmt) Sym() *types.Sym { return n.Label }
 
 // A CaseClause is a case statement in a switch or select: case List: Body.