cmd/compile: fix SCCP propagation into jump tables

We can't delete all the outgoing edges and then add one back in, because
then we've lost the argument of any phi at the target. Instead, move
the important target to the front of the list and delete the rest.

This normally isn't a problem, because there is never normally a phi
at the target of a jump table. But this isn't quite true when in race
build mode, because there is a phi of the result of a bunch of raceread
calls.

The reason this happens is that each case is written like this (where e
is the runtime.eface we're switching on):

if e.type == $type.int32 {
   m = raceread(e.data, m1)
}
m2 = phi(m1, m)
if e.type == $type.int32 {
   .. do case ..
   goto blah
}

so that if e.type is not $type.int32, it falls through to the default
case. This default case will have a memory phi for all the (jumped around
and not actually called) raceread calls.

If we instead did it like

if e.type == $type.int32 {
  raceread(e.data)
  .. do case ..
  goto blah
}

That would paper over this bug, as it is the only way to construct
a jump table whose target is a block with a phi in it. (Yet.)

But we'll fix the underlying bug in this CL. Maybe we can do the
rewrite mentioned above later.  (It is an optimization for -race mode,
which isn't particularly important.)

Fixes #64606

Change-Id: I6f6e3c90eb1e2638112920ee2e5b6581cef04ea4
Reviewed-on: https://go-review.googlesource.com/c/go/+/548356
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: David Chase <drchase@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

diff --git a/src/cmd/compile/internal/ssa/block.go b/src/cmd/compile/internal/ssa/block.go
index 4a24a181e5de197c616b160de63313ef167e31ce..26af10b59c4bf6c2ff6fd4ec4ac46e5f9106661a 100644 (file)
--- a/src/cmd/compile/internal/ssa/block.go
+++ b/src/cmd/compile/internal/ssa/block.go
@@ -297,6 +297,8 @@ func (b *Block) removePred(i int) {
 // removeSucc removes the ith output edge from b.
 // It is the responsibility of the caller to remove
 // the corresponding predecessor edge.
+// Note that this potentially reorders successors of b, so it
+// must be used very carefully.
 func (b *Block) removeSucc(i int) {
        n := len(b.Succs) - 1
        if i != n {
@@ -323,6 +325,19 @@ func (b *Block) swapSuccessors() {
        b.Likely *= -1
 }
 
+// Swaps b.Succs[x] and b.Succs[y].
+func (b *Block) swapSuccessorsByIdx(x, y int) {
+       if x == y {
+               return
+       }
+       ex := b.Succs[x]
+       ey := b.Succs[y]
+       b.Succs[x] = ey
+       b.Succs[y] = ex
+       ex.b.Preds[ex.i].i = y
+       ey.b.Preds[ey.i].i = x
+}
+
 // removePhiArg removes the ith arg from phi.
 // It must be called after calling b.removePred(i) to
 // adjust the corresponding phi value of the block:
@@ -339,7 +354,7 @@ func (b *Block) swapSuccessors() {
 func (b *Block) removePhiArg(phi *Value, i int) {
        n := len(b.Preds)
        if numPhiArgs := len(phi.Args); numPhiArgs-1 != n {
-               b.Fatalf("inconsistent state, num predecessors: %d, num phi args: %d", n, numPhiArgs)
+               b.Fatalf("inconsistent state for %v, num predecessors: %d, num phi args: %d", phi, n, numPhiArgs)
        }
        phi.Args[i].Uses--
        phi.Args[i] = phi.Args[n]

diff --git a/src/cmd/compile/internal/ssa/deadcode.go b/src/cmd/compile/internal/ssa/deadcode.go
index ae9fd2ef2426812031b9227bb0b28521abdbddaa..3bd1737babbb252aaba47ea094ef3af40c28222c 100644 (file)
--- a/src/cmd/compile/internal/ssa/deadcode.go
+++ b/src/cmd/compile/internal/ssa/deadcode.go
@@ -312,6 +312,8 @@ func deadcode(f *Func) {
 
 // removeEdge removes the i'th outgoing edge from b (and
 // the corresponding incoming edge from b.Succs[i].b).
+// Note that this potentially reorders successors of b, so it
+// must be used very carefully.
 func (b *Block) removeEdge(i int) {
        e := b.Succs[i]
        c := e.b

diff --git a/src/cmd/compile/internal/ssa/sccp.go b/src/cmd/compile/internal/ssa/sccp.go
index 3c109548abd1e38e0b5b2fa0985895eb070d1638..86c6117d872b5dc5d54a533ba874428227c456d7 100644 (file)
--- a/src/cmd/compile/internal/ssa/sccp.go
+++ b/src/cmd/compile/internal/ssa/sccp.go
@@ -533,12 +533,12 @@ func rewireSuccessor(block *Block, constVal *Value) bool {
                block.ResetControls()
                return true
        case BlockJumpTable:
+               // Remove everything but the known taken branch.
                idx := int(constVal.AuxInt)
-               targetBlock := block.Succs[idx].b
-               for len(block.Succs) > 0 {
-                       block.removeEdge(0)
+               block.swapSuccessorsByIdx(0, idx)
+               for len(block.Succs) > 1 {
+                       block.removeEdge(1)
                }
-               block.AddEdgeTo(targetBlock)
                block.Kind = BlockPlain
                block.Likely = BranchUnknown
                block.ResetControls()

diff --git a/test/fixedbugs/issue64606.go b/test/fixedbugs/issue64606.go
new file mode 100644 (file)
index 0000000..9b53c10
--- /dev/null
+++ b/test/fixedbugs/issue64606.go
@@ -0,0 +1,32 @@
+// build -race
+
+//go:build race
+
+// Copyright 2023 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 main() {
+       var o any = uint64(5)
+       switch o.(type) {
+       case int:
+               goto ret
+       case int8:
+               goto ret
+       case int16:
+               goto ret
+       case int32:
+               goto ret
+       case int64:
+               goto ret
+       case float32:
+               goto ret
+       case float64:
+               goto ret
+       default:
+               goto ret
+       }
+ret:
+}