go/ssa: fix incorrect SSA code for composite literals

Composite literals are initialized in place where possible, but in
cases the initializer expression refers to the variable that
is being updated

	x = T{a: x.a}

we must ensure that the RHS is fully evaluated before we execute any
stores to x.  This means we need to record the sequence of stores in a
"store buffer" and execute it only once the entire composite literal
has been evaluated.

Fixes issue #10127

Change-Id: If94e3b179beb25feea5b298ed43de6a199aaf347
Reviewed-on: https://go-review.googlesource.com/7533
Reviewed-by: Robert Griesemer <gri@golang.org>

go/ssa/builder.go

@@ -358,7 +358,9 @@ func (b *builder) addr(fn *Function, e ast.Expr, escaping bool) lvalue {
 			v = fn.addLocal(t, e.Lbrace)
 		}
 		v.Comment = "complit"
-		b.compLit(fn, v, e, true) // initialize in place
+		var sb storebuf
+		b.compLit(fn, v, e, true, &sb)
+		sb.emit(fn)
 		return &address{addr: v, pos: e.Lbrace, expr: e}
 
 	case *ast.ParenExpr:
@@ -420,15 +422,39 @@ func (b *builder) addr(fn *Function, e ast.Expr, escaping bool) lvalue {
 	panic(fmt.Sprintf("unexpected address expression: %T", e))
 }
 
-// exprInPlace emits to fn code to initialize the lvalue loc with the
-// value of expression e. If isZero is true, exprInPlace assumes that loc
-// holds the zero value for its type.
+type store struct {
+	lhs lvalue
+	rhs Value
+}
+
+type storebuf struct{ stores []store }
+
+func (sb *storebuf) store(lhs lvalue, rhs Value) {
+	sb.stores = append(sb.stores, store{lhs, rhs})
+}
+
+func (sb *storebuf) emit(fn *Function) {
+	for _, s := range sb.stores {
+		s.lhs.store(fn, s.rhs)
+	}
+}
+
+// assign emits to fn code to initialize the lvalue loc with the value
+// of expression e.  If isZero is true, assign assumes that loc holds
+// the zero value for its type.
 //
-// This is equivalent to loc.store(fn, b.expr(fn, e)) but may
-// generate better code in some cases, e.g. for composite literals
-// in an addressable location.
+// This is equivalent to loc.store(fn, b.expr(fn, e)), but may generate
+// better code in some cases, e.g., for composite literals in an
+// addressable location.
 //
-func (b *builder) exprInPlace(fn *Function, loc lvalue, e ast.Expr, isZero bool) {
+// If sb is not nil, assign generates code to evaluate expression e, but
+// not to update loc.  Instead, the necessary stores are appended to the
+// storebuf sb so that they can be executed later.  This allows correct
+// in-place update of existing variables when the RHS is a composite
+// literal that may reference parts of the LHS.
+//
+func (b *builder) assign(fn *Function, loc lvalue, e ast.Expr, isZero bool, sb *storebuf) {
+	// Can we initialize it in place?
 	if e, ok := unparen(e).(*ast.CompositeLit); ok {
 		// A CompositeLit never evaluates to a pointer,
 		// so if the type of the location is a pointer,
@@ -436,7 +462,12 @@ func (b *builder) exprInPlace(fn *Function, loc lvalue, e ast.Expr, isZero bool)
 		if _, ok := loc.(blank); !ok { // avoid calling blank.typ()
 			if isPointer(loc.typ()) {
 				ptr := b.addr(fn, e, true).address(fn)
-				loc.store(fn, ptr) // copy address
+				// copy address
+				if sb != nil {
+					sb.store(loc, ptr)
+				} else {
+					loc.store(fn, ptr)
+				}
 				return
 			}
 		}
@@ -447,14 +478,35 @@ func (b *builder) exprInPlace(fn *Function, loc lvalue, e ast.Expr, isZero bool)
 				// Can't in-place initialize an interface value.
 				// Fall back to copying.
 			} else {
+				// x = T{...} or x := T{...}
 				addr := loc.address(fn)
-				b.compLit(fn, addr, e, isZero) // in place
+				if sb != nil {
+					b.compLit(fn, addr, e, isZero, sb)
+				} else {
+					var sb storebuf
+					b.compLit(fn, addr, e, isZero, &sb)
+					sb.emit(fn)
+				}
+
+				// Subtle: emit debug ref for aggregate types only;
+				// slice and map are handled by store ops in compLit.
+				switch loc.typ().Underlying().(type) {
+				case *types.Struct, *types.Array:
 					emitDebugRef(fn, e, addr, true)
+				}
+
 				return
 			}
 		}
 	}
