From 33fcc815f25acbad230357a496e783fb450fb0fe Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Fri, 13 Mar 2015 14:29:51 -0400 Subject: [PATCH] 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 --- go/ssa/builder.go | 168 +++++++++++++++++++++--------- go/ssa/emit.go | 6 -- go/ssa/interp/testdata/complit.go | 84 +++++++++++++++ go/ssa/lvalue.go | 2 +- go/ssa/testdata/valueforexpr.go | 10 +- 5 files changed, 211 insertions(+), 59 deletions(-) diff --git a/go/ssa/builder.go b/go/ssa/builder.go index f4418dfc217..d4c41d4a249 100644 --- a/go/ssa/builder.go +++ b/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 - emitDebugRef(fn, e, addr, true) + 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 + if !isZero && int64(len(e.Elts)) != at.Len() { + // 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) diff --git a/go/ssa/emit.go b/go/ssa/emit.go index f1ba0f7694a..fa9646bfd51 100644 --- a/go/ssa/emit.go +++ b/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. // diff --git a/go/ssa/interp/testdata/complit.go b/go/ssa/interp/testdata/complit.go index c44fc0068e0..02f9916436b 100644 --- a/go/ssa/interp/testdata/complit.go +++ b/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() { } diff --git a/go/ssa/lvalue.go b/go/ssa/lvalue.go index 8342645be25..4284b1c0e95 100644 --- a/go/ssa/lvalue.go +++ b/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 { diff --git a/go/ssa/testdata/valueforexpr.go b/go/ssa/testdata/valueforexpr.go index 0697ec63985..028c1531330 100644 --- a/go/ssa/testdata/valueforexpr.go +++ b/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{})