[release-branch.go1.21] cmd/compile: ensure pointer arithmetic happen…

…s after the nil check

Have nil checks return a pointer that is known non-nil. Users of
that pointer can use the result, ensuring that they are ordered
after the nil check itself.

The order dependence goes away after scheduling, when we've fixed
an order. At that point we move uses back to the original pointer
so it doesn't change regalloc any.

This prevents pointer arithmetic on nil from being spilled to the
stack and then observed by a stack scan.

Fixes #63743

Change-Id: I1a5fa4f2e6d9000d672792b4f90dfc1b7b67f6ea
Reviewed-on: https://go-review.googlesource.com/c/go/+/537775
Reviewed-by: David Chase <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Keith Randall <[email protected]>
(cherry picked from commit 962ccbe)
Reviewed-on: https://go-review.googlesource.com/c/go/+/538717
Auto-Submit: Heschi Kreinick <[email protected]>
Reviewed-by: Heschi Kreinick <[email protected]>

src/cmd/compile/internal/ssa/_gen/generic.rules

@@ -981,7 +981,7 @@
     (ConstNil <typ.Uintptr>)
     (ConstNil <typ.BytePtr>))
 
-(NilCheck (GetG mem) mem) => mem
+(NilCheck ptr:(GetG mem) mem) => ptr
 
 (If (Not cond) yes no) => (If cond no yes)
 (If (ConstBool [c]) yes no) && c => (First yes no)
@@ -2055,19 +2055,19 @@
 	&& isSameCall(call.Aux, "runtime.newobject")
 	=> mem
 
-(NilCheck (SelectN [0] call:(StaticLECall _ _)) _)
+(NilCheck ptr:(SelectN [0] call:(StaticLECall _ _)) _)
 	&& isSameCall(call.Aux, "runtime.newobject")
 	&& warnRule(fe.Debug_checknil(), v, "removed nil check")
-	=> (Invalid)
+	=> ptr
 
-(NilCheck (OffPtr (SelectN [0] call:(StaticLECall _ _))) _)
+(NilCheck ptr:(OffPtr (SelectN [0] call:(StaticLECall _ _))) _)
 	&& isSameCall(call.Aux, "runtime.newobject")
 	&& warnRule(fe.Debug_checknil(), v, "removed nil check")
-	=> (Invalid)
+	=> ptr
 
 // Addresses of globals are always non-nil.
-(NilCheck          (Addr {_} (SB))    _) => (Invalid)
-(NilCheck (Convert (Addr {_} (SB)) _) _) => (Invalid)
+(NilCheck          ptr:(Addr {_} (SB))    _) => ptr
+(NilCheck ptr:(Convert (Addr {_} (SB)) _) _) => ptr
 
 // for late-expanded calls, recognize memequal applied to a single constant byte
 // Support is limited by 1, 2, 4, 8 byte sizes

src/cmd/compile/internal/ssa/_gen/genericOps.go

