cmd/compile: disallow CMOV optimization with ptr arithmetic as an arg

    if q != nil {
        p = &q.f
    }

Which gets rewritten to a conditional move:

    tmp := &q.f
    p = Select q!=nil, tmp, p

Unfortunately, we can't compute &q.f before we've checked if q is nil,
because if it is nil, &q.f is an invalid pointer (if f's offset is
nonzero but small).

Normally this is not a problem because the tmp variable above
immediately dies, and is thus not live across any safepoint. However,
if later there is another &q.f computation, those two computations are
CSEd, causing tmp to be used at both use points. That will extend
tmp's lifetime, possibly across a call.

Fixes #56990

Change-Id: I3ea31be93feae04fbe3304cb11323194c5df3879
Reviewed-on: https://go-review.googlesource.com/c/go/+/454155
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>

diff --git a/src/cmd/compile/internal/ssa/branchelim.go b/src/cmd/compile/internal/ssa/branchelim.go
index 5a06bfb2209a8eb581c423781845710c079e5757..f16959dd572973e62fb46a8ab1a61522b173b1ec 100644 (file)
--- a/src/cmd/compile/internal/ssa/branchelim.go
+++ b/src/cmd/compile/internal/ssa/branchelim.go
@@ -436,7 +436,7 @@ func canSpeculativelyExecute(b *Block) bool {
        // don't fuse memory ops, Phi ops, divides (can panic),
        // or anything else with side-effects
        for _, v := range b.Values {
-               if v.Op == OpPhi || isDivMod(v.Op) || v.Type.IsMemory() ||
+               if v.Op == OpPhi || isDivMod(v.Op) || isPtrArithmetic(v.Op) || v.Type.IsMemory() ||
                        v.MemoryArg() != nil || opcodeTable[v.Op].hasSideEffects {
                        return false
                }
@@ -456,3 +456,15 @@ func isDivMod(op Op) bool {
                return false
        }
 }
+
+func isPtrArithmetic(op Op) bool {
+       // Pointer arithmetic can't be speculatively executed because the result
+       // may be an invalid pointer (if, for example, the condition is that the
+       // base pointer is not nil). See issue 56990.
+       switch op {
+       case OpOffPtr, OpAddPtr, OpSubPtr:
+               return true
+       default:
+               return false
+       }
+}

diff --git a/test/fixedbugs/issue56990.go b/test/fixedbugs/issue56990.go
new file mode 100644 (file)
index 0000000..4fa6d75
--- /dev/null
+++ b/test/fixedbugs/issue56990.go
@@ -0,0 +1,119 @@
+// run
+
+// 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
+
+import (
+       "fmt"
+       "path/filepath"
+       "testing"
+)
+
+var t *testing.T
+
+type TypeMeta struct {
+       Kind       string
+       APIVersion string
+}
+
+type ObjectMeta struct {
+       Name         string `json:"name,omitempty" protobuf:"bytes,1,opt,name=name"`
+       GenerateName string `json:"generateName,omitempty" protobuf:"bytes,2,opt,name=generateName"`
+       Namespace    string `json:"namespace,omitempty" protobuf:"bytes,3,opt,name=namespace"`
+       SelfLink     string `json:"selfLink,omitempty" protobuf:"bytes,4,opt,name=selfLink"`
+}
+
+type ConfigSpec struct {
+       Disks        []DiskSpec
+       StorageClass string
+}
+
+type DiskSpec struct {
+       Name         string
+       Size         string
+       StorageClass string
+       Annotations  map[string]string
+       VolumeName   string
+}
+
+// Config is the Schema for the configs API.
+type Config struct {
+       TypeMeta
+       ObjectMeta
+
+       Spec ConfigSpec
+}
+
+func findDiskSize(diskSpec *DiskSpec, configSpec *ConfigSpec) string {
+       t.Log(fmt.Sprintf("Hello World"))
+       return diskSpec.Size
+}
+
+func findStorageClassName(diskSpec *DiskSpec, configSpec *ConfigSpec) *string {
+       if diskSpec.StorageClass != "" {
+               return &diskSpec.StorageClass
+       }
+
+       if configSpec != nil {
+               for _, d := range configSpec.Disks {
+                       if d.Name == diskSpec.Name {
+                               if d.StorageClass != "" {
+                                       return &d.StorageClass
+                               }
+                               break
+                       }
+               }
+
+               if configSpec.StorageClass != "" {
+                       return &configSpec.StorageClass
+               }
+       }
+       return nil
+}
+
+func Bar(config *Config) *ConfigSpec {
+       var configSpec *ConfigSpec
+       if config != nil {
+               configSpec = &config.Spec
+       }
+       return configSpec
+}
+
+func Foo(diskSpec DiskSpec, config *Config) {
+       cs := Bar(config)
+       _ = findDiskSize(&diskSpec, cs)
+       cs = Bar(config)
+       _ = findStorageClassName(&diskSpec, cs)
+
+}
+
+func TestPanic(tt *testing.T) {
+       t = tt
+       myarray := []string{filepath.Join("..", "config", "crd", "bases")}
+
+       for i := 0; i < 1000; i++ {
+               Foo(DiskSpec{
+                       Name: "DataDisk",
+                       Size: "1Gi",
+               }, nil)
+       }
+
+       t.Log(myarray)
+}
+
+// Hack to run tests in a playground
+func matchString(a, b string) (bool, error) {
+       return a == b, nil
+}
+func main() {
+       testSuite := []testing.InternalTest{
+               {
+                       Name: "TestPanic",
+                       F:    TestPanic,
+               },
+       }
+       testing.Main(matchString, testSuite, nil, nil)
+}

diff --git a/test/fixedbugs/issue56990.out b/test/fixedbugs/issue56990.out
new file mode 100644 (file)
index 0000000..7ef22e9
--- /dev/null
+++ b/test/fixedbugs/issue56990.out
@@ -0,0 +1 @@
+PASS