Skip to content

Commit

Permalink
cmd/compile: fix ordering problems in struct equality
Browse files Browse the repository at this point in the history 
Make sure that if a field comparison might panic, we evaluate
(and short circuit if not equal) all previous fields, and don't
evaluate any subsequent fields.

Add a bunch more tests to the equality+panic checker.

Update #8606

Change-Id: I6a159bbc8da5b2b7ee835c0cd1fc565575b58c46
Reviewed-on: https://go-review.googlesource.com/c/go/+/237919
Run-TryBot: Keith Randall <[email protected]>
Reviewed-by: Matthew Dempsky <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
  • Loading branch information
@randall77
randall77 committed Jun 15, 2020
1 parent 8a33a8c commit 8c8045f
Show file tree
Hide file tree
Showing 2 changed files with 101 additions and 21 deletions.
63 changes: 50 additions & 13 deletions src/cmd/compile/internal/gc/alg.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,26 @@ func IncomparableField(t *types.Type) *types.Field {
return nil
}

// EqCanPanic reports whether == on type t could panic (has an interface somewhere).
// t must be comparable.
func EqCanPanic(t *types.Type) bool {
switch t.Etype {
default:
return false
case TINTER:
return true
case TARRAY:
return EqCanPanic(t.Elem())
case TSTRUCT:
for _, f := range t.FieldSlice() {
if !f.Sym.IsBlank() && EqCanPanic(f.Type) {
return true
}
}
return false
}
}

// algtype is like algtype1, except it returns the fixed-width AMEMxx variants
// instead of the general AMEM kind when possible.
func algtype(t *types.Type) AlgKind {
Expand Down Expand Up @@ -624,14 +644,19 @@ func geneq(t *types.Type) *obj.LSym {

case TSTRUCT:
// Build a list of conditions to satisfy.
// Track their order so that we can preserve aspects of that order.
// The conditions are a list-of-lists. Conditions are reorderable
// within each inner list. The outer lists must be evaluated in order.
// Even within each inner list, track their order so that we can preserve
// aspects of that order. (TODO: latter part needed?)
type nodeIdx struct {
n *Node
idx int
}
var conds []nodeIdx
var conds [][]nodeIdx
conds = append(conds, []nodeIdx{})
and := func(n *Node) {
conds = append(conds, nodeIdx{n: n, idx: len(conds)})
i := len(conds) - 1
conds[i] = append(conds[i], nodeIdx{n: n, idx: len(conds[i])})
}

// Walk the struct using memequal for runs of AMEM
Expand All @@ -647,6 +672,10 @@ func geneq(t *types.Type) *obj.LSym {

// Compare non-memory fields with field equality.
if !IsRegularMemory(f.Type) {
if EqCanPanic(f.Type) {
// Enforce ordering by starting a new set of reorderable conditions.
conds = append(conds, []nodeIdx{})
}
p := nodSym(OXDOT, np, f.Sym)
q := nodSym(OXDOT, nq, f.Sym)
switch {
Expand All @@ -657,6 +686,10 @@ func geneq(t *types.Type) *obj.LSym {
default:
and(nod(OEQ, p, q))
}
if EqCanPanic(f.Type) {
// Also enforce ordering after something that can panic.
conds = append(conds, []nodeIdx{})
}
i++
continue
}
Expand All @@ -680,20 +713,24 @@ func geneq(t *types.Type) *obj.LSym {

// Sort conditions to put runtime calls last.
// Preserve the rest of the ordering.
sort.SliceStable(conds, func(i, j int) bool {
x, y := conds[i], conds[j]
if (x.n.Op != OCALL) == (y.n.Op != OCALL) {
return x.idx < y.idx
}
return x.n.Op != OCALL
})
var flatConds []nodeIdx
for _, c := range conds {
sort.SliceStable(c, func(i, j int) bool {
x, y := c[i], c[j]
if (x.n.Op != OCALL) == (y.n.Op != OCALL) {
return x.idx < y.idx
}
return x.n.Op != OCALL
})
flatConds = append(flatConds, c...)
}

var cond *Node
if len(conds) == 0 {
if len(flatConds) == 0 {
cond = nodbool(true)
} else {
cond = conds[0].n
for _, c := range conds[1:] {
cond = flatConds[0].n
for _, c := range flatConds[1:] {
cond = nod(OANDAND, cond, c.n)
}
}
Expand Down
59 changes: 51 additions & 8 deletions test/fixedbugs/issue8606.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,22 +12,65 @@ import "fmt"

func main() {
type A [2]interface{}
type A2 [6]interface{}
type S struct{ x, y interface{} }
type S2 struct{ x, y, z, a, b, c interface{} }
type T1 struct {
i interface{}
a int64
j interface{}
}
type T2 struct {
i interface{}
a, b, c int64
j interface{}
}
type T3 struct {
i interface{}
s string
j interface{}
}
b := []byte{1}

for _, test := range []struct {
panic bool
a, b interface{}
}{
{false, A{1, []byte{1}}, A{2, []byte{1}}},
{true, A{[]byte{1}, 1}, A{[]byte{1}, 2}},
{false, S{1, []byte{1}}, S{2, []byte{1}}},
{true, S{[]byte{1}, 1}, S{[]byte{1}, 2}},
{false, A{1, []byte{1}}, A{"2", []byte{1}}},
{true, A{[]byte{1}, 1}, A{[]byte{1}, "2"}},
{false, S{1, []byte{1}}, S{"2", []byte{1}}},
{true, S{[]byte{1}, 1}, S{[]byte{1}, "2"}},
{false, A{1, b}, A{2, b}},
{true, A{b, 1}, A{b, 2}},
{false, A{1, b}, A{"2", b}},
{true, A{b, 1}, A{b, "2"}},

{false, A2{1, b}, A2{2, b}},
{true, A2{b, 1}, A2{b, 2}},
{false, A2{1, b}, A2{"2", b}},
{true, A2{b, 1}, A2{b, "2"}},

{false, S{1, b}, S{2, b}},
{true, S{b, 1}, S{b, 2}},
{false, S{1, b}, S{"2", b}},
{true, S{b, 1}, S{b, "2"}},

{false, S2{x: 1, y: b}, S2{x: 2, y: b}},
{true, S2{x: b, y: 1}, S2{x: b, y: 2}},
{false, S2{x: 1, y: b}, S2{x: "2", y: b}},
{true, S2{x: b, y: 1}, S2{x: b, y: "2"}},

{true, T1{i: b, a: 1}, T1{i: b, a: 2}},
{false, T1{a: 1, j: b}, T1{a: 2, j: b}},
{true, T2{i: b, a: 1}, T2{i: b, a: 2}},
{false, T2{a: 1, j: b}, T2{a: 2, j: b}},
{true, T3{i: b, s: "foo"}, T3{i: b, s: "bar"}},
{false, T3{s: "foo", j: b}, T3{s: "bar", j: b}},
{true, T3{i: b, s: "fooz"}, T3{i: b, s: "bar"}},
{false, T3{s: "fooz", j: b}, T3{s: "bar", j: b}},
} {
f := func() {
defer func() {
if recover() != nil {
panic(fmt.Sprintf("comparing %#v and %#v panicked", test.a, test.b))
}
}()
if test.a == test.b {
panic(fmt.Sprintf("values %#v and %#v should not be equal", test.a, test.b))
}
Expand Down

0 comments on commit 8c8045f

Please sign in to comment.