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

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>

diff --git a/src/cmd/compile/internal/gc/ssa.go b/src/cmd/compile/internal/gc/ssa.go
index d1eef69189abae9a4a189ee64a4be23940c73095..f76b6d4c021dbb98a79b173fce6aded203f6d57f 100644 (file)
--- a/src/cmd/compile/internal/gc/ssa.go
+++ b/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

diff --git a/src/runtime/defer_test.go b/src/runtime/defer_test.go
index d830fc591f0a4d3f619ba0639c7d12f03bae0cb1..f03bdb47d58a65f8c2fba8a4b86c0155b87e7e66 100644 (file)
--- a/src/runtime/defer_test.go
+++ b/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)
 }