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>

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
+		var conds [][]*Node
-		// aspects of that order. (TODO: latter part needed?)
+		conds = append(conds, []*Node{})
-		type nodeIdx struct {
-			n   *Node
-			idx int
-		}
-		var conds [][]nodeIdx
-		conds = append(conds, []nodeIdx{})
 		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 {
-			sort.SliceStable(c, func(i, j int) bool {
+			isCall := func(n *Node) bool {
-				x, y := c[i], c[j]
+				return n.Op == OCALL || n.Op == OCALLFUNC
-				if (x.n.Op != OCALL) == (y.n.Op != OCALL) {
-					return x.idx < y.idx
 			}
-				return x.n.Op != OCALL
+			sort.SliceStable(c, func(i, j int) bool {
+				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)
 			}
 		}
 

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))
+		}
+	}
+
+}