-	loc.store(fn, b.expr(fn, e)) // copy value
+
+	// simple case: just copy
+	rhs := b.expr(fn, e)
+	if sb != nil {
+		sb.store(loc, rhs)
+	} else {
+		loc.store(fn, rhs)
+	}
 }
 
 // expr lowers a single-result expression e to SSA form, emitting code
@@ -938,7 +990,7 @@ func (b *builder) localValueSpec(fn *Function, spec *ast.ValueSpec) {
 				fn.addLocalForIdent(id)
 			}
 			lval := b.addr(fn, id, false) // non-escaping
-			b.exprInPlace(fn, lval, spec.Values[i], true)
+			b.assign(fn, lval, spec.Values[i], true, nil)
 		}
 
 	case len(spec.Values) == 0:
@@ -973,37 +1025,33 @@ func (b *builder) localValueSpec(fn *Function, spec *ast.ValueSpec) {
 //
 func (b *builder) assignStmt(fn *Function, lhss, rhss []ast.Expr, isDef bool) {
 	// Side effects of all LHSs and RHSs must occur in left-to-right order.
-	var lvals []lvalue
-	for _, lhs := range lhss {
+	lvals := make([]lvalue, len(lhss))
+	isZero := make([]bool, len(lhss))
+	for i, lhs := range lhss {
 		var lval lvalue = blank{}
 		if !isBlankIdent(lhs) {
 			if isDef {
 				if obj := fn.Pkg.info.Defs[lhs.(*ast.Ident)]; obj != nil {
 					fn.addNamedLocal(obj)
+					isZero[i] = true
 				}
 			}
 			lval = b.addr(fn, lhs, false) // non-escaping
 		}
-		lvals = append(lvals, lval)
+		lvals[i] = lval
 	}
 	if len(lhss) == len(rhss) {
-		// e.g. x, y = f(), g()
-		if len(lhss) == 1 {
-			// x = type{...}
-			// Optimization: in-place construction
-			// of composite literals.
-			b.exprInPlace(fn, lvals[0], rhss[0], false)
-		} else {
-			// Parallel assignment.  All reads must occur
-			// before all updates, precluding exprInPlace.
-			var rvals []Value
-			for _, rval := range rhss {
-				rvals = append(rvals, b.expr(fn, rval))
-			}
-			for i, lval := range lvals {
-				lval.store(fn, rvals[i])
-			}
+		// Simple assignment:   x     = f()        (!isDef)
+		// Parallel assignment: x, y  = f(), g()   (!isDef)
+		// or short var decl:   x, y := f(), g()   (isDef)
+		//
+		// In all cases, the RHSs may refer to the LHSs,
+		// so we need a storebuf.
+		var sb storebuf
+		for i := range rhss {
+			b.assign(fn, lvals[i], rhss[i], isZero[i], &sb)
 		}
+		sb.emit(fn)
 	} else {
 		// e.g. x, y = pos()
 		tuple := b.exprN(fn, rhss[0])
@@ -1031,22 +1079,32 @@ func (b *builder) arrayLen(fn *Function, elts []ast.Expr) int64 {
 }
 
 // compLit emits to fn code to initialize a composite literal e at
-// address addr with type typ, typically allocated by Alloc.
+// address addr with type typ.
+//
 // Nested composite literals are recursively initialized in place
 // where possible. If isZero is true, compLit assumes that addr
 // holds the zero value for typ.
 //
+// Because the elements of a composite literal may refer to the
+// variables being updated, as in the second line below,
+//	x := T{a: 1}
+//	x = T{a: x.a}
+// all the reads must occur before all the writes.  Thus all stores to
+// loc are emitted to the storebuf sb for later execution.
+//
 // A CompositeLit may have pointer type only in the recursive (nested)
 // case when the type name is implicit.  e.g. in []*T{{}}, the inner
 // literal has type *T behaves like &T{}.
 // In that case, addr must hold a T, not a *T.
 //
-func (b *builder) compLit(fn *Function, addr Value, e *ast.CompositeLit, isZero bool) {
+func (b *builder) compLit(fn *Function, addr Value, e *ast.CompositeLit, isZero bool, sb *storebuf) {
 	typ := deref(fn.Pkg.typeOf(e))
 	switch t := typ.Underlying().(type) {
 	case *types.Struct:
 		if !isZero && len(e.Elts) != t.NumFields() {
-			emitMemClear(fn, addr, e.Lbrace)
+			// memclear
+			sb.store(&address{addr, e.Lbrace, nil},
+				zeroValue(fn, deref(addr.Type())))
 			isZero = true
 		}
 		for i, e := range e.Elts {
@@ -1071,7 +1129,7 @@ func (b *builder) compLit(fn *Function, addr Value, e *ast.CompositeLit, isZero
 			}
 			faddr.setType(types.NewPointer(sf.Type()))
 			fn.emit(faddr)
-			b.exprInPlace(fn, &address{addr: faddr, pos: pos, expr: e}, e, isZero)
+			b.assign(fn, &address{addr: faddr, pos: pos, expr: e}, e, isZero, sb)
 		}
 
 	case *types.Array, *types.Slice:
@@ -1083,21 +1141,23 @@ func (b *builder) compLit(fn *Function, addr Value, e *ast.CompositeLit, isZero
 			alloc := emitNew(fn, at, e.Lbrace)
 			alloc.Comment = "slicelit"
 			array = alloc
-			isZero = true
 		case *types.Array:
 			at = t
 			array = addr
-		}
 
 			if !isZero && int64(len(e.Elts)) != at.Len() {
-			emitMemClear(fn, array, e.Lbrace)
-			isZero = true
+				// memclear
+				sb.store(&address{array, e.Lbrace, nil},
+					zeroValue(fn, deref(array.Type())))
+			}
 		}
 
 		var idx *Const
 		for _, e := range e.Elts {
+			pos := e.Pos()
 			if kv, ok := e.(*ast.KeyValueExpr); ok {
 				idx = b.expr(fn, kv.Key).(*Const)
+				pos = kv.Colon
 				e = kv.Value
 			} else {
 				var idxval int64
@@ -1112,30 +1172,44 @@ func (b *builder) compLit(fn *Function, addr Value, e *ast.CompositeLit, isZero
 			}
 			iaddr.setType(types.NewPointer(at.Elem()))
 			fn.emit(iaddr)
-			b.exprInPlace(fn, &address{addr: iaddr, pos: e.Pos(), expr: e}, e, isZero)
+			if t != at { // slice
+				// backing array is unaliased => storebuf not needed.
+				b.assign(fn, &address{addr: iaddr, pos: pos, expr: e}, e, true, nil)
+			} else {
+				b.assign(fn, &address{addr: iaddr, pos: pos, expr: e}, e, true, sb)
 			}
+		}
+
 		if t != at { // slice
 			s := &Slice{X: array}
 			s.setPos(e.Lbrace)
 			s.setType(typ)
-			emitStore(fn, addr, fn.emit(s), e.Lbrace)
+			sb.store(&address{addr: addr, pos: e.Lbrace, expr: e}, fn.emit(s))
 		}
 
 	case *types.Map:
 		m := &MakeMap{Reserve: intConst(int64(len(e.Elts)))}
 		m.setPos(e.Lbrace)
 		m.setType(typ)
-		emitStore(fn, addr, fn.emit(m), e.Lbrace)
+		fn.emit(m)
 		for _, e := range e.Elts {
 			e := e.(*ast.KeyValueExpr)
-			loc := &element{
+			loc := element{
 				m:   m,
 				k:   emitConv(fn, b.expr(fn, e.Key), t.Key()),
 				t:   t.Elem(),
 				pos: e.Colon,
 			}
-			b.exprInPlace(fn, loc, e.Value, true)
+
+			// We call assign() only because it takes care
+			// of any &-operation required in the recursive
+			// case, e.g.,
+			// map[int]*struct{}{0: {}} implies &struct{}{}.
+			// In-place update is of course impossible,
+			// and no storebuf is needed.
+			b.assign(fn, &loc, e.Value, true, nil)
 		}
+		sb.store(&address{addr: addr, pos: e.Lbrace, expr: e}, m)
 
 	default:
 		panic("unexpected CompositeLit type: " + t.String())
@@ -2231,7 +2305,7 @@ func (p *Package) Build() {
 			} else {
 				lval = blank{}
 			}
-			b.exprInPlace(init, lval, varinit.Rhs, true)
+			b.assign(init, lval, varinit.Rhs, true, nil)
 		} else {
 			// n:1 initialization: var x, y :=  f()
 			tuple := b.exprN(init, varinit.Rhs)

go/ssa/emit.go

@@ -430,12 +430,6 @@ func zeroValue(f *Function, t types.Type) Value {
 	}
 }
 
-// emitMemClear emits to f code to zero the value pointed to by ptr.
-func emitMemClear(f *Function, ptr Value, pos token.Pos) {
-	// TODO(adonovan): define and use a 'memclr' intrinsic for aggregate types.
-	emitStore(f, ptr, zeroValue(f, deref(ptr.Type())), pos)
-}
-
 // createRecoverBlock emits to f a block of code to return after a
 // recovered panic, and sets f.Recover to it.
 //

go/ssa/interp/testdata/complit.go

@@ -80,5 +80,89 @@ func init() {
 	}
 }
 
+// Regression test for https://github.com/golang/go/issues/10127:
+// composite literal clobbers destination before reading from it.
+func init() {
+	// map
+	{
+		type M map[string]int
+		m := M{"x": 1, "y": 2}
+		m = M{"x": m["y"], "y": m["x"]}
+		if m["x"] != 2 || m["y"] != 1 {
+			panic(fmt.Sprint(m))
+		}
+
+		n := M{"x": 3}
+		m, n = M{"x": n["x"]}, M{"x": m["x"]} // parallel assignment
+		if got := fmt.Sprint(m["x"], n["x"]); got != "3 2" {
+			panic(got)
+		}
+	}
+
+	// struct
+	{
+		type T struct{ x, y, z int }
+		t := T{x: 1, y: 2, z: 3}
+
+		t = T{x: t.y, y: t.z, z: t.x} // all fields
+		if got := fmt.Sprint(t); got != "{2 3 1}" {
+			panic(got)
+		}
+
+		t = T{x: t.y, y: t.z + 3} // not all fields
+		if got := fmt.Sprint(t); got != "{3 4 0}" {
+			panic(got)
+		}
+
+		u := T{x: 5, y: 6, z: 7}
+		t, u = T{x: u.x}, T{x: t.x} // parallel assignment
+		if got := fmt.Sprint(t, u); got != "{5 0 0} {3 0 0}" {
+			panic(got)
+		}
+	}
+
+	// array
+	{
+		a := [3]int{0: 1, 1: 2, 2: 3}
+
+		a = [3]int{0: a[1], 1: a[2], 2: a[0]} //  all elements
+		if got := fmt.Sprint(a); got != "[2 3 1]" {
+			panic(got)
+		}
+
+		a = [3]int{0: a[1], 1: a[2] + 3} //  not all elements
+		if got := fmt.Sprint(a); got != "[3 4 0]" {
+			panic(got)
+		}
+
+		b := [3]int{0: 5, 1: 6, 2: 7}
+		a, b = [3]int{0: b[0]}, [3]int{0: a[0]} // parallel assignment
+		if got := fmt.Sprint(a, b); got != "[5 0 0] [3 0 0]" {
+			panic(got)
+		}
+	}
+
+	// slice
+	{
+		s := []int{0: 1, 1: 2, 2: 3}
+
+		s = []int{0: s[1], 1: s[2], 2: s[0]} //  all elements
+		if got := fmt.Sprint(s); got != "[2 3 1]" {
+			panic(got)
+		}
+
+		s = []int{0: s[1], 1: s[2] + 3} //  not all elements
+		if got := fmt.Sprint(s); got != "[3 4]" {
+			panic(got)
+		}
+
+		t := []int{0: 5, 1: 6, 2: 7}
+		s, t = []int{0: t[0]}, []int{0: s[0]} // parallel assignment
+		if got := fmt.Sprint(s, t); got != "[5] [3]" {
+			panic(got)
+		}
+	}
+}
+
 func main() {
 }

go/ssa/lvalue.go

@@ -29,7 +29,7 @@ type lvalue interface {
 type address struct {
 	addr Value
 	pos  token.Pos // source position
-	expr ast.Expr  // source syntax [debug mode]
+	expr ast.Expr  // source syntax of the value (not address) [debug mode]
 }
 
 func (a *address) load(fn *Function) Value {

go/ssa/testdata/valueforexpr.go

@@ -76,11 +76,11 @@ func complit() {
 	// 1. Slices
 	print( /*@Slice*/ ([]int{}))
 	print( /*@Alloc*/ (&[]int{}))
-	print(& /*@Alloc*/ ([]int{}))
+	print(& /*@Slice*/ ([]int{}))
 
 	sl1 := /*@Slice*/ ([]int{})
 	sl2 := /*@Alloc*/ (&[]int{})
-	sl3 := & /*@Alloc*/ ([]int{})
+	sl3 := & /*@Slice*/ ([]int{})
 	_, _, _ = sl1, sl2, sl3
 
 	_ = /*@Slice*/ ([]int{})
@@ -98,18 +98,18 @@ func complit() {
 	_, _, _ = arr1, arr2, arr3
 
 	_ = /*@UnOp*/ ([1]int{})
-	_ = /*@Alloc*/ (& /*@Alloc*/ ([1]int{})) // & optimized away
+	_ = /*@Alloc*/ (& /*@Alloc*/ ([1]int{}))
 	_ = & /*@Alloc*/ ([1]int{})
 
 	// 3. Maps
 	type M map[int]int
 	print( /*@MakeMap*/ (M{}))
 	print( /*@Alloc*/ (&M{}))
-	print(& /*@Alloc*/ (M{}))
+	print(& /*@MakeMap*/ (M{}))
 
 	m1 := /*@MakeMap*/ (M{})
 	m2 := /*@Alloc*/ (&M{})
-	m3 := & /*@Alloc*/ (M{})
+	m3 := & /*@MakeMap*/ (M{})
 	_, _, _ = m1, m2, m3
 
 	_ = /*@MakeMap*/ (M{})