From c20d95916357acf141159d4efc8894bfc14a2cd6 Mon Sep 17 00:00:00 2001 From: David Chase Date: Sun, 12 Jun 2022 15:33:57 -0400 Subject: [PATCH] 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= intended for use with hash search and GOCOMPILEDEBUG=loopvarhash= (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 Run-TryBot: David Chase TryBot-Result: Gopher Robot --- src/cmd/compile/internal/base/debug.go | 2 + src/cmd/compile/internal/base/flag.go | 56 ++- src/cmd/compile/internal/base/hashdebug.go | 58 ++- src/cmd/compile/internal/escape/stmt.go | 2 +- src/cmd/compile/internal/gc/main.go | 45 +- src/cmd/compile/internal/ir/stmt.go | 9 + src/cmd/compile/internal/loopvar/loopvar.go | 449 ++++++++++++++++++ .../compile/internal/loopvar/loopvar_test.go | 152 ++++++ .../testdata/for_complicated_esc_address.go | 115 +++++ .../loopvar/testdata/for_esc_address.go | 45 ++ .../loopvar/testdata/for_esc_closure.go | 51 ++ .../loopvar/testdata/for_esc_method.go | 51 ++ .../testdata/for_esc_minimal_closure.go | 48 ++ .../internal/loopvar/testdata/for_nested.go | 47 ++ .../internal/loopvar/testdata/inlines/a/a.go | 20 + .../internal/loopvar/testdata/inlines/b/b.go | 21 + .../internal/loopvar/testdata/inlines/c/c.go | 14 + .../internal/loopvar/testdata/inlines/main.go | 53 +++ .../loopvar/testdata/range_esc_address.go | 47 ++ .../loopvar/testdata/range_esc_closure.go | 53 +++ .../loopvar/testdata/range_esc_method.go | 53 +++ .../testdata/range_esc_minimal_closure.go | 50 ++ src/cmd/compile/internal/noder/reader.go | 2 +- src/internal/goexperiment/exp_loopvar_off.go | 9 + src/internal/goexperiment/exp_loopvar_on.go | 9 + src/internal/goexperiment/flags.go | 4 + src/runtime/race/testdata/mop_test.go | 3 +- test/closure2.go | 3 +- test/escape.go | 6 +- test/fixedbugs/issue13799.go | 5 +- 30 files changed, 1462 insertions(+), 20 deletions(-) create mode 100644 src/cmd/compile/internal/loopvar/loopvar.go create mode 100644 src/cmd/compile/internal/loopvar/loopvar_test.go create mode 100644 src/cmd/compile/internal/loopvar/testdata/for_complicated_esc_address.go create mode 100644 src/cmd/compile/internal/loopvar/testdata/for_esc_address.go create mode 100644 src/cmd/compile/internal/loopvar/testdata/for_esc_closure.go create mode 100644 src/cmd/compile/internal/loopvar/testdata/for_esc_method.go create mode 100644 src/cmd/compile/internal/loopvar/testdata/for_esc_minimal_closure.go create mode 100644 src/cmd/compile/internal/loopvar/testdata/for_nested.go create mode 100644 src/cmd/compile/internal/loopvar/testdata/inlines/a/a.go create mode 100644 src/cmd/compile/internal/loopvar/testdata/inlines/b/b.go create mode 100644 src/cmd/compile/internal/loopvar/testdata/inlines/c/c.go create mode 100644 src/cmd/compile/internal/loopvar/testdata/inlines/main.go create mode 100644 src/cmd/compile/internal/loopvar/testdata/range_esc_address.go create mode 100644 src/cmd/compile/internal/loopvar/testdata/range_esc_closure.go create mode 100644 src/cmd/compile/internal/loopvar/testdata/range_esc_method.go create mode 100644 src/cmd/compile/internal/loopvar/testdata/range_esc_minimal_closure.go create mode 100644 src/internal/goexperiment/exp_loopvar_off.go create mode 100644 src/internal/goexperiment/exp_loopvar_on.go diff --git a/src/cmd/compile/internal/base/debug.go b/src/cmd/compile/internal/base/debug.go index 04b8469eef..288c2d82ef 100644 --- a/src/cmd/compile/internal/base/debug.go +++ b/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"` diff --git a/src/cmd/compile/internal/base/flag.go b/src/cmd/compile/internal/base/flag.go index a833364c66..4baff74917 100644 --- a/src/cmd/compile/internal/base/flag.go +++ b/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 != "" { diff --git a/src/cmd/compile/internal/base/hashdebug.go b/src/cmd/compile/internal/base/hashdebug.go index 6c4821bbf6..a13c66646b 100644 --- a/src/cmd/compile/internal/base/hashdebug.go +++ b/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,6 +258,8 @@ 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 @@ -242,6 +267,11 @@ func (d *HashDebug) DebugHashMatchPos(ctxt *obj.Link, pos src.XPos) bool { 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(';') } diff --git a/src/cmd/compile/internal/escape/stmt.go b/src/cmd/compile/internal/escape/stmt.go index 1ce04d98f3..98cd2d53a6 100644 --- a/src/cmd/compile/internal/escape/stmt.go +++ b/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) diff --git a/src/cmd/compile/internal/gc/main.go b/src/cmd/compile/internal/gc/main.go index 3c2b9c48ec..fa9f429a1e 100644 --- a/src/cmd/compile/internal/gc/main.go +++ b/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 diff --git a/src/cmd/compile/internal/ir/stmt.go b/src/cmd/compile/internal/ir/stmt.go index 9f2d04f450..dd3908e665 100644 --- a/src/cmd/compile/internal/ir/stmt.go +++ b/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. diff --git a/src/cmd/compile/internal/loopvar/loopvar.go b/src/cmd/compile/internal/loopvar/loopvar.go new file mode 100644 index 0000000000..bc288657ab --- /dev/null +++ b/src/cmd/compile/internal/loopvar/loopvar.go @@ -0,0 +1,449 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// Package loopvar applies the proper variable capture, according +// to experiment, flags, language version, etc. +package loopvar + +import ( + "cmd/compile/internal/base" + "cmd/compile/internal/ir" + "cmd/compile/internal/typecheck" + "cmd/compile/internal/types" + "fmt" +) + +// ForCapture transforms for and range loops that declare variables that might be +// captured by a closure or escaped to the heap, using a syntactic check that +// conservatively overestimates the loops where capture occurs, but still avoids +// transforming the (large) majority of loops. It returns the list of names +// subject to this change, that may (once transformed) be heap allocated in the +// process. (This allows checking after escape analysis to call out any such +// variables, in case it causes allocation/performance problems). + +// For this code, the meaningful debug and hash flag settings +// +// base.Debug.LoopVar <= 0 => do not transform +// +// base.LoopVarHash != nil => use hash setting to govern transformation. +// note that LoopVarHash != nil sets base.Debug.LoopVar to 1 (unless it is >= 11, for testing/debugging). +// +// base.Debug.LoopVar == 11 => transform ALL loops ignoring syntactic/potential escape. Do not log, can be in addition to GOEXPERIMENT. +// +// The effect of GOEXPERIMENT=loopvar is to change the default value (0) of base.Debug.LoopVar to 1 for all packages. + +func ForCapture(fn *ir.Func) []*ir.Name { + if base.Debug.LoopVar <= 0 { // code in base:flags.go ensures >= 1 if loopvarhash != "" + // TODO remove this when the transformation is made sensitive to inlining; this is least-risk for 1.21 + return nil + } + + // if a loop variable is transformed it is appended to this slice for later logging + var transformed []*ir.Name + + forCapture := func() { + seq := 1 + + dclFixups := make(map[*ir.Name]ir.Stmt) + + // possibly leaked includes names of declared loop variables that may be leaked; + // the mapped value is true if the name is *syntactically* leaked, and those loops + // will be transformed. + possiblyLeaked := make(map[*ir.Name]bool) + + // noteMayLeak is called for candidate variables in for range/3-clause, and + // adds them (mapped to false) to possiblyLeaked. + noteMayLeak := func(x ir.Node) { + if n, ok := x.(*ir.Name); ok { + if n.Type().Kind() == types.TBLANK { + return + } + // default is false (leak candidate, not yet known to leak), but flag can make all variables "leak" + possiblyLeaked[n] = base.Debug.LoopVar >= 11 + } + } + + // maybeReplaceVar unshares an iteration variable for a range loop, + // if that variable was actually (syntactically) leaked, + // subject to hash-variable debugging. + maybeReplaceVar := func(k ir.Node, x *ir.RangeStmt) ir.Node { + if n, ok := k.(*ir.Name); ok && possiblyLeaked[n] { + if base.LoopVarHash.DebugHashMatchPos(base.Ctxt, n.Pos()) { + // Rename the loop key, prefix body with assignment from loop key + transformed = append(transformed, n) + tk := typecheck.Temp(n.Type()) + tk.SetTypecheck(1) + as := ir.NewAssignStmt(x.Pos(), n, tk) + as.Def = true + as.SetTypecheck(1) + x.Body.Prepend(as) + dclFixups[n] = as + return tk + } + } + return k + } + + // scanChildrenThenTransform processes node x to: + // 1. if x is a for/range, note declared iteration variables possiblyLeaked (PL) + // 2. search all of x's children for syntactically escaping references to v in PL, + // meaning either address-of-v or v-captured-by-a-closure + // 3. for all v in PL that had a syntactically escaping reference, transform the declaration + // and (in case of 3-clause loop) the loop to the unshared loop semantics. + // This is all much simpler for range loops; 3-clause loops can have an arbitrary number + // of iteration variables and the transformation is more involved, range loops have at most 2. + var scanChildrenThenTransform func(x ir.Node) bool + scanChildrenThenTransform = func(n ir.Node) bool { + switch x := n.(type) { + case *ir.ClosureExpr: + for _, cv := range x.Func.ClosureVars { + v := cv.Canonical() + if _, ok := possiblyLeaked[v]; ok { + possiblyLeaked[v] = true + } + } + + case *ir.AddrExpr: + // Explicitly note address-taken so that return-statements can be excluded + y := ir.OuterValue(x.X) + if y.Op() != ir.ONAME { + break + } + z, ok := y.(*ir.Name) + if !ok { + break + } + switch z.Class { + case ir.PAUTO, ir.PPARAM, ir.PPARAMOUT, ir.PAUTOHEAP: + if _, ok := possiblyLeaked[z]; ok { + possiblyLeaked[z] = true + } + } + + case *ir.RangeStmt: + if !x.Def { + break + } + noteMayLeak(x.Key) + noteMayLeak(x.Value) + ir.DoChildren(n, scanChildrenThenTransform) + x.Key = maybeReplaceVar(x.Key, x) + x.Value = maybeReplaceVar(x.Value, x) + return false + + case *ir.ForStmt: + forAllDefInInit(x, noteMayLeak) + ir.DoChildren(n, scanChildrenThenTransform) + var leaked []*ir.Name + // Collect the leaking variables for the much-more-complex transformation. + forAllDefInInit(x, func(z ir.Node) { + if n, ok := z.(*ir.Name); ok && possiblyLeaked[n] { + // Hash on n.Pos() for most precise failure location. + if base.LoopVarHash.DebugHashMatchPos(base.Ctxt, n.Pos()) { + leaked = append(leaked, n) + } + } + }) + + if len(leaked) > 0 { + // need to transform the for loop just so. + + /* Contrived example, w/ numbered comments from the transformation: + BEFORE: + var escape []*int + for z := 0; z < n; z++ { + if reason() { + escape = append(escape, &z) + continue + } + z = z + z + stuff + } + AFTER: + for z', tmp_first := 0, true; ; { // (4) + // (5) body' follows: + z := z' // (1) + if tmp_first {tmp_first = false} else {z++} // (6) + if ! (z < n) { break } // (7) + // (3, 8) body_continue + if reason() { + escape = append(escape, &z) + goto next // rewritten continue + } + z = z + z + stuff + next: // (9) + z' = z // (2) + } + + In the case that the loop contains no increment (z++), + there is no need for step 6, + and thus no need to test, update, or declare tmp_first (part of step 4). + Similarly if the loop contains no exit test (z < n), + then there is no need for step 7. + */ + + // Expressed in terms of the input ForStmt + // + // type ForStmt struct { + // init Nodes + // Label *types.Sym + // Cond Node // empty if OFORUNTIL + // Post Node + // Body Nodes + // HasBreak bool + // } + + // OFOR: init; loop: if !Cond {break}; Body; Post; goto loop + + // (1) prebody = {z := z' for z in leaked} + // (2) postbody = {z' = z for z in leaked} + // (3) body_continue = {body : s/continue/goto next} + // (4) init' = (init : s/z/z' for z in leaked) + tmp_first := true + // (5) body' = prebody + // appears out of order below + // (6) if tmp_first {tmp_first = false} else {Post} + + // (7) if !cond {break} + + // (8) body_continue (3) + + // (9) next: postbody (2) + // (10) cond' = {} + // (11) post' = {} + + // minor optimizations: + // if Post is empty, tmp_first and step 6 can be skipped. + // if Cond is empty, that code can also be skipped. + + var preBody, postBody ir.Nodes + + // Given original iteration variable z, what is the corresponding z' + // that carries the value from iteration to iteration? + zPrimeForZ := make(map[*ir.Name]*ir.Name) + + // (1,2) initialize preBody and postBody + for _, z := range leaked { + transformed = append(transformed, z) + + tz := typecheck.Temp(z.Type()) + tz.SetTypecheck(1) + zPrimeForZ[z] = tz + + as := ir.NewAssignStmt(x.Pos(), z, tz) + as.Def = true + as.SetTypecheck(1) + preBody.Append(as) + dclFixups[z] = as + + as = ir.NewAssignStmt(x.Pos(), tz, z) + as.SetTypecheck(1) + postBody.Append(as) + + } + + // (3) rewrite continues in body -- rewrite is inplace, so works for top level visit, too. + label := typecheck.Lookup(fmt.Sprintf(".3clNext_%d", seq)) + seq++ + labelStmt := ir.NewLabelStmt(x.Pos(), label) + labelStmt.SetTypecheck(1) + + loopLabel := x.Label + loopDepth := 0 + var editContinues func(x ir.Node) bool + editContinues = func(x ir.Node) bool { + + switch c := x.(type) { + case *ir.BranchStmt: + // If this is a continue targeting the loop currently being rewritten, transform it to an appropriate GOTO + if c.Op() == ir.OCONTINUE && (loopDepth == 0 && c.Label == nil || loopLabel != nil && c.Label == loopLabel) { + c.Label = label + c.SetOp(ir.OGOTO) + } + case *ir.RangeStmt, *ir.ForStmt: + loopDepth++ + ir.DoChildren(x, editContinues) + loopDepth-- + return false + } + ir.DoChildren(x, editContinues) + return false + } + for _, y := range x.Body { + editContinues(y) + } + bodyContinue := x.Body + + // (4) rewrite init + forAllDefInInitUpdate(x, func(z ir.Node, pz *ir.Node) { + // note tempFor[n] can be nil if hash searching. + if n, ok := z.(*ir.Name); ok && possiblyLeaked[n] && zPrimeForZ[n] != nil { + *pz = zPrimeForZ[n] + } + }) + + postNotNil := x.Post != nil + var tmpFirstDcl *ir.AssignStmt + if postNotNil { + // body' = prebody + + // (6) if tmp_first {tmp_first = false} else {Post} + + // if !cond {break} + ... + tmpFirst := typecheck.Temp(types.Types[types.TBOOL]) + + // tmpFirstAssign assigns val to tmpFirst + tmpFirstAssign := func(val bool) *ir.AssignStmt { + s := ir.NewAssignStmt(x.Pos(), tmpFirst, typecheck.OrigBool(tmpFirst, val)) + s.SetTypecheck(1) + return s + } + + tmpFirstDcl = tmpFirstAssign(true) + tmpFirstDcl.Def = true // also declares tmpFirst + tmpFirstSetFalse := tmpFirstAssign(false) + ifTmpFirst := ir.NewIfStmt(x.Pos(), tmpFirst, ir.Nodes{tmpFirstSetFalse}, ir.Nodes{x.Post}) + ifTmpFirst.SetTypecheck(1) + preBody.Append(ifTmpFirst) + } + + // body' = prebody + + // if tmp_first {tmp_first = false} else {Post} + + // (7) if !cond {break} + ... + if x.Cond != nil { + notCond := ir.NewUnaryExpr(x.Cond.Pos(), ir.ONOT, x.Cond) + notCond.SetType(x.Cond.Type()) + notCond.SetTypecheck(1) + newBreak := ir.NewBranchStmt(x.Pos(), ir.OBREAK, nil) + newBreak.SetTypecheck(1) + ifNotCond := ir.NewIfStmt(x.Pos(), notCond, ir.Nodes{newBreak}, nil) + ifNotCond.SetTypecheck(1) + preBody.Append(ifNotCond) + } + + if postNotNil { + x.PtrInit().Append(tmpFirstDcl) + } + + // (8) + preBody.Append(bodyContinue...) + // (9) + preBody.Append(labelStmt) + preBody.Append(postBody...) + + // (5) body' = prebody + ... + x.Body = preBody + + // (10) cond' = {} + x.Cond = nil + + // (11) post' = {} + x.Post = nil + } + + return false + } + + ir.DoChildren(n, scanChildrenThenTransform) + + return false + } + scanChildrenThenTransform(fn) + if len(transformed) > 0 { + // editNodes scans a slice C of ir.Node, looking for declarations that + // appear in dclFixups. Any declaration D whose "fixup" is an assignmnt + // statement A is removed from the C and relocated to the Init + // of A. editNodes returns the modified slice of ir.Node. + editNodes := func(c ir.Nodes) ir.Nodes { + j := 0 + for _, n := range c { + if d, ok := n.(*ir.Decl); ok { + if s := dclFixups[d.X]; s != nil { + switch a := s.(type) { + case *ir.AssignStmt: + a.PtrInit().Prepend(d) + delete(dclFixups, d.X) // can't be sure of visit order, wouldn't want to visit twice. + default: + base.Fatalf("not implemented yet for node type %v", s.Op()) + } + continue // do not copy this node, and do not increment j + } + } + c[j] = n + j++ + } + for k := j; k < len(c); k++ { + c[k] = nil + } + return c[:j] + } + // fixup all tagged declarations in all the statements lists in fn. + rewriteNodes(fn, editNodes) + } + } + ir.WithFunc(fn, forCapture) + return transformed +} + +// forAllDefInInitUpdate applies "do" to all the defining assignemnts in the Init clause of a ForStmt. +// This abstracts away some of the boilerplate from the already complex and verbose for-3-clause case. +func forAllDefInInitUpdate(x *ir.ForStmt, do func(z ir.Node, update *ir.Node)) { + for _, s := range x.Init() { + switch y := s.(type) { + case *ir.AssignListStmt: + if !y.Def { + continue + } + for i, z := range y.Lhs { + do(z, &y.Lhs[i]) + } + case *ir.AssignStmt: + if !y.Def { + continue + } + do(y.X, &y.X) + } + } +} + +// forAllDefInInit is forAllDefInInitUpdate without the update option. +func forAllDefInInit(x *ir.ForStmt, do func(z ir.Node)) { + forAllDefInInitUpdate(x, func(z ir.Node, _ *ir.Node) { do(z) }) +} + +// rewriteNodes applies editNodes to all statement lists in fn. +func rewriteNodes(fn *ir.Func, editNodes func(c ir.Nodes) ir.Nodes) { + var forNodes func(x ir.Node) bool + forNodes = func(n ir.Node) bool { + if stmt, ok := n.(ir.InitNode); ok { + // process init list + stmt.SetInit(editNodes(stmt.Init())) + } + switch x := n.(type) { + case *ir.Func: + x.Body = editNodes(x.Body) + x.Enter = editNodes(x.Enter) + x.Exit = editNodes(x.Exit) + case *ir.InlinedCallExpr: + x.Body = editNodes(x.Body) + + case *ir.CaseClause: + x.Body = editNodes(x.Body) + case *ir.CommClause: + x.Body = editNodes(x.Body) + + case *ir.BlockStmt: + x.List = editNodes(x.List) + + case *ir.ForStmt: + x.Body = editNodes(x.Body) + case *ir.RangeStmt: + x.Body = editNodes(x.Body) + case *ir.IfStmt: + x.Body = editNodes(x.Body) + x.Else = editNodes(x.Else) + case *ir.SelectStmt: + x.Compiled = editNodes(x.Compiled) + case *ir.SwitchStmt: + x.Compiled = editNodes(x.Compiled) + } + ir.DoChildren(n, forNodes) + return false + } + forNodes(fn) +} diff --git a/src/cmd/compile/internal/loopvar/loopvar_test.go b/src/cmd/compile/internal/loopvar/loopvar_test.go new file mode 100644 index 0000000000..6ff4fc9e22 --- /dev/null +++ b/src/cmd/compile/internal/loopvar/loopvar_test.go @@ -0,0 +1,152 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package loopvar_test + +import ( + "internal/testenv" + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" +) + +type testcase struct { + lvFlag string // ==-2, -1, 0, 1, 2 + buildExpect string // message, if any + expectRC int + files []string +} + +var for_files = []string{ + "for_esc_address.go", // address of variable + "for_esc_closure.go", // closure of variable + "for_esc_minimal_closure.go", // simple closure of variable + "for_esc_method.go", // method value of variable + "for_complicated_esc_address.go", // modifies loop index in body +} + +var range_files = []string{ + "range_esc_address.go", // address of variable + "range_esc_closure.go", // closure of variable + "range_esc_minimal_closure.go", // simple closure of variable + "range_esc_method.go", // method value of variable +} + +var cases = []testcase{ + {"-1", "", 11, for_files[:1]}, + {"0", "", 0, for_files[:1]}, + {"1", "", 0, for_files[:1]}, + {"2", "transformed loop variable i ", 0, for_files}, + + {"-1", "", 11, range_files[:1]}, + {"0", "", 0, range_files[:1]}, + {"1", "", 0, range_files[:1]}, + {"2", "transformed loop variable i ", 0, range_files}, + + {"1", "", 0, []string{"for_nested.go"}}, +} + +// TestLoopVar checks that the GOEXPERIMENT and debug flags behave as expected. +func TestLoopVar(t *testing.T) { + switch runtime.GOOS { + case "linux", "darwin": + default: + t.Skipf("Slow test, usually avoid it, os=%s not linux or darwin", runtime.GOOS) + } + switch runtime.GOARCH { + case "amd64", "arm64": + default: + t.Skipf("Slow test, usually avoid it, arch=%s not amd64 or arm64", runtime.GOARCH) + } + + testenv.MustHaveGoBuild(t) + gocmd := testenv.GoToolPath(t) + tmpdir := t.TempDir() + output := filepath.Join(tmpdir, "foo.exe") + + for i, tc := range cases { + for _, f := range tc.files { + source := f + cmd := testenv.Command(t, gocmd, "build", "-o", output, "-gcflags=-d=loopvar="+tc.lvFlag, source) + cmd.Env = append(cmd.Env, "GOEXPERIMENT=loopvar", "HOME="+tmpdir) + cmd.Dir = "testdata" + t.Logf("File %s loopvar=%s expect '%s' exit code %d", f, tc.lvFlag, tc.buildExpect, tc.expectRC) + b, e := cmd.CombinedOutput() + if e != nil { + t.Error(e) + } + if tc.buildExpect != "" { + s := string(b) + if !strings.Contains(s, tc.buildExpect) { + t.Errorf("File %s test %d expected to match '%s' with \n-----\n%s\n-----", f, i, tc.buildExpect, s) + } + } + // run what we just built. + cmd = testenv.Command(t, output) + b, e = cmd.CombinedOutput() + if tc.expectRC != 0 { + if e == nil { + t.Errorf("Missing expected error, file %s, case %d", f, i) + } else if ee, ok := (e).(*exec.ExitError); !ok || ee.ExitCode() != tc.expectRC { + t.Error(e) + } else { + // okay + } + } else if e != nil { + t.Error(e) + } + } + } +} + +func TestLoopVarHashes(t *testing.T) { + switch runtime.GOOS { + case "linux", "darwin": + default: + t.Skipf("Slow test, usually avoid it, os=%s not linux or darwin", runtime.GOOS) + } + switch runtime.GOARCH { + case "amd64", "arm64": + default: + t.Skipf("Slow test, usually avoid it, arch=%s not amd64 or arm64", runtime.GOARCH) + } + + testenv.MustHaveGoBuild(t) + gocmd := testenv.GoToolPath(t) + tmpdir := t.TempDir() + + root := "cmd/compile/internal/loopvar/testdata/inlines" + + f := func(hash string) string { + // This disables the loopvar change, except for the specified package. + // The effect should follow the package, even though everything (except "c") + // is inlined. + cmd := testenv.Command(t, gocmd, "run", root) + cmd.Env = append(cmd.Env, "GOCOMPILEDEBUG=loopvarhash=FS"+hash, "HOME="+tmpdir) + cmd.Dir = filepath.Join("testdata", "inlines") + + b, _ := cmd.CombinedOutput() + // Ignore the error, sometimes it's supposed to fail, the output test will catch it. + return string(b) + } + + m := f("000100000010011111101100") + t.Logf(m) + + mCount := strings.Count(m, "loopvarhash triggered POS=main.go:27:6") + otherCount := strings.Count(m, "loopvarhash") + if mCount < 1 { + t.Errorf("Did not see expected value of m compile") + } + if mCount != otherCount { + t.Errorf("Saw extraneous hash matches") + } + // This next test carefully dodges a bug-to-be-fixed with inlined locations for ir.Names. + if !strings.Contains(m, ", 100, 100, 100, 100") { + t.Errorf("Did not see expected value of m run") + } + +} diff --git a/src/cmd/compile/internal/loopvar/testdata/for_complicated_esc_address.go b/src/cmd/compile/internal/loopvar/testdata/for_complicated_esc_address.go new file mode 100644 index 0000000000..c658340fd5 --- /dev/null +++ b/src/cmd/compile/internal/loopvar/testdata/for_complicated_esc_address.go @@ -0,0 +1,115 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package main + +import ( + "fmt" + "os" +) + +func main() { + ss, sa := shared(23) + ps, pa := private(23) + es, ea := experiment(23) + + fmt.Printf("shared s, a; private, s, a; experiment s, a = %d, %d; %d, %d; %d, %d\n", ss, sa, ps, pa, es, ea) + + if ss != ps || ss != es || ea != pa || sa == pa { + os.Exit(11) + } else { + fmt.Println("PASS") + } +} + +func experiment(x int) (int, int) { + sum := 0 + var is []*int + for i := x; i != 1; i = i / 2 { + for j := 0; j < 10; j++ { + if i == j { // 10 skips + continue + } + sum++ + } + i = i*3 + 1 + if i&1 == 0 { + is = append(is, &i) + for i&2 == 0 { + i = i >> 1 + } + } else { + i = i + i + } + } + + asum := 0 + for _, pi := range is { + asum += *pi + } + + return sum, asum +} + +func private(x int) (int, int) { + sum := 0 + var is []*int + I := x + for ; I != 1; I = I / 2 { + i := I + for j := 0; j < 10; j++ { + if i == j { // 10 skips + I = i + continue + } + sum++ + } + i = i*3 + 1 + if i&1 == 0 { + is = append(is, &i) + for i&2 == 0 { + i = i >> 1 + } + } else { + i = i + i + } + I = i + } + + asum := 0 + for _, pi := range is { + asum += *pi + } + + return sum, asum +} + +func shared(x int) (int, int) { + sum := 0 + var is []*int + i := x + for ; i != 1; i = i / 2 { + for j := 0; j < 10; j++ { + if i == j { // 10 skips + continue + } + sum++ + } + i = i*3 + 1 + if i&1 == 0 { + is = append(is, &i) + for i&2 == 0 { + i = i >> 1 + } + } else { + i = i + i + } + } + + asum := 0 + for _, pi := range is { + asum += *pi + } + return sum, asum +} diff --git a/src/cmd/compile/internal/loopvar/testdata/for_esc_address.go b/src/cmd/compile/internal/loopvar/testdata/for_esc_address.go new file mode 100644 index 0000000000..beaefb10ab --- /dev/null +++ b/src/cmd/compile/internal/loopvar/testdata/for_esc_address.go @@ -0,0 +1,45 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package main + +import ( + "fmt" + "os" +) + +func main() { + sum := 0 + var is []*int + for i := 0; i < 10; i++ { + for j := 0; j < 10; j++ { + if i == j { // 10 skips + continue + } + sum++ + } + if i&1 == 0 { + is = append(is, &i) + } + } + + bug := false + if sum != 100-10 { + fmt.Printf("wrong sum, expected %d, saw %d\n", 90, sum) + bug = true + } + sum = 0 + for _, pi := range is { + sum += *pi + } + if sum != 2+4+6+8 { + fmt.Printf("wrong sum, expected %d, saw %d\n", 20, sum) + bug = true + } + if !bug { + fmt.Printf("PASS\n") + } else { + os.Exit(11) + } +} diff --git a/src/cmd/compile/internal/loopvar/testdata/for_esc_closure.go b/src/cmd/compile/internal/loopvar/testdata/for_esc_closure.go new file mode 100644 index 0000000000..b60d0007bd --- /dev/null +++ b/src/cmd/compile/internal/loopvar/testdata/for_esc_closure.go @@ -0,0 +1,51 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package main + +import ( + "fmt" + "os" +) + +var is []func() int + +func main() { + sum := 0 + for i := 0; i < 10; i++ { + for j := 0; j < 10; j++ { + if i == j { // 10 skips + continue + } + sum++ + } + if i&1 == 0 { + is = append(is, func() int { + if i%17 == 15 { + i++ + } + return i + }) + } + } + + bug := false + if sum != 100-10 { + fmt.Printf("wrong sum, expected %d, saw %d\n", 90, sum) + bug = true + } + sum = 0 + for _, f := range is { + sum += f() + } + if sum != 2+4+6+8 { + fmt.Printf("wrong sum, expected %d, saw %d\n", 20, sum) + bug = true + } + if !bug { + fmt.Printf("PASS\n") + } else { + os.Exit(11) + } +} diff --git a/src/cmd/compile/internal/loopvar/testdata/for_esc_method.go b/src/cmd/compile/internal/loopvar/testdata/for_esc_method.go new file mode 100644 index 0000000000..0e2f8017bc --- /dev/null +++ b/src/cmd/compile/internal/loopvar/testdata/for_esc_method.go @@ -0,0 +1,51 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package main + +import ( + "fmt" + "os" +) + +type I int + +func (x *I) method() int { + return int(*x) +} + +func main() { + sum := 0 + var is []func() int + for i := I(0); int(i) < 10; i++ { + for j := 0; j < 10; j++ { + if int(i) == j { // 10 skips + continue + } + sum++ + } + if i&1 == 0 { + is = append(is, i.method) + } + } + + bug := false + if sum != 100-10 { + fmt.Printf("wrong sum, expected %d, saw %d\n", 90, sum) + bug = true + } + sum = 0 + for _, m := range is { + sum += m() + } + if sum != 2+4+6+8 { + fmt.Printf("wrong sum, expected %d, saw %d\n", 20, sum) + bug = true + } + if !bug { + fmt.Printf("PASS\n") + } else { + os.Exit(11) + } +} diff --git a/src/cmd/compile/internal/loopvar/testdata/for_esc_minimal_closure.go b/src/cmd/compile/internal/loopvar/testdata/for_esc_minimal_closure.go new file mode 100644 index 0000000000..971c91dde1 --- /dev/null +++ b/src/cmd/compile/internal/loopvar/testdata/for_esc_minimal_closure.go @@ -0,0 +1,48 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package main + +import ( + "fmt" + "os" +) + +var is []func() int + +func main() { + sum := 0 + for i := 0; i < 10; i++ { + for j := 0; j < 10; j++ { + if i == j { // 10 skips + continue + } + sum++ + } + if i&1 == 0 { + is = append(is, func() int { + return i + }) + } + } + + bug := false + if sum != 100-10 { + fmt.Printf("wrong sum, expected %d, saw %d\n", 90, sum) + bug = true + } + sum = 0 + for _, f := range is { + sum += f() + } + if sum != 2+4+6+8 { + fmt.Printf("wrong sum, expected %d, saw %d\n", 20, sum) + bug = true + } + if !bug { + fmt.Printf("PASS\n") + } else { + os.Exit(11) + } +} diff --git a/src/cmd/compile/internal/loopvar/testdata/for_nested.go b/src/cmd/compile/internal/loopvar/testdata/for_nested.go new file mode 100644 index 0000000000..4888fabc45 --- /dev/null +++ b/src/cmd/compile/internal/loopvar/testdata/for_nested.go @@ -0,0 +1,47 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package main + +import ( + "fmt" + "os" +) + +func main() { + x := f(60) + fmt.Println(x) + if x != 54 { + os.Exit(11) + } +} + +var escape *int + +func f(i int) int { + a := 0 +outer: + for { + switch { + case i > 55: + i-- + continue + case i == 55: + for j := i; j != 1; j = j / 2 { + a++ + if j == 4 { + escape = &j + i-- + continue outer + } + if j&1 == 1 { + j = 2 * (3*j + 1) + } + } + return a + case i < 55: + return i + } + } +} diff --git a/src/cmd/compile/internal/loopvar/testdata/inlines/a/a.go b/src/cmd/compile/internal/loopvar/testdata/inlines/a/a.go new file mode 100644 index 0000000000..0bae36daff --- /dev/null +++ b/src/cmd/compile/internal/loopvar/testdata/inlines/a/a.go @@ -0,0 +1,20 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package a + +import "cmd/compile/internal/loopvar/testdata/inlines/b" + +func F() []*int { + var s []*int + for i := 0; i < 10; i++ { + s = append(s, &i) + } + return s +} + +func Fb() []*int { + bf, _ := b.F() + return bf +} diff --git a/src/cmd/compile/internal/loopvar/testdata/inlines/b/b.go b/src/cmd/compile/internal/loopvar/testdata/inlines/b/b.go new file mode 100644 index 0000000000..7b1d8cede1 --- /dev/null +++ b/src/cmd/compile/internal/loopvar/testdata/inlines/b/b.go @@ -0,0 +1,21 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package b + +var slice = []int{1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024} + +func F() ([]*int, []*int) { + return g() +} + +func g() ([]*int, []*int) { + var s []*int + var t []*int + for i, j := range slice { + s = append(s, &i) + t = append(t, &j) + } + return s[:len(s)-1], t +} diff --git a/src/cmd/compile/internal/loopvar/testdata/inlines/c/c.go b/src/cmd/compile/internal/loopvar/testdata/inlines/c/c.go new file mode 100644 index 0000000000..0405ace9fe --- /dev/null +++ b/src/cmd/compile/internal/loopvar/testdata/inlines/c/c.go @@ -0,0 +1,14 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package c + +//go:noinline +func F() []*int { + var s []*int + for i := 0; i < 10; i++ { + s = append(s, &i) + } + return s +} diff --git a/src/cmd/compile/internal/loopvar/testdata/inlines/main.go b/src/cmd/compile/internal/loopvar/testdata/inlines/main.go new file mode 100644 index 0000000000..46fcee1a6d --- /dev/null +++ b/src/cmd/compile/internal/loopvar/testdata/inlines/main.go @@ -0,0 +1,53 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package main + +import ( + "cmd/compile/internal/loopvar/testdata/inlines/a" + "cmd/compile/internal/loopvar/testdata/inlines/b" + "cmd/compile/internal/loopvar/testdata/inlines/c" + "fmt" + "os" +) + +func sum(s []*int) int { + sum := 0 + for _, pi := range s { + sum += *pi + } + return sum +} + +var t []*int + +func F() []*int { + var s []*int + for i, j := 0, 0; j < 10; i, j = i+1, j+1 { + s = append(s, &i) + t = append(s, &j) + } + return s +} + +func main() { + f := F() + af := a.F() + bf, _ := b.F() + abf := a.Fb() + cf := c.F() + + sf, saf, sbf, sabf, scf := sum(f), sum(af), sum(bf), sum(abf), sum(cf) + + fmt.Printf("f, af, bf, abf, cf sums = %d, %d, %d, %d, %d\n", sf, saf, sbf, sabf, scf) + + // Special failure just for use with hash searching, to prove it fires exactly once. + // To test: `gossahash -e loopvarhash go run .` in this directory. + // This is designed to fail in two different ways, because gossahash searches randomly + // it will find both failures over time. + if os.Getenv("GOCOMPILEDEBUG") != "" && (sabf == 45 || sf == 45) { + os.Exit(11) + } + os.Exit(0) +} diff --git a/src/cmd/compile/internal/loopvar/testdata/range_esc_address.go b/src/cmd/compile/internal/loopvar/testdata/range_esc_address.go new file mode 100644 index 0000000000..79d7f04a0c --- /dev/null +++ b/src/cmd/compile/internal/loopvar/testdata/range_esc_address.go @@ -0,0 +1,47 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package main + +import ( + "fmt" + "os" +) + +var ints = []int{0, 1, 2, 3, 4, 5, 6, 7, 8, 9} + +func main() { + sum := 0 + var is []*int + for _, i := range ints { + for j := 0; j < 10; j++ { + if i == j { // 10 skips + continue + } + sum++ + } + if i&1 == 0 { + is = append(is, &i) + } + } + + bug := false + if sum != 100-10 { + fmt.Printf("wrong sum, expected %d, saw %d\n", 90, sum) + bug = true + } + sum = 0 + for _, pi := range is { + sum += *pi + } + if sum != 2+4+6+8 { + fmt.Printf("wrong sum, expected %d, saw %d\n", 20, sum) + bug = true + } + if !bug { + fmt.Printf("PASS\n") + } else { + os.Exit(11) + } +} diff --git a/src/cmd/compile/internal/loopvar/testdata/range_esc_closure.go b/src/cmd/compile/internal/loopvar/testdata/range_esc_closure.go new file mode 100644 index 0000000000..9bcb5efb09 --- /dev/null +++ b/src/cmd/compile/internal/loopvar/testdata/range_esc_closure.go @@ -0,0 +1,53 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package main + +import ( + "fmt" + "os" +) + +var is []func() int + +var ints = []int{0, 1, 2, 3, 4, 5, 6, 7, 8, 9} + +func main() { + sum := 0 + for _, i := range ints { + for j := 0; j < 10; j++ { + if i == j { // 10 skips + continue + } + sum++ + } + if i&1 == 0 { + is = append(is, func() int { + if i%17 == 15 { + i++ + } + return i + }) + } + } + + bug := false + if sum != 100-10 { + fmt.Printf("wrong sum, expected %d, saw %d\n", 90, sum) + bug = true + } + sum = 0 + for _, f := range is { + sum += f() + } + if sum != 2+4+6+8 { + fmt.Printf("wrong sum, expected %d, saw %d\n", 20, sum) + bug = true + } + if !bug { + fmt.Printf("PASS\n") + } else { + os.Exit(11) + } +} diff --git a/src/cmd/compile/internal/loopvar/testdata/range_esc_method.go b/src/cmd/compile/internal/loopvar/testdata/range_esc_method.go new file mode 100644 index 0000000000..9a85ab02d3 --- /dev/null +++ b/src/cmd/compile/internal/loopvar/testdata/range_esc_method.go @@ -0,0 +1,53 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package main + +import ( + "fmt" + "os" +) + +type I int + +func (x *I) method() int { + return int(*x) +} + +var ints = []I{0, 1, 2, 3, 4, 5, 6, 7, 8, 9} + +func main() { + sum := 0 + var is []func() int + for _, i := range ints { + for j := 0; j < 10; j++ { + if int(i) == j { // 10 skips + continue + } + sum++ + } + if i&1 == 0 { + is = append(is, i.method) + } + } + + bug := false + if sum != 100-10 { + fmt.Printf("wrong sum, expected %d, saw %d\n", 90, sum) + bug = true + } + sum = 0 + for _, m := range is { + sum += m() + } + if sum != 2+4+6+8 { + fmt.Printf("wrong sum, expected %d, saw %d\n", 20, sum) + bug = true + } + if !bug { + fmt.Printf("PASS\n") + } else { + os.Exit(11) + } +} diff --git a/src/cmd/compile/internal/loopvar/testdata/range_esc_minimal_closure.go b/src/cmd/compile/internal/loopvar/testdata/range_esc_minimal_closure.go new file mode 100644 index 0000000000..8804d8b789 --- /dev/null +++ b/src/cmd/compile/internal/loopvar/testdata/range_esc_minimal_closure.go @@ -0,0 +1,50 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package main + +import ( + "fmt" + "os" +) + +var is []func() int + +var ints = []int{0, 1, 2, 3, 4, 5, 6, 7, 8, 9} + +func main() { + sum := 0 + for _, i := range ints { + for j := 0; j < 10; j++ { + if i == j { // 10 skips + continue + } + sum++ + } + if i&1 == 0 { + is = append(is, func() int { + return i + }) + } + } + + bug := false + if sum != 100-10 { + fmt.Printf("wrong sum, expected %d, saw %d\n", 90, sum) + bug = true + } + sum = 0 + for _, f := range is { + sum += f() + } + if sum != 2+4+6+8 { + fmt.Printf("wrong sum, expected %d, saw %d\n", 20, sum) + bug = true + } + if !bug { + fmt.Printf("PASS\n") + } else { + os.Exit(11) + } +} diff --git a/src/cmd/compile/internal/noder/reader.go b/src/cmd/compile/internal/noder/reader.go index e4ab80b2d0..64173312ac 100644 --- a/src/cmd/compile/internal/noder/reader.go +++ b/src/cmd/compile/internal/noder/reader.go @@ -2366,7 +2366,7 @@ func (r *reader) expr() (res ir.Node) { if recv.Type().IsInterface() { // N.B., this happens currently for typeparam/issue51521.go // and typeparam/typeswitch3.go. - if base.Flag.LowerM > 0 { + if base.Flag.LowerM != 0 { base.WarnfAt(method.Pos(), "imprecise interface call") } } diff --git a/src/internal/goexperiment/exp_loopvar_off.go b/src/internal/goexperiment/exp_loopvar_off.go new file mode 100644 index 0000000000..1cd7ee15d7 --- /dev/null +++ b/src/internal/goexperiment/exp_loopvar_off.go @@ -0,0 +1,9 @@ +// Code generated by mkconsts.go. DO NOT EDIT. + +//go:build !goexperiment.loopvar +// +build !goexperiment.loopvar + +package goexperiment + +const LoopVar = false +const LoopVarInt = 0 diff --git a/src/internal/goexperiment/exp_loopvar_on.go b/src/internal/goexperiment/exp_loopvar_on.go new file mode 100644 index 0000000000..e3c8980c1e --- /dev/null +++ b/src/internal/goexperiment/exp_loopvar_on.go @@ -0,0 +1,9 @@ +// Code generated by mkconsts.go. DO NOT EDIT. + +//go:build goexperiment.loopvar +// +build goexperiment.loopvar + +package goexperiment + +const LoopVar = true +const LoopVarInt = 1 diff --git a/src/internal/goexperiment/flags.go b/src/internal/goexperiment/flags.go index 07481bcd50..8758505173 100644 --- a/src/internal/goexperiment/flags.go +++ b/src/internal/goexperiment/flags.go @@ -101,4 +101,8 @@ type Flags struct { // When this experiment is enabled, cgo rule checks occur regardless // of the GODEBUG=cgocheck setting provided at runtime. CgoCheck2 bool + + // LoopVar changes loop semantics so that each iteration gets its own + // copy of the iteration variable. + LoopVar bool } diff --git a/src/runtime/race/testdata/mop_test.go b/src/runtime/race/testdata/mop_test.go index 0d79091df4..0da539fc01 100644 --- a/src/runtime/race/testdata/mop_test.go +++ b/src/runtime/race/testdata/mop_test.go @@ -331,7 +331,8 @@ func TestRaceRange(t *testing.T) { var x, y int _ = x + y done := make(chan bool, N) - for i, v := range a { + var i, v int // declare here (not in for stmt) so that i and v are shared w/ or w/o loop variable sharing change + for i, v = range a { go func(i int) { // we don't want a write-vs-write race // so there is no array b here diff --git a/test/closure2.go b/test/closure2.go index 812d41f8ce..081d2e2d3d 100644 --- a/test/closure2.go +++ b/test/closure2.go @@ -73,7 +73,8 @@ func main() { { var g func() int - for i := range [2]int{} { + var i int + for i = range [2]int{} { if i == 0 { g = func() int { return i // test that we capture by ref here, i is mutated on every interaction diff --git a/test/escape.go b/test/escape.go index 252a1e59cc..e6103f72c8 100644 --- a/test/escape.go +++ b/test/escape.go @@ -126,7 +126,8 @@ func range_escapes2(x, y int) (*int, *int) { var p [2]*int a[0] = x a[1] = y - for k, v := range a { + var k, v int + for k, v = range a { p[k] = &v } return p[0], p[1] @@ -136,7 +137,8 @@ func range_escapes2(x, y int) (*int, *int) { func for_escapes2(x int, y int) (*int, *int) { var p [2]*int n := 0 - for i := x; n < 2; i = y { + i := x + for ; n < 2; i = y { p[n] = &i n++ } diff --git a/test/fixedbugs/issue13799.go b/test/fixedbugs/issue13799.go index c8ecfc54e4..7ab4040434 100644 --- a/test/fixedbugs/issue13799.go +++ b/test/fixedbugs/issue13799.go @@ -45,8 +45,9 @@ func test1(iter int) { // Heap -> stack pointer eventually causes badness when stack reallocation // occurs. - var fn func() // ERROR "moved to heap: fn$" - for i := 0; i < maxI; i++ { // ERROR "moved to heap: i$" + var fn func() // ERROR "moved to heap: fn$" + i := 0 // ERROR "moved to heap: i$" + for ; i < maxI; i++ { // var fn func() // this makes it work, because fn stays off heap j := 0 // ERROR "moved to heap: j$" fn = func() { // ERROR "func literal escapes to heap$"