cmd/compile: handle some missing cases of non-SSAable values for args of open-coded defers

In my experimentation, I had found that most non-SSAable expressions were
converted to autotmp variables during AST evaluation. However, this was not true
generally, as witnessed by issue #35213, which has a non-SSAable field reference
of a struct that is not converted to an autotmp. So, I fixed openDeferSave() to
handle non-SSAable nodes more generally, and make sure that these non-SSAable
expressions are not evaluated more than once (which could incorrectly repeat side
effects).

Fixes #35213

Change-Id: I8043d5576b455e94163599e930ca0275e550d594
Reviewed-on: https://go-review.googlesource.com/c/go/+/203888
Run-TryBot: Dan Scales <danscales@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>

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

@@ -4105,7 +4105,7 @@ func (s *state) openDeferRecord(n *Node) {
 		// runtime panic code to use. But in the defer exit code, we will
 		// call the function directly if it is a static function.
 		closureVal := s.expr(fn)
-		closure := s.openDeferSave(fn, fn.Type, closureVal)
+		closure := s.openDeferSave(nil, fn.Type, closureVal)
 		opendefer.closureNode = closure.Aux.(*Node)
 		if !(fn.Op == ONAME && fn.Class() == PFUNC) {
 			opendefer.closure = closure
@@ -4118,14 +4118,14 @@ func (s *state) openDeferRecord(n *Node) {
 		// We must always store the function value in a stack slot for the
 		// runtime panic code to use. But in the defer exit code, we will
 		// call the method directly.
-		closure := s.openDeferSave(fn, fn.Type, closureVal)
+		closure := s.openDeferSave(nil, fn.Type, closureVal)
 		opendefer.closureNode = closure.Aux.(*Node)
 	} else {
 		if fn.Op != ODOTINTER {
 			Fatalf("OCALLINTER: n.Left not an ODOTINTER: %v", fn.Op)
 		}
 		closure, rcvr := s.getClosureAndRcvr(fn)
-		opendefer.closure = s.openDeferSave(fn, closure.Type, closure)
+		opendefer.closure = s.openDeferSave(nil, closure.Type, closure)
 		// Important to get the receiver type correct, so it is recognized
 		// as a pointer for GC purposes.
 		opendefer.rcvr = s.openDeferSave(nil, fn.Type.Recv().Type, rcvr)
@@ -4133,7 +4133,12 @@ func (s *state) openDeferRecord(n *Node) {
 		opendefer.rcvrNode = opendefer.rcvr.Aux.(*Node)
 	}
 	for _, argn := range n.Rlist.Slice() {
-		v := s.openDeferSave(argn, argn.Type, s.expr(argn))
+		var v *ssa.Value
+		if canSSAType(argn.Type) {
+			v = s.openDeferSave(nil, argn.Type, s.expr(argn))
+		} else {
+			v = s.openDeferSave(argn, argn.Type, nil)
+		}
 		args = append(args, v)
 		argNodes = append(argNodes, v.Aux.(*Node))
 	}
@@ -4150,13 +4155,22 @@ func (s *state) openDeferRecord(n *Node) {
 	s.store(types.Types[TUINT8], s.deferBitsAddr, newDeferBits)
 }
 
-// openDeferSave generates SSA nodes to store a value val (with type t) for an
-// open-coded defer on the stack at an explicit autotmp location, so it can be
-// reloaded and used for the appropriate call on exit. n is the associated node,
-// which is only needed if the associated type is non-SSAable. It returns an SSA
-// value representing a pointer to the stack location.
+// openDeferSave generates SSA nodes to store a value (with type t) for an
+// open-coded defer at an explicit autotmp location on the stack, so it can be
+// reloaded and used for the appropriate call on exit. If type t is SSAable, then
+// val must be non-nil (and n should be nil) and val is the value to be stored. If
+// type t is non-SSAable, then n must be non-nil (and val should be nil) and n is
+// evaluated (via s.addr() below) to get the value that is to be stored. The
+// function returns an SSA value representing a pointer to the autotmp location.
 func (s *state) openDeferSave(n *Node, t *types.Type, val *ssa.Value) *ssa.Value {
-	argTemp := tempAt(val.Pos.WithNotStmt(), s.curfn, t)
+	canSSA := canSSAType(t)
+	var pos src.XPos
+	if canSSA {
+		pos = val.Pos
+	} else {
+		pos = n.Pos
+	}
+	argTemp := tempAt(pos.WithNotStmt(), s.curfn, t)
 	argTemp.Name.SetOpenDeferSlot(true)
 	var addrArgTemp *ssa.Value
 	// Use OpVarLive to make sure stack slots for the args, etc. are not
@@ -4185,10 +4199,7 @@ func (s *state) openDeferSave(n *Node, t *types.Type, val *ssa.Value) *ssa.Value
 		// uninitialized pointer value.
 		argTemp.Name.SetNeedzero(true)
 	}
-	if !canSSAType(t) {
-		if n.Op != ONAME {
-			panic(fmt.Sprintf("Non-SSAable value should be a named location: %v", n))
-		}
+	if !canSSA {
 		a := s.addr(n, false)
 		s.move(t, addrArgTemp, a)
 		return addrArgTemp

src/runtime/defer_test.go

@@ -181,12 +181,16 @@ type bigStruct struct {
 	x, y, z, w, p, q int64
 }
 
+type containsBigStruct struct {
+	element bigStruct
+}
+
 func mknonSSAable() nonSSAable {
 	globint1++
 	return nonSSAable{0, 0, 0, 0, 5}
 }
 
-var globint1, globint2 int
+var globint1, globint2, globint3 int
 
 //go:noinline
 func sideeffect(n int64) int64 {
@@ -194,12 +198,20 @@ func sideeffect(n int64) int64 {
 	return n
 }
 
+func sideeffect2(in containsBigStruct) containsBigStruct {
+	globint3++
+	return in
+}
+
 // Test that nonSSAable arguments to defer are handled correctly and only evaluated once.
 func TestNonSSAableArgs(t *testing.T) {
 	globint1 = 0
 	globint2 = 0
+	globint3 = 0
 	var save1 byte
 	var save2 int64
+	var save3 int64
+	var save4 int64
 
 	defer func() {
 		if globint1 != 1 {
@@ -214,12 +226,33 @@ func TestNonSSAableArgs(t *testing.T) {
 		if save2 != 2 {
 			t.Fatal(fmt.Sprintf("save2:  wanted: 2, got %v", save2))
 		}
+		if save3 != 4 {
+			t.Fatal(fmt.Sprintf("save3:  wanted: 4, got %v", save3))
+		}
+		if globint3 != 1 {
+			t.Fatal(fmt.Sprintf("globint3:  wanted: 1, got %v", globint3))
+		}
+		if save4 != 4 {
+			t.Fatal(fmt.Sprintf("save1:  wanted: 4, got %v", save4))
+		}
 	}()
 
+	// Test function returning a non-SSAable arg
 	defer func(n nonSSAable) {
 		save1 = n[4]
 	}(mknonSSAable())
+	// Test composite literal that is not SSAable
 	defer func(b bigStruct) {
 		save2 = b.y
 	}(bigStruct{1, 2, 3, 4, 5, sideeffect(6)})
+
+	// Test struct field reference that is non-SSAable
+	foo := containsBigStruct{}
+	foo.element.z = 4
+	defer func(element bigStruct) {
+		save3 = element.z
+	}(foo.element)
+	defer func(element bigStruct) {
+		save4 = element.z
+	}(sideeffect2(foo).element)
 }