From: Dan Scales Date: Mon, 28 Oct 2019 22:55:36 +0000 (-0700) Subject: cmd/compile: handle some missing cases of non-SSAable values for args of open-coded... X-Git-Tag: go1.14beta1~507 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=cc47b0d2cd1c633f1a3c21632978d1180f65195f;p=gostls13.git 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 TryBot-Result: Gobot Gobot Reviewed-by: Keith Randall --- diff --git a/src/cmd/compile/internal/gc/ssa.go b/src/cmd/compile/internal/gc/ssa.go index d1eef69189..f76b6d4c02 100644 --- 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 d830fc591f..f03bdb47d5 100644 --- 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) }