@@ -471,7 +471,7 @@ var genericOps = []opData{
 	{name: "IsNonNil", argLength: 1, typ: "Bool"},        // arg0 != nil
 	{name: "IsInBounds", argLength: 2, typ: "Bool"},      // 0 <= arg0 < arg1. arg1 is guaranteed >= 0.
 	{name: "IsSliceInBounds", argLength: 2, typ: "Bool"}, // 0 <= arg0 <= arg1. arg1 is guaranteed >= 0.
-	{name: "NilCheck", argLength: 2, typ: "Void"},        // arg0=ptr, arg1=mem. Panics if arg0 is nil. Returns void.
+	{name: "NilCheck", argLength: 2, nilCheck: true},     // arg0=ptr, arg1=mem. Panics if arg0 is nil. Returns the ptr unmodified.
 
 	// Pseudo-ops
 	{name: "GetG", argLength: 1, zeroWidth: true}, // runtime.getg() (read g pointer). arg0=mem

src/cmd/compile/internal/ssa/check.go

@@ -317,7 +317,28 @@ func checkFunc(f *Func) {
 				if !v.Aux.(*ir.Name).Type().HasPointers() {
 					f.Fatalf("vardef must have pointer type %s", v.Aux.(*ir.Name).Type().String())
 				}
-
+			case OpNilCheck:
+				// nil checks have pointer type before scheduling, and
+				// void type after scheduling.
+				if f.scheduled {
+					if v.Uses != 0 {
+						f.Fatalf("nilcheck must have 0 uses %s", v.Uses)
+					}
+					if !v.Type.IsVoid() {
+						f.Fatalf("nilcheck must have void type %s", v.Type.String())
+					}
+				} else {
+					if !v.Type.IsPtrShaped() && !v.Type.IsUintptr() {
+						f.Fatalf("nilcheck must have pointer type %s", v.Type.String())
+					}
+				}
+				if !v.Args[0].Type.IsPtrShaped() && !v.Args[0].Type.IsUintptr() {
+					f.Fatalf("nilcheck must have argument of pointer type %s", v.Args[0].Type.String())
+				}
+				if !v.Args[1].Type.IsMemory() {
+					f.Fatalf("bad arg 1 type to %s: want mem, have %s",
+						v.Op, v.Args[1].Type.String())
+				}
 			}
 
 			// TODO: check for cycles in values

src/cmd/compile/internal/ssa/deadcode.go

@@ -110,16 +110,15 @@ func liveValues(f *Func, reachable []bool) (live []bool, liveOrderStmts []*Value
 			}
 		}
 		for _, v := range b.Values {
-			if (opcodeTable[v.Op].call || opcodeTable[v.Op].hasSideEffects) && !live[v.ID] {
+			if (opcodeTable[v.Op].call || opcodeTable[v.Op].hasSideEffects || opcodeTable[v.Op].nilCheck) && !live[v.ID] {
 				live[v.ID] = true
 				q = append(q, v)
 				if v.Pos.IsStmt() != src.PosNotStmt {
 					liveOrderStmts = append(liveOrderStmts, v)
 				}
 			}
-			if v.Type.IsVoid() && !live[v.ID] {
-				// The only Void ops are nil checks and inline marks.  We must keep these.
-				if v.Op == OpInlMark && !liveInlIdx[int(v.AuxInt)] {
+			if v.Op == OpInlMark {
+				if !liveInlIdx[int(v.AuxInt)] {
 					// We don't need marks for bodies that
 					// have been completely optimized away.
 					// TODO: save marks only for bodies which

src/cmd/compile/internal/ssa/deadstore.go

@@ -249,7 +249,7 @@ func elimDeadAutosGeneric(f *Func) {
 		}
 
 		if v.Uses == 0 && v.Op != OpNilCheck && !v.Op.IsCall() && !v.Op.HasSideEffects() || len(args) == 0 {
-			// Nil check has no use, but we need to keep it.
+			// We need to keep nil checks even if they have no use.
 			// Also keep calls and values that have side effects.
 			return
 		}

src/cmd/compile/internal/ssa/fuse.go

@@ -169,7 +169,7 @@ func fuseBlockIf(b *Block) bool {
 // There may be false positives.
 func isEmpty(b *Block) bool {
 	for _, v := range b.Values {
-		if v.Uses > 0 || v.Op.IsCall() || v.Op.HasSideEffects() || v.Type.IsVoid() {
+		if v.Uses > 0 || v.Op.IsCall() || v.Op.HasSideEffects() || v.Type.IsVoid() || opcodeTable[v.Op].nilCheck {
 			return false
 		}
 	}

src/cmd/compile/internal/ssa/fuse_test.go

@@ -254,7 +254,7 @@ func TestFuseSideEffects(t *testing.T) {
 			Valu("p", OpArg, c.config.Types.IntPtr, 0, nil),
 			If("c1", "z0", "exit")),
 		Bloc("z0",
-			Valu("nilcheck", OpNilCheck, types.TypeVoid, 0, nil, "p", "mem"),
+			Valu("nilcheck", OpNilCheck, c.config.Types.IntPtr, 0, nil, "p", "mem"),
 			Goto("exit")),
 		Bloc("exit",
 			Exit("mem"),

src/cmd/compile/internal/ssa/nilcheck.go

@@ -38,11 +38,14 @@ func nilcheckelim(f *Func) {
 	work := make([]bp, 0, 256)
 	work = append(work, bp{block: f.Entry})
 
-	// map from value ID to bool indicating if value is known to be non-nil
-	// in the current dominator path being walked. This slice is updated by
+	// map from value ID to known non-nil version of that value ID
+	// (in the current dominator path being walked). This slice is updated by
 	// walkStates to maintain the known non-nil values.
-	nonNilValues := f.Cache.allocBoolSlice(f.NumValues())
-	defer f.Cache.freeBoolSlice(nonNilValues)
+	// If there is extrinsic information about non-nil-ness, this map
+	// points a value to itself. If a value is known non-nil because we
+	// already did a nil check on it, it points to the nil check operation.
+	nonNilValues := f.Cache.allocValueSlice(f.NumValues())
+	defer f.Cache.freeValueSlice(nonNilValues)
 
 	// make an initial pass identifying any non-nil values
 	for _, b := range f.Blocks {
@@ -54,7 +57,7 @@ func nilcheckelim(f *Func) {
 			// We assume that SlicePtr is non-nil because we do a bounds check
 			// before the slice access (and all cap>0 slices have a non-nil ptr). See #30366.
 			if v.Op == OpAddr || v.Op == OpLocalAddr || v.Op == OpAddPtr || v.Op == OpOffPtr || v.Op == OpAdd32 || v.Op == OpAdd64 || v.Op == OpSub32 || v.Op == OpSub64 || v.Op == OpSlicePtr {
-				nonNilValues[v.ID] = true
+				nonNilValues[v.ID] = v
 			}
 		}
 	}
@@ -68,16 +71,16 @@ func nilcheckelim(f *Func) {
 				if v.Op == OpPhi {
 					argsNonNil := true
 					for _, a := range v.Args {
-						if !nonNilValues[a.ID] {
+						if nonNilValues[a.ID] == nil {
 							argsNonNil = false
 							break
 						}
 					}
 					if argsNonNil {
-						if !nonNilValues[v.ID] {
+						if nonNilValues[v.ID] == nil {
 							changed = true
 						}
-						nonNilValues[v.ID] = true
+						nonNilValues[v.ID] = v
 					}
 				}
 			}
@@ -103,8 +106,8 @@ func nilcheckelim(f *Func) {
 			if len(b.Preds) == 1 {
 				p := b.Preds[0].b
 				if p.Kind == BlockIf && p.Controls[0].Op == OpIsNonNil && p.Succs[0].b == b {
-					if ptr := p.Controls[0].Args[0]; !nonNilValues[ptr.ID] {
-						nonNilValues[ptr.ID] = true
+					if ptr := p.Controls[0].Args[0]; nonNilValues[ptr.ID] == nil {
+						nonNilValues[ptr.ID] = ptr
 						work = append(work, bp{op: ClearPtr, ptr: ptr})
 					}
 				}
@@ -117,14 +120,11 @@ func nilcheckelim(f *Func) {
 			pendingLines.clear()
 
 			// Next, process values in the block.
-			i := 0
 			for _, v := range b.Values {
-				b.Values[i] = v
-				i++
 				switch v.Op {
 				case OpIsNonNil:
 					ptr := v.Args[0]
-					if nonNilValues[ptr.ID] {
+					if nonNilValues[ptr.ID] != nil {
 						if v.Pos.IsStmt() == src.PosIsStmt { // Boolean true is a terrible statement boundary.
 							pendingLines.add(v.Pos)
 							v.Pos = v.Pos.WithNotStmt()
@@ -135,7 +135,7 @@ func nilcheckelim(f *Func) {
 					}
 				case OpNilCheck:
 					ptr := v.Args[0]
-					if nonNilValues[ptr.ID] {
+					if nilCheck := nonNilValues[ptr.ID]; nilCheck != nil {
 						// This is a redundant implicit nil check.
 						// Logging in the style of the former compiler -- and omit line 1,
 						// which is usually in generated code.
@@ -145,14 +145,13 @@ func nilcheckelim(f *Func) {
 						if v.Pos.IsStmt() == src.PosIsStmt { // About to lose a statement boundary
 							pendingLines.add(v.Pos)
 						}
-						v.reset(OpUnknown)
-						f.freeValue(v)
-						i--
+						v.Op = OpCopy
+						v.SetArgs1(nilCheck)
 						continue
 					}
 					// Record the fact that we know ptr is non nil, and remember to
 					// undo that information when this dominator subtree is done.
-					nonNilValues[ptr.ID] = true
+					nonNilValues[ptr.ID] = v
 					work = append(work, bp{op: ClearPtr, ptr: ptr})
 					fallthrough // a non-eliminated nil check might be a good place for a statement boundary.
 				default:
@@ -163,7 +162,7 @@ func nilcheckelim(f *Func) {
 				}
 			}
 			// This reduces the lost statement count in "go" by 5 (out of 500 total).
-			for j := 0; j < i; j++ { // is this an ordering problem?
+			for j := range b.Values { // is this an ordering problem?
 				v := b.Values[j]
 				if v.Pos.IsStmt() != src.PosNotStmt && !isPoorStatementOp(v.Op) && pendingLines.contains(v.Pos) {
 					v.Pos = v.Pos.WithIsStmt()
@@ -174,15 +173,14 @@ func nilcheckelim(f *Func) {
 				b.Pos = b.Pos.WithIsStmt()
 				pendingLines.remove(b.Pos)
 			}
-			b.truncateValues(i)
 
 			// Add all dominated blocks to the work list.
 			for w := sdom[node.block.ID].child; w != nil; w = sdom[w.ID].sibling {
 				work = append(work, bp{op: Work, block: w})
 			}
 
 		case ClearPtr:
-			nonNilValues[node.ptr.ID] = false
+			nonNilValues[node.ptr.ID] = nil
 			continue
 		}
 	}

src/cmd/compile/internal/ssa/rewrite.go

@@ -859,6 +859,9 @@ func disjoint(p1 *Value, n1 int64, p2 *Value, n2 int64) bool {
 			offset += base.AuxInt
 			base = base.Args[0]
 		}
+		if opcodeTable[base.Op].nilCheck {
+			base = base.Args[0]
+		}
 		return base, offset
 	}
 	p1, off1 := baseAndOffset(p1)