cmd/compile: compare size in dead store elimination

Only remove stores that is shadowed by another store with same or
larger size. Normally we don't need this check because we did check
the types, but unsafe pointer casting can get around it.

Fixes #16769.

Change-Id: I3f7c6c57807b590a2f735007dec6c65a4fa01a34
Reviewed-on: https://go-review.googlesource.com/27320
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>

diff --git a/src/cmd/compile/internal/ssa/deadstore.go b/src/cmd/compile/internal/ssa/deadstore.go
index a1fc5fd0bbcb8d514046ddf507f009c1c323257a..7acc3b09d498ac92eabcda92bf85c43a6b0acad8 100644 (file)
--- a/src/cmd/compile/internal/ssa/deadstore.go
+++ b/src/cmd/compile/internal/ssa/deadstore.go
@@ -14,8 +14,7 @@ func dse(f *Func) {
        defer f.retSparseSet(loadUse)
        storeUse := f.newSparseSet(f.NumValues())
        defer f.retSparseSet(storeUse)
-       shadowed := f.newSparseSet(f.NumValues())
-       defer f.retSparseSet(shadowed)
+       shadowed := newSparseMap(f.NumValues()) // TODO: cache
        for _, b := range f.Blocks {
                // Find all the stores in this block. Categorize their uses:
                //  loadUse contains stores which are used by a subsequent load.
@@ -81,17 +80,23 @@ func dse(f *Func) {
                        shadowed.clear()
                }
                if v.Op == OpStore || v.Op == OpZero {
-                       if shadowed.contains(v.Args[0].ID) {
+                       var sz int64
+                       if v.Op == OpStore {
+                               sz = v.AuxInt
+                       } else { // OpZero
+                               sz = SizeAndAlign(v.AuxInt).Size()
+                       }
+                       if shadowedSize := int64(shadowed.get(v.Args[0].ID)); shadowedSize != -1 && shadowedSize >= sz {
                                // Modify store into a copy
                                if v.Op == OpStore {
                                        // store addr value mem
                                        v.SetArgs1(v.Args[2])
                                } else {
                                        // zero addr mem
-                                       sz := v.Args[0].Type.ElemType().Size()
-                                       if SizeAndAlign(v.AuxInt).Size() != sz {
+                                       typesz := v.Args[0].Type.ElemType().Size()
+                                       if sz != typesz {
                                                f.Fatalf("mismatched zero/store sizes: %d and %d [%s]",
-                                                       v.AuxInt, sz, v.LongString())
+                                                       sz, typesz, v.LongString())
                                        }
                                        v.SetArgs1(v.Args[1])
                                }
@@ -99,7 +104,10 @@ func dse(f *Func) {
                                v.AuxInt = 0
                                v.Op = OpCopy
                        } else {
-                               shadowed.add(v.Args[0].ID)
+                               if sz > 0x7fffffff { // work around sparseMap's int32 value type
+                                       sz = 0x7fffffff
+                               }
+                               shadowed.set(v.Args[0].ID, int32(sz))
                        }
                }
                // walk to previous store

diff --git a/src/cmd/compile/internal/ssa/deadstore_test.go b/src/cmd/compile/internal/ssa/deadstore_test.go
index c38f1cdbaf7682605e3614c30d4bc4823ff2a937..beb6de063705a96a24cd3eb1d926d52b65a435e2 100644 (file)
--- a/src/cmd/compile/internal/ssa/deadstore_test.go
+++ b/src/cmd/compile/internal/ssa/deadstore_test.go
@@ -8,7 +8,7 @@ import "testing"
 
 func TestDeadStore(t *testing.T) {
        c := testConfig(t)
-       elemType := &TypeImpl{Size_: 8, Name: "testtype"}
+       elemType := &TypeImpl{Size_: 1, Name: "testtype"}
        ptrType := &TypeImpl{Size_: 8, Ptr: true, Name: "testptr", Elem_: elemType} // dummy for testing
        fun := Fun(c, "entry",
                Bloc("entry",
@@ -18,7 +18,7 @@ func TestDeadStore(t *testing.T) {
                        Valu("addr1", OpAddr, ptrType, 0, nil, "sb"),
                        Valu("addr2", OpAddr, ptrType, 0, nil, "sb"),
                        Valu("addr3", OpAddr, ptrType, 0, nil, "sb"),
-                       Valu("zero1", OpZero, TypeMem, 8, nil, "addr3", "start"),
+                       Valu("zero1", OpZero, TypeMem, 1, nil, "addr3", "start"),
                        Valu("store1", OpStore, TypeMem, 1, nil, "addr1", "v", "zero1"),
                        Valu("store2", OpStore, TypeMem, 1, nil, "addr2", "v", "store1"),
                        Valu("store3", OpStore, TypeMem, 1, nil, "addr1", "v", "store2"),
@@ -95,3 +95,32 @@ func TestDeadStoreTypes(t *testing.T) {
                t.Errorf("store %s incorrectly removed", v)
        }
 }
+
+func TestDeadStoreUnsafe(t *testing.T) {
+       // Make sure a narrow store can't shadow a wider one. The test above
+       // covers the case of two different types, but unsafe pointer casting
+       // can get to a point where the size is changed but type unchanged.
+       c := testConfig(t)
+       ptrType := &TypeImpl{Size_: 8, Ptr: true, Name: "testptr"} // dummy for testing
+       fun := Fun(c, "entry",
+               Bloc("entry",
+                       Valu("start", OpInitMem, TypeMem, 0, nil),
+                       Valu("sb", OpSB, TypeInvalid, 0, nil),
+                       Valu("v", OpConstBool, TypeBool, 1, nil),
+                       Valu("addr1", OpAddr, ptrType, 0, nil, "sb"),
+                       Valu("store1", OpStore, TypeMem, 8, nil, "addr1", "v", "start"),  // store 8 bytes
+                       Valu("store2", OpStore, TypeMem, 1, nil, "addr1", "v", "store1"), // store 1 byte
+                       Goto("exit")),
+               Bloc("exit",
+                       Exit("store2")))
+
+       CheckFunc(fun.f)
+       cse(fun.f)
+       dse(fun.f)
+       CheckFunc(fun.f)
+
+       v := fun.values["store1"]
+       if v.Op == OpCopy {
+               t.Errorf("store %s incorrectly removed", v)
+       }
+}