From 1928cea0f0cc5f74e1840d00b5c809f7ed73402b Mon Sep 17 00:00:00 2001 From: Tim King Date: Wed, 12 Oct 2022 14:24:32 -0700 Subject: [PATCH] go/ssa: emit field and index lvals on demand Adds a new lazyAddress construct. This is the same as an *address except it emits a FieldAddr selection, Field selection, or IndexAddr on demand. This fixes issues with ordering on assignment statements. For example, x.f = e panics on x being nil in phase 2 of assignment statements. This change delays the introduction of the FieldAddr for x.f until it is used instead of as a side effect of (*builder).addr. The nil deref panic is from FieldAddr is now after side-effects of evaluating x and e but before the assignment to x.f. Fixes golang/go#55086 Change-Id: I0f215b209de5c5fd319aef3af677e071dbd168f8 Reviewed-on: https://go-review.googlesource.com/c/tools/+/442655 Reviewed-by: Alan Donovan Reviewed-by: Zvonimir Pavlinovic --- .../vta/testdata/src/function_alias.go | 44 +++--- go/ssa/builder.go | 32 +++-- go/ssa/interp/interp_test.go | 1 + .../interp/testdata/fixedbugs/issue55086.go | 132 ++++++++++++++++++ go/ssa/lvalue.go | 36 +++++ 5 files changed, 212 insertions(+), 33 deletions(-) create mode 100644 go/ssa/interp/testdata/fixedbugs/issue55086.go diff --git a/go/callgraph/vta/testdata/src/function_alias.go b/go/callgraph/vta/testdata/src/function_alias.go index b38e0e00d6..0a8dffe79d 100644 --- a/go/callgraph/vta/testdata/src/function_alias.go +++ b/go/callgraph/vta/testdata/src/function_alias.go @@ -33,42 +33,42 @@ func Baz(f func()) { // t2 = *t1 // *t2 = Baz$1 // t3 = local A (a) -// t4 = &t3.foo [#0] -// t5 = *t1 -// t6 = *t5 -// *t4 = t6 +// t4 = *t1 +// t5 = *t4 +// t6 = &t3.foo [#0] +// *t6 = t5 // t7 = &t3.foo [#0] // t8 = *t7 // t9 = t8() -// t10 = &t3.do [#1] *Doer -// t11 = &t3.foo [#0] *func() -// t12 = *t11 func() -// t13 = changetype Doer <- func() (t12) Doer -// *t10 = t13 +// t10 = &t3.foo [#0] *func() +// t11 = *t10 func() +// t12 = &t3.do [#1] *Doer +// t13 = changetype Doer <- func() (t11) Doer +// *t12 = t13 // t14 = &t3.do [#1] *Doer // t15 = *t14 Doer // t16 = t15() () // Flow chain showing that Baz$1 reaches t8(): -// Baz$1 -> t2 <-> PtrFunction(func()) <-> t5 -> t6 -> t4 <-> Field(testdata.A:foo) <-> t7 -> t8 +// Baz$1 -> t2 <-> PtrFunction(func()) <-> t4 -> t5 -> t6 <-> Field(testdata.A:foo) <-> t7 -> t8 // Flow chain showing that Baz$1 reaches t15(): -// Field(testdata.A:foo) <-> t11 -> t12 -> t13 -> t10 <-> Field(testdata.A:do) <-> t14 -> t15 +// Field(testdata.A:foo) <-> t10 -> t11 -> t13 -> t12 <-> Field(testdata.A:do) <-> t14 -> t15 // WANT: // Local(f) -> Local(t0) // Local(t0) -> PtrFunction(func()) // Function(Baz$1) -> Local(t2) -// PtrFunction(func()) -> Local(t0), Local(t2), Local(t5) +// PtrFunction(func()) -> Local(t0), Local(t2), Local(t4) // Local(t2) -> PtrFunction(func()) -// Local(t4) -> Field(testdata.A:foo) -// Local(t5) -> Local(t6), PtrFunction(func()) -// Local(t6) -> Local(t4) +// Local(t6) -> Field(testdata.A:foo) +// Local(t4) -> Local(t5), PtrFunction(func()) +// Local(t5) -> Local(t6) // Local(t7) -> Field(testdata.A:foo), Local(t8) -// Field(testdata.A:foo) -> Local(t11), Local(t4), Local(t7) -// Local(t4) -> Field(testdata.A:foo) -// Field(testdata.A:do) -> Local(t10), Local(t14) -// Local(t10) -> Field(testdata.A:do) -// Local(t11) -> Field(testdata.A:foo), Local(t12) -// Local(t12) -> Local(t13) -// Local(t13) -> Local(t10) +// Field(testdata.A:foo) -> Local(t10), Local(t6), Local(t7) +// Local(t6) -> Field(testdata.A:foo) +// Field(testdata.A:do) -> Local(t12), Local(t14) +// Local(t12) -> Field(testdata.A:do) +// Local(t10) -> Field(testdata.A:foo), Local(t11) +// Local(t11) -> Local(t13) +// Local(t13) -> Local(t12) // Local(t14) -> Field(testdata.A:do), Local(t15) diff --git a/go/ssa/builder.go b/go/ssa/builder.go index 23674c3d0d..8ec8f6e310 100644 --- a/go/ssa/builder.go +++ b/go/ssa/builder.go @@ -453,12 +453,16 @@ func (b *builder) addr(fn *Function, e ast.Expr, escaping bool) lvalue { } wantAddr := true v := b.receiver(fn, e.X, wantAddr, escaping, sel) - last := len(sel.index) - 1 - return &address{ - addr: emitFieldSelection(fn, v, sel.index[last], true, e.Sel), - pos: e.Sel.Pos(), - expr: e.Sel, + index := sel.index[len(sel.index)-1] + fld := typeparams.CoreType(deref(v.Type())).(*types.Struct).Field(index) + + // Due to the two phases of resolving AssignStmt, a panic from x.f = p() + // when x is nil is required to come after the side-effects of + // evaluating x and p(). + emit := func(fn *Function) Value { + return emitFieldSelection(fn, v, index, true, e.Sel) } + return &lazyAddress{addr: emit, t: fld.Type(), pos: e.Sel.Pos(), expr: e.Sel} case *ast.IndexExpr: var x Value @@ -487,13 +491,19 @@ func (b *builder) addr(fn *Function, e ast.Expr, escaping bool) lvalue { if isUntyped(index.Type()) { index = emitConv(fn, index, tInt) } - v := &IndexAddr{ - X: x, - Index: index, + // Due to the two phases of resolving AssignStmt, a panic from x[i] = p() + // when x is nil or i is out-of-bounds is required to come after the + // side-effects of evaluating x, i and p(). + emit := func(fn *Function) Value { + v := &IndexAddr{ + X: x, + Index: index, + } + v.setPos(e.Lbrack) + v.setType(et) + return fn.emit(v) } - v.setPos(e.Lbrack) - v.setType(et) - return &address{addr: fn.emit(v), pos: e.Lbrack, expr: e} + return &lazyAddress{addr: emit, t: deref(et), pos: e.Lbrack, expr: e} case *ast.StarExpr: return &address{addr: b.expr(fn, e.X), pos: e.Star, expr: e} diff --git a/go/ssa/interp/interp_test.go b/go/ssa/interp/interp_test.go index a0acf2f968..51a74015c9 100644 --- a/go/ssa/interp/interp_test.go +++ b/go/ssa/interp/interp_test.go @@ -127,6 +127,7 @@ var testdataTests = []string{ "width32.go", "fixedbugs/issue52342.go", + "fixedbugs/issue55086.go", } func init() { diff --git a/go/ssa/interp/testdata/fixedbugs/issue55086.go b/go/ssa/interp/testdata/fixedbugs/issue55086.go new file mode 100644 index 0000000000..84c81e91a2 --- /dev/null +++ b/go/ssa/interp/testdata/fixedbugs/issue55086.go @@ -0,0 +1,132 @@ +// Copyright 2022 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package main + +func a() (r string) { + s := "initial" + var p *struct{ i int } + defer func() { + recover() + r = s + }() + + s, p.i = "set", 2 // s must be set before p.i panics + return "unreachable" +} + +func b() (r string) { + s := "initial" + fn := func() []int { panic("") } + defer func() { + recover() + r = s + }() + + s, fn()[0] = "set", 2 // fn() panics before any assignment occurs + return "unreachable" +} + +func c() (r string) { + s := "initial" + var p map[int]int + defer func() { + recover() + r = s + }() + + s, p[0] = "set", 2 //s must be set before p[0] index panics" + return "unreachable" +} + +func d() (r string) { + s := "initial" + var p map[int]int + defer func() { + recover() + r = s + }() + fn := func() int { panic("") } + + s, p[0] = "set", fn() // fn() panics before s is set + return "unreachable" +} + +func e() (r string) { + s := "initial" + p := map[int]int{} + defer func() { + recover() + r = s + }() + fn := func() int { panic("") } + + s, p[fn()] = "set", 0 // fn() panics before any assignment occurs + return "unreachable" +} + +func f() (r string) { + s := "initial" + p := []int{} + defer func() { + recover() + r = s + }() + + s, p[1] = "set", 0 // p[1] panics after s is set + return "unreachable" +} + +func g() (r string) { + s := "initial" + p := map[any]any{} + defer func() { + recover() + r = s + }() + var i any = func() {} + s, p[i] = "set", 0 // p[i] panics after s is set + return "unreachable" +} + +func h() (r string) { + fail := false + defer func() { + recover() + if fail { + r = "fail" + } else { + r = "success" + } + }() + + type T struct{ f int } + var p *struct{ *T } + + // The implicit "p.T" operand should be evaluated in phase 1 (and panic), + // before the "fail = true" assignment in phase 2. + fail, p.f = true, 0 + return "unreachable" +} + +func main() { + for _, test := range []struct { + fn func() string + want string + desc string + }{ + {a, "set", "s must be set before p.i panics"}, + {b, "initial", "p() panics before s is set"}, + {c, "set", "s must be set before p[0] index panics"}, + {d, "initial", "fn() panics before s is set"}, + {e, "initial", "fn() panics before s is set"}, + {f, "set", "p[1] panics after s is set"}, + {g, "set", "p[i] panics after s is set"}, + {h, "success", "p.T panics before fail is set"}, + } { + if test.fn() != test.want { + panic(test.desc) + } + } +} diff --git a/go/ssa/lvalue.go b/go/ssa/lvalue.go index 64262def8b..455b1e50fa 100644 --- a/go/ssa/lvalue.go +++ b/go/ssa/lvalue.go @@ -93,6 +93,42 @@ func (e *element) typ() types.Type { return e.t } +// A lazyAddress is an lvalue whose address is the result of an instruction. +// These work like an *address except a new address.address() Value +// is created on each load, store and address call. +// A lazyAddress can be used to control when a side effect (nil pointer +// dereference, index out of bounds) of using a location happens. +type lazyAddress struct { + addr func(fn *Function) Value // emit to fn the computation of the address + t types.Type // type of the location + pos token.Pos // source position + expr ast.Expr // source syntax of the value (not address) [debug mode] +} + +func (l *lazyAddress) load(fn *Function) Value { + load := emitLoad(fn, l.addr(fn)) + load.pos = l.pos + return load +} + +func (l *lazyAddress) store(fn *Function, v Value) { + store := emitStore(fn, l.addr(fn), v, l.pos) + if l.expr != nil { + // store.Val is v, converted for assignability. + emitDebugRef(fn, l.expr, store.Val, false) + } +} + +func (l *lazyAddress) address(fn *Function) Value { + addr := l.addr(fn) + if l.expr != nil { + emitDebugRef(fn, l.expr, addr, true) + } + return addr +} + +func (l *lazyAddress) typ() types.Type { return l.t } + // A blank is a dummy variable whose name is "_". // It is not reified: loads are illegal and stores are ignored. type blank struct{}