cmd/compile: clean up equality generation

We're using sort.SliceStable, so no need to keep track of indexes as well.

Use a more robust test for whether a node is a call.

Add a test that we're actually reordering comparisons. This test fails
without the alg.go changes in this CL because eqstring uses OCALLFUNC
instead of OCALL for its data comparisons.

Update #8606

Change-Id: Ieeec33434c72e3aa328deb11cc415cfda05632e2
Reviewed-on: https://go-review.googlesource.com/c/go/+/237921
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>

diff --git a/src/cmd/compile/internal/gc/alg.go b/src/cmd/compile/internal/gc/alg.go
index e2e2374717e7db08efd7e677c43dc145a2590042..2b63700569e453be6646ad81cc3ccf2de3da474c 100644 (file)
--- a/src/cmd/compile/internal/gc/alg.go
+++ b/src/cmd/compile/internal/gc/alg.go
@@ -646,17 +646,11 @@ func geneq(t *types.Type) *obj.LSym {
                // Build a list of conditions to satisfy.
                // 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
-               conds = append(conds, []nodeIdx{})
+               var conds [][]*Node
+               conds = append(conds, []*Node{})
                and := func(n *Node) {
                        i := len(conds) - 1
-                       conds[i] = append(conds[i], nodeIdx{n: n, idx: len(conds[i])})
+                       conds[i] = append(conds[i], n)
                }
 
                // Walk the struct using memequal for runs of AMEM
@@ -674,7 +668,7 @@ func geneq(t *types.Type) *obj.LSym {
                        if !IsRegularMemory(f.Type) {
                                if EqCanPanic(f.Type) {
                                        // Enforce ordering by starting a new set of reorderable conditions.
-                                       conds = append(conds, []nodeIdx{})
+                                       conds = append(conds, []*Node{})
                                }
                                p := nodSym(OXDOT, np, f.Sym)
                                q := nodSym(OXDOT, nq, f.Sym)
@@ -688,7 +682,7 @@ func geneq(t *types.Type) *obj.LSym {
                                }
                                if EqCanPanic(f.Type) {
                                        // Also enforce ordering after something that can panic.
-                                       conds = append(conds, []nodeIdx{})
+                                       conds = append(conds, []*Node{})
                                }
                                i++
                                continue
@@ -713,14 +707,13 @@ func geneq(t *types.Type) *obj.LSym {
 
                // Sort conditions to put runtime calls last.
                // Preserve the rest of the ordering.
-               var flatConds []nodeIdx
+               var flatConds []*Node
                for _, c := range conds {
+                       isCall := func(n *Node) bool {
+                               return n.Op == OCALL || n.Op == OCALLFUNC
+                       }
                        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
+                               return !isCall(c[i]) && isCall(c[j])
                        })
                        flatConds = append(flatConds, c...)
                }
@@ -729,9 +722,9 @@ func geneq(t *types.Type) *obj.LSym {
                if len(flatConds) == 0 {
                        cond = nodbool(true)
                } else {
-                       cond = flatConds[0].n
+                       cond = flatConds[0]
                        for _, c := range flatConds[1:] {
-                               cond = nod(OANDAND, cond, c.n)
+                               cond = nod(OANDAND, cond, c)
                        }
                }
 

diff --git a/test/fixedbugs/issue8606b.go b/test/fixedbugs/issue8606b.go
new file mode 100644 (file)
index 0000000..448ea56
--- /dev/null
+++ b/test/fixedbugs/issue8606b.go
@@ -0,0 +1,63 @@
+// run
+
+// Copyright 2020 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.
+
+// This is an optimization check. We want to make sure that we compare
+// string lengths, and other scalar fields, before checking string
+// contents.  There's no way to verify this in the language, and
+// codegen tests in test/codegen can't really detect ordering
+// optimizations like this. Instead, we generate invalid strings with
+// bad backing store pointers but nonzero length, so we can check that
+// the backing store never gets compared.
+//
+// We use two different bad strings so that pointer comparisons of
+// backing store pointers fail.
+
+package main
+
+import (
+       "fmt"
+       "reflect"
+       "unsafe"
+)
+
+func bad1() string {
+       s := "foo"
+       (*reflect.StringHeader)(unsafe.Pointer(&s)).Data = 1 // write bad value to data ptr
+       return s
+}
+func bad2() string {
+       s := "foo"
+       (*reflect.StringHeader)(unsafe.Pointer(&s)).Data = 2 // write bad value to data ptr
+       return s
+}
+
+type SI struct {
+       s string
+       i int
+}
+
+type SS struct {
+       s string
+       t string
+}
+
+func main() {
+       for _, test := range []struct {
+               a, b interface{}
+       }{
+               {SI{s: bad1(), i: 1}, SI{s: bad2(), i: 2}},
+               {SS{s: bad1(), t: "a"}, SS{s: bad2(), t: "aa"}},
+               {SS{s: "a", t: bad1()}, SS{s: "b", t: bad2()}},
+               // This one would panic because the length of both strings match, and we check
+               // the body of the bad strings before the body of the good strings.
+               //{SS{s: bad1(), t: "a"}, SS{s: bad2(), t: "b"}},
+       } {
+               if test.a == test.b {
+                       panic(fmt.Sprintf("values %#v and %#v should not be equal", test.a, test.b))
+               }
+       }
+
+}