cmd/compile/internal/typecheck: normalize go/defer statements earlier

Normalizing go/defer statements to always use functions with zero
parameters and zero results was added to escape analysis, because that
was the earliest point at which all three frontends converged. Now
that we only have the unified frontend, we can do it during typecheck,
which is where we perform all other desugaring and normalization
rewrites.

Change-Id: Iebf7679b117fd78b1dffee2974bbf85ebc923b23
Reviewed-on: https://go-review.googlesource.com/c/go/+/520260
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Auto-Submit: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

8 files changed, 223 insertions, 119 deletions

diff --git a/src/cmd/compile/internal/escape/call.go b/src/cmd/compile/internal/escape/call.go
index d87dca23e1..eb87954299 100644
--- a/src/cmd/compile/internal/escape/call.go
+++ b/src/cmd/compile/internal/escape/call.go
@@ -210,17 +210,6 @@ func (e *escape) callCommon(ks []hole, call ir.Node, init *ir.Nodes, wrapper *ir
 }
 
 // goDeferStmt analyzes a "go" or "defer" statement.
-//
-// In the process, it also normalizes the statement to always use a
-// simple function call with no arguments and no results. For example,
-// it rewrites:
-//
-//	defer f(x, y)
-//
-// into:
-//
-//	x1, y1 := x, y
-//	defer func() { f(x1, y1) }()
 func (e *escape) goDeferStmt(n *ir.GoDeferStmt) {
 	k := e.heapHole()
 	if n.Op() == ir.ODEFER && e.loopDepth == 1 {
@@ -233,57 +222,26 @@ func (e *escape) goDeferStmt(n *ir.GoDeferStmt) {
 		n.SetEsc(ir.EscNever)
 	}
 
-	call := n.Call
-
-	init := n.PtrInit()
-	init.Append(ir.TakeInit(call)...)
-	e.stmts(*init)
-
 	// If the function is already a zero argument/result function call,
 	// just escape analyze it normally.
 	//
 	// Note that the runtime is aware of this optimization for
 	// "go" statements that start in reflect.makeFuncStub or
 	// reflect.methodValueCall.
-	if call, ok := call.(*ir.CallExpr); ok && call.Op() == ir.OCALLFUNC {
-		if sig := call.X.Type(); sig.NumParams()+sig.NumResults() == 0 {
-			if clo, ok := call.X.(*ir.ClosureExpr); ok && n.Op() == ir.OGO {
-				clo.IsGoWrap = true
-			}
-			e.expr(k, call.X)
-			return
-		}
-	}
 
-	// Create a new no-argument function that we'll hand off to defer.
-	fn := ir.NewClosureFunc(n.Pos(), n.Pos(), types.NewSignature(nil, nil, nil), e.curfn, typecheck.Target)
-	fn.SetWrapper(true)
-	fn.SetEsc(escFuncTagged) // no params; effectively tagged already
-	fn.Body = []ir.Node{call}
-	if call, ok := call.(*ir.CallExpr); ok && call.Op() == ir.OCALLFUNC {
-		// If the callee is a named function, link to the original callee.
-		x := call.X
-		if x.Op() == ir.ONAME && x.(*ir.Name).Class == ir.PFUNC {
-			fn.WrappedFunc = call.X.(*ir.Name).Func
-		} else if x.Op() == ir.OMETHEXPR && ir.MethodExprFunc(x).Nname != nil {
-			fn.WrappedFunc = ir.MethodExprName(x).Func
-		}
+	call, ok := n.Call.(*ir.CallExpr)
+	if !ok || call.Op() != ir.OCALLFUNC {
+		base.FatalfAt(n.Pos(), "expected function call: %v", n.Call)
+	}
+	if sig := call.X.Type(); sig.NumParams()+sig.NumResults() != 0 {
+		base.FatalfAt(n.Pos(), "expected signature without parameters or results: %v", sig)
 	}
 
-	clo := fn.OClosure
-
-	if n.Op() == ir.OGO {
+	if clo, ok := call.X.(*ir.ClosureExpr); ok && n.Op() == ir.OGO {
 		clo.IsGoWrap = true
 	}
 
-	e.callCommon(nil, call, init, fn)
-	e.closures = append(e.closures, closure{e.spill(k, clo), clo})
-
-	// Create new top level call to closure.
-	n.Call = ir.NewCallExpr(call.Pos(), ir.OCALL, clo, nil)
-	ir.WithFunc(e.curfn, func() {
-		typecheck.Stmt(n.Call)
-	})
+	e.expr(k, call.X)
 }
 
 // rewriteArgument rewrites the argument *argp of the given call expression.
@@ -317,6 +275,10 @@ func (e *escape) rewriteArgument(argp *ir.Node, init *ir.Nodes, call ir.Node, fn
 		}
 
 		// Create and declare a new pointer-typed temp variable.
+		//
+		// TODO(mdempsky): This potentially violates the Go spec's order
+		// of evaluations, by evaluating arg.X before any other
+		// operands.
 		tmp := e.wrapExpr(arg.Pos(), &arg.X, init, call, wrapper)
 
 		k := e.mutatorHole()
diff --git a/src/cmd/compile/internal/ir/func.go b/src/cmd/compile/internal/ir/func.go
index 7efc71d2c7..356d0b070f 100644
--- a/src/cmd/compile/internal/ir/func.go
+++ b/src/cmd/compile/internal/ir/func.go
@@ -96,10 +96,15 @@ type Func struct {
 
 	Inl *Inline
 
-	// Closgen tracks how many closures have been generated within
-	// this function. Used by closurename for creating unique
+	// funcLitGen and goDeferGen track how many closures have been
+	// created in this function for function literals and go/defer
+	// wrappers, respectively. Used by closureName for creating unique
 	// function names.
-	Closgen int32
+	//
+	// Tracking goDeferGen separately avoids wrappers throwing off
+	// function literal numbering (e.g., runtime/trace_test.TestTraceSymbolize.func11).
+	funcLitGen int32
+	goDeferGen int32
 
 	Label int32 // largest auto-generated label in this function
 
@@ -358,25 +363,35 @@ func IsTrivialClosure(clo *ClosureExpr) bool {
 var globClosgen int32
 
 // closureName generates a new unique name for a closure within outerfn at pos.
-func closureName(outerfn *Func, pos src.XPos) *types.Sym {
+func closureName(outerfn *Func, pos src.XPos, why Op) *types.Sym {
 	pkg := types.LocalPkg
 	outer := "glob."
-	prefix := "func"
-	gen := &globClosgen
-
-	if outerfn != nil {
-		if outerfn.OClosure != nil {
-			prefix = ""
+	var prefix string
+	switch why {
+	default:
+		base.FatalfAt(pos, "closureName: bad Op: %v", why)
+	case OCLOSURE:
+		if outerfn == nil || outerfn.OClosure == nil {
+			prefix = "func"
 		}
+	case OGO:
+		prefix = "gowrap"
+	case ODEFER:
+		prefix = "deferwrap"
+	}
+	gen := &globClosgen
 
+	// There may be multiple functions named "_". In those
+	// cases, we can't use their individual Closgens as it
+	// would lead to name clashes.
+	if outerfn != nil && !IsBlank(outerfn.Nname) {
 		pkg = outerfn.Sym().Pkg
 		outer = FuncName(outerfn)
 
-		// There may be multiple functions named "_". In those
-		// cases, we can't use their individual Closgens as it
-		// would lead to name clashes.
-		if !IsBlank(outerfn.Nname) {
-			gen = &outerfn.Closgen
+		if why == OCLOSURE {
+			gen = &outerfn.funcLitGen
+		} else {
+			gen = &outerfn.goDeferGen
 		}
 	}
 
@@ -406,8 +421,12 @@ func closureName(outerfn *Func, pos src.XPos) *types.Sym {
 //
 // outerfn is the enclosing function, if any. The returned function is
 // appending to pkg.Funcs.
-func NewClosureFunc(fpos, cpos src.XPos, typ *types.Type, outerfn *Func, pkg *Package) *Func {
-	fn := NewFunc(fpos, fpos, closureName(outerfn, cpos), typ)
+//
+// why is the reason we're generating this Func. It can be OCLOSURE
+// (for a normal function literal) or OGO or ODEFER (for wrapping a
+// call expression that has parameters or results).
+func NewClosureFunc(fpos, cpos src.XPos, why Op, typ *types.Type, outerfn *Func, pkg *Package) *Func {
+	fn := NewFunc(fpos, fpos, closureName(outerfn, cpos, why), typ)
 	fn.SetIsHiddenClosure(outerfn != nil)
 
 	clo := &ClosureExpr{Func: fn}
diff --git a/src/cmd/compile/internal/ir/sizeof_test.go b/src/cmd/compile/internal/ir/sizeof_test.go
index 307f40d484..3d2c14318f 100644
--- a/src/cmd/compile/internal/ir/sizeof_test.go
+++ b/src/cmd/compile/internal/ir/sizeof_test.go
@@ -20,7 +20,7 @@ func TestSizeof(t *testing.T) {
 		_32bit uintptr     // size on 32bit platforms
 		_64bit uintptr     // size on 64bit platforms
 	}{
-		{Func{}, 188, 328},
+		{Func{}, 192, 336},
 		{Name{}, 100, 176},
 	}
 
diff --git a/src/cmd/compile/internal/noder/reader.go b/src/cmd/compile/internal/noder/reader.go
index a07fec68ec..013c73f3d5 100644
--- a/src/cmd/compile/internal/noder/reader.go
+++ b/src/cmd/compile/internal/noder/reader.go
@@ -3143,7 +3143,7 @@ func (r *reader) inlClosureFunc(origPos src.XPos, sig *types.Type) *ir.Func {
 	}
 
 	// TODO(mdempsky): Remove hard-coding of typecheck.Target.
-	return ir.NewClosureFunc(origPos, r.inlPos(origPos), sig, curfn, typecheck.Target)
+	return ir.NewClosureFunc(origPos, r.inlPos(origPos), ir.OCLOSURE, sig, curfn, typecheck.Target)
 }
 
 func (r *reader) exprList() []ir.Node {
diff --git a/src/cmd/compile/internal/typecheck/stmt.go b/src/cmd/compile/internal/typecheck/stmt.go
index b902fd9a58..4c21f045af 100644
--- a/src/cmd/compile/internal/typecheck/stmt.go
+++ b/src/cmd/compile/internal/typecheck/stmt.go
@@ -198,58 +198,181 @@ func tcFor(n *ir.ForStmt) ir.Node {
 	return n
 }
 
+// tcGoDefer typechecks an OGO/ODEFER statement.
+//
+// Really, this means normalizing the statement to always use a simple
+// function call with no arguments and no results. For example, it
+// rewrites:
+//
+//	defer f(x, y)
+//
+// into:
+//
+//	x1, y1 := x, y
+//	defer func() { f(x1, y1) }()
 func tcGoDefer(n *ir.GoDeferStmt) {
-	what := "defer"
-	if n.Op() == ir.OGO {
-		what = "go"
-	}
-
-	switch n.Call.Op() {
-	// ok
-	case ir.OCALLINTER,
-		ir.OCALLMETH,
-		ir.OCALLFUNC,
-		ir.OCLEAR,
-		ir.OCLOSE,
-		ir.OCOPY,
-		ir.ODELETE,
-		ir.OMAX,
-		ir.OMIN,
-		ir.OPANIC,
-		ir.OPRINT,
-		ir.OPRINTN,
-		ir.ORECOVER,
-		ir.ORECOVERFP:
-		return
+	call := n.Call
+
+	init := n.PtrInit()
+	init.Append(ir.TakeInit(call)...)
 
-	case ir.OAPPEND,
-		ir.OCAP,
-		ir.OCOMPLEX,
-		ir.OIMAG,
-		ir.OLEN,
-		ir.OMAKE,
-		ir.OMAKESLICE,
-		ir.OMAKECHAN,
-		ir.OMAKEMAP,
-		ir.ONEW,
-		ir.OREAL,
-		ir.OLITERAL: // conversion or unsafe.Alignof, Offsetof, Sizeof
-		if orig := ir.Orig(n.Call); orig.Op() == ir.OCONV {
-			break
+	if call, ok := n.Call.(*ir.CallExpr); ok && call.Op() == ir.OCALLFUNC {
+		if sig := call.X.Type(); sig.NumParams()+sig.NumResults() == 0 {
+			return // already in normal form
 		}
-		base.ErrorfAt(n.Pos(), errors.UnusedResults, "%s discards result of %v", what, n.Call)
-		return
 	}
 
-	// type is broken or missing, most likely a method call on a broken type
-	// we will warn about the broken type elsewhere. no need to emit a potentially confusing error
-	if n.Call.Type() == nil {
-		return
+	// Create a new wrapper function without parameters or results.
+	wrapperFn := ir.NewClosureFunc(n.Pos(), n.Pos(), n.Op(), types.NewSignature(nil, nil, nil), ir.CurFunc, Target)
+	wrapperFn.SetWrapper(true)
+
+	// argps collects the list of operands within the call expression
+	// that must be evaluated at the go/defer statement.
+	var argps []*ir.Node
+
+	var visit func(argp *ir.Node)
+	visit = func(argp *ir.Node) {
+		arg := *argp
+		if arg == nil {
+			return
+		}
+
+		// Recognize a few common expressions that can be evaluated within
+		// the wrapper, so we don't need to allocate space for them within
+		// the closure.
+		switch arg.Op() {
+		case ir.OLITERAL, ir.ONIL, ir.OMETHEXPR:
+			return
+		case ir.ONAME:
+			arg := arg.(*ir.Name)
+			if arg.Class == ir.PFUNC {
+				return // reference to global function
+			}
+		case ir.OADDR:
+			arg := arg.(*ir.AddrExpr)
+			if arg.X.Op() == ir.OLINKSYMOFFSET {
+				return // address of global symbol
+			}
+
+		case ir.OCONVNOP:
+			arg := arg.(*ir.ConvExpr)
+
+			// For unsafe.Pointer->uintptr conversion arguments, save the
+			// unsafe.Pointer argument. This is necessary to handle cases
+			// like fixedbugs/issue24491a.go correctly.
+			//
+			// TODO(mdempsky): Limit to static callees with
+			// //go:uintptr{escapes,keepalive}?
+			if arg.Type().IsUintptr() && arg.X.Type().IsUnsafePtr() {
+				visit(&arg.X)
+				return
+			}
+
+		case ir.OARRAYLIT, ir.OSLICELIT, ir.OSTRUCTLIT:
+			// TODO(mdempsky): For very large slices, it may be preferable
+			// to construct them at the go/defer statement instead.
+			list := arg.(*ir.CompLitExpr).List
+			for i, el := range list {
+				switch el := el.(type) {
+				case *ir.KeyExpr:
+					visit(&el.Value)
+				case *ir.StructKeyExpr:
+					visit(&el.Value)
+				default:
+					visit(&list[i])
+				}
+			}
+			return
+		}
+
+		argps = append(argps, argp)
+	}
+
+	visitList := func(list []ir.Node) {
+		for i := range list {
+			visit(&list[i])
+		}
+	}
+
+	switch call.Op() {
+	default:
+		base.Fatalf("unexpected call op: %v", call.Op())
+
+	case ir.OCALLFUNC:
+		call := call.(*ir.CallExpr)
+
+		// If the callee is a named function, link to the original callee.
+		if wrapped := ir.StaticCalleeName(call.X); wrapped != nil {
+			wrapperFn.WrappedFunc = wrapped.Func
+		}
+
+		visit(&call.X)
+		visitList(call.Args)
+
+	case ir.OCALLINTER:
+		call := call.(*ir.CallExpr)
+		argps = append(argps, &call.X.(*ir.SelectorExpr).X) // must be first for OCHECKNIL; see below
+		visitList(call.Args)
+
+	case ir.OAPPEND, ir.ODELETE, ir.OPRINT, ir.OPRINTN, ir.ORECOVERFP:
+		call := call.(*ir.CallExpr)
+		visitList(call.Args)
+		visit(&call.RType)
+
+	case ir.OCOPY:
+		call := call.(*ir.BinaryExpr)
+		visit(&call.X)
+		visit(&call.Y)
+		visit(&call.RType)
+
+	case ir.OCLEAR, ir.OCLOSE, ir.OPANIC:
+		call := call.(*ir.UnaryExpr)
+		visit(&call.X)
 	}
 
-	// The syntax made sure it was a call, so this must be
-	// a conversion.
-	base.FatalfAt(n.Pos(), "%s requires function call, not conversion", what)
+	if len(argps) != 0 {
+		// Found one or more operands that need to be evaluated upfront
+		// and spilled to temporary variables, which can be captured by
+		// the wrapper function.
+
+		stmtPos := base.Pos
+		callPos := base.Pos
+
+		as := ir.NewAssignListStmt(callPos, ir.OAS2, make([]ir.Node, len(argps)), make([]ir.Node, len(argps)))
+		for i, argp := range argps {
+			arg := *argp
+
+			pos := callPos
+			if ir.HasUniquePos(arg) {
+				pos = arg.Pos()
+			}
+
+			// tmp := arg
+			tmp := TempAt(pos, ir.CurFunc, arg.Type())
+			init.Append(Stmt(ir.NewDecl(pos, ir.ODCL, tmp)))
+			tmp.Defn = as
+			as.Lhs[i] = tmp
+			as.Rhs[i] = arg
+
+			// Rewrite original expression to use/capture tmp.
+			*argp = ir.NewClosureVar(pos, wrapperFn, tmp)
+		}
+		init.Append(Stmt(as))
+
+		// For "go/defer iface.M()", if iface is nil, we need to panic at
+		// the point of the go/defer statement.
+		if call.Op() == ir.OCALLINTER {
+			iface := as.Lhs[0]
+			init.Append(Stmt(ir.NewUnaryExpr(stmtPos, ir.OCHECKNIL, ir.NewUnaryExpr(iface.Pos(), ir.OITAB, iface))))
+		}
+	}
+
+	// Move call into the wrapper function, now that it's safe to
+	// evaluate there.
+	wrapperFn.Body = []ir.Node{call}
+
+	// Finally, rewrite the go/defer statement to call the wrapper.
+	n.Call = Call(call.Pos(), wrapperFn.OClosure, nil, false)
 }
 
 // tcIf typechecks an OIF node.
diff --git a/src/runtime/race/output_test.go b/src/runtime/race/output_test.go
index 4c2c3397cf..0c636ff6c1 100644
--- a/src/runtime/race/output_test.go
+++ b/src/runtime/race/output_test.go
@@ -439,7 +439,7 @@ Goroutine [0-9] \(running\) created at:
   main\.main\(\)
       .*/main.go:[0-9]+ \+0x[0-9,a-f]+
 ==================`}},
-	// Test symbolizing wrappers. Both (*T).f and main.func1 are wrappers.
+	// Test symbolizing wrappers. Both (*T).f and main.gowrap1 are wrappers.
 	// go.dev/issue/60245
 	{"wrappersym", "run", "", "atexit_sleep_ms=0", `
 package main
@@ -465,7 +465,7 @@ Write at 0x[0-9,a-f]+ by goroutine [0-9]:
       .*/main.go:15 \+0x[0-9,a-f]+
   main\.\(\*T\)\.f\(\)
       <autogenerated>:1 \+0x[0-9,a-f]+
-  main\.main\.func1\(\)
+  main\.main\.gowrap1\(\)
       .*/main.go:9 \+0x[0-9,a-f]+
 
 Previous write at 0x[0-9,a-f]+ by main goroutine:
diff --git a/test/fixedbugs/issue24491a.go b/test/fixedbugs/issue24491a.go
index d30b65b233..1f74818604 100644
--- a/test/fixedbugs/issue24491a.go
+++ b/test/fixedbugs/issue24491a.go
@@ -74,8 +74,8 @@ func main() {
 			break
 		}
 	}()
-
 	<-done
+
 	func() {
 		s := &S{}
 		defer s.test("method call", uintptr(setup()), uintptr(setup()), uintptr(setup()), uintptr(setup()))
diff --git a/test/fixedbugs/issue31573.go b/test/fixedbugs/issue31573.go
index eaab563431..a0cff3099a 100644
--- a/test/fixedbugs/issue31573.go
+++ b/test/fixedbugs/issue31573.go
@@ -1,4 +1,4 @@
-// errorcheck -0 -m
+// errorcheck -0 -m -l
 
 // Copyright 2019 The Go Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style
@@ -6,7 +6,7 @@
 
 package p
 
-func f(...*int) {} // ERROR "can inline f$"
+func f(...*int) {}
 
 func g() {
 	defer f()