cmd/compile: keep pointer input arguments live throughout function

Introduce a KeepAlive op which makes sure that its argument is kept
live until the KeepAlive.  Use KeepAlive to mark pointer input
arguments as live after each function call and at each return.

We do this change only for pointer arguments.  Those are the
critical ones to handle because they might have finalizers.
Doing compound arguments (slices, structs, ...) is more complicated
because we would need to track field liveness individually (we do
that for auto variables now, but inputs requires extra trickery).

Turn off the automatic marking of args as live.  That way, when args
are explicitly nulled, plive will know that the original argument is
dead.

The KeepAlive op will be the eventual implementation of
runtime.KeepAlive.

Fixes #15277

Change-Id: I5f223e65d99c9f8342c03fbb1512c4d363e903e5
Reviewed-on: https://go-review.googlesource.com/22365
Reviewed-by: David Chase <[email protected]>
Reviewed-by: Russ Cox <[email protected]>

Author: randall77

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

@@ -878,6 +878,18 @@ func ssaGenValue(s *gc.SSAGenState, v *ssa.Value) {
 		gc.Gvarkill(v.Aux.(*gc.Node))
 	case ssa.OpVarLive:
 		gc.Gvarlive(v.Aux.(*gc.Node))
+	case ssa.OpKeepAlive:
+		if !v.Args[0].Type.IsPtrShaped() {
+			v.Fatalf("keeping non-pointer alive %v", v.Args[0])
+		}
+		n, off := gc.AutoVar(v.Args[0])
+		if n == nil {
+			v.Fatalf("KeepLive with non-spilled value %s %s", v, v.Args[0])
+		}
+		if off != 0 {
+			v.Fatalf("KeepLive with non-zero offset spill location %s:%d", n, off)
+		}
+		gc.Gvarlive(n)
 	case ssa.OpAMD64LoweredNilCheck:
 		// Optimization - if the subsequent block has a load or store
 		// at the same address, we don't need to issue this instruction.

src/cmd/compile/internal/gc/plive.go

@@ -569,7 +569,9 @@ func progeffects(prog *obj.Prog, vars []*Node, uevar bvec, varkill bvec, avarini
 		for i, node := range vars {
 			switch node.Class &^ PHEAP {
 			case PPARAM:
-				bvset(uevar, int32(i))
+				if !node.NotLiveAtEnd() {
+					bvset(uevar, int32(i))
+				}
 
 				// If the result had its address taken, it is being tracked
 			// by the avarinit code, which does not use uevar.
@@ -980,23 +982,6 @@ func onebitlivepointermap(lv *Liveness, liveout bvec, vars []*Node, args bvec, l
 			onebitwalktype1(node.Type, &xoffset, args)
 		}
 	}
-
-	// The node list only contains declared names.
-	// If the receiver or arguments are unnamed, they will be omitted
-	// from the list above. Preserve those values - even though they are unused -
-	// in order to keep their addresses live for use in stack traces.
-	thisargtype := lv.fn.Type.Recvs()
-
-	if thisargtype != nil {
-		xoffset = 0
-		onebitwalktype1(thisargtype, &xoffset, args)
-	}
-
-	inargtype := lv.fn.Type.Params()
-	if inargtype != nil {
-		xoffset = 0
-		onebitwalktype1(inargtype, &xoffset, args)
-	}
 }
 
 // Construct a disembodied instruction.

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

@@ -161,6 +161,10 @@ func buildssa(fn *Node) *ssa.Func {
 				// the function.
 				s.returns = append(s.returns, n)
 			}
+			if n.Class == PPARAM && s.canSSA(n) && n.Type.IsPtrShaped() {
+				s.ptrargs = append(s.ptrargs, n)
+				n.SetNotLiveAtEnd(true) // SSA takes care of this explicitly
+			}
 		case PAUTO | PHEAP:
 			// TODO this looks wrong for PAUTO|PHEAP, no vardef, but also no definition
 			aux := s.lookupSymbol(n, &ssa.AutoSymbol{Typ: n.Type, Node: n})
@@ -293,6 +297,10 @@ type state struct {
 	// list of PPARAMOUT (return) variables. Does not include PPARAM|PHEAP vars.
 	returns []*Node
 
+	// list of PPARAM SSA-able pointer-shaped args. We ensure these are live
+	// throughout the function to help users avoid premature finalizers.
+	ptrargs []*Node
+
 	cgoUnsafeArgs bool
 	noWB          bool
 	WBLineno      int32 // line number of first write barrier. 0=no write barriers
@@ -988,8 +996,7 @@ func (s *state) exit() *ssa.Block {
 
 	// Store SSAable PPARAMOUT variables back to stack locations.
 	for _, n := range s.returns {
-		aux := &ssa.ArgSymbol{Typ: n.Type, Node: n}
-		addr := s.newValue1A(ssa.OpAddr, Ptrto(n.Type), aux, s.sp)
+		addr := s.decladdrs[n]
 		val := s.variable(n, n.Type)
 		s.vars[&memVar] = s.newValue1A(ssa.OpVarDef, ssa.TypeMem, n, s.mem())
 		s.vars[&memVar] = s.newValue3I(ssa.OpStore, ssa.TypeMem, n.Type.Size(), addr, val, s.mem())
@@ -998,6 +1005,16 @@ func (s *state) exit() *ssa.Block {
 		// currently.
 	}
 
+	// Keep input pointer args live until the return. This is a bandaid
+	// fix for 1.7 for what will become in 1.8 explicit runtime.KeepAlive calls.
+	// For <= 1.7 we guarantee that pointer input arguments live to the end of
+	// the function to prevent premature (from the user's point of view)
+	// execution of finalizers. See issue 15277.
+	// TODO: remove for 1.8?
+	for _, n := range s.ptrargs {
+		s.vars[&memVar] = s.newValue2(ssa.OpKeepAlive, ssa.TypeMem, s.variable(n, n.Type), s.mem())
+	}
+
 	// Do actual return.
 	m := s.mem()
 	b := s.endBlock()
@@ -2648,6 +2665,10 @@ func (s *state) call(n *Node, k callKind) *ssa.Value {
 
 	// Start exit block, find address of result.
 	s.startBlock(bNext)
+	// Keep input pointer args live across calls.  This is a bandaid until 1.8.
+	for _, n := range s.ptrargs {
+		s.vars[&memVar] = s.newValue2(ssa.OpKeepAlive, ssa.TypeMem, s.variable(n, n.Type), s.mem())
+	}
 	res := n.Left.Type.Results()
 	if res.NumFields() == 0 || k != callNormal {
 		// call has no return value. Continue with the next statement.
@@ -2997,6 +3018,11 @@ func (s *state) rtcall(fn *Node, returns bool, results []*Type, args ...*ssa.Val
 	b.AddEdgeTo(bNext)
 	s.startBlock(bNext)
 
+	// Keep input pointer args live across calls.  This is a bandaid until 1.8.
+	for _, n := range s.ptrargs {
+		s.vars[&memVar] = s.newValue2(ssa.OpKeepAlive, ssa.TypeMem, s.variable(n, n.Type), s.mem())
+	}
+
 	// Load results
 	res := make([]*ssa.Value, len(results))
 	for i, t := range results {

src/cmd/compile/internal/gc/syntax.go

@@ -68,11 +68,37 @@ type Node struct {
 	Used      bool
 	Isddd     bool // is the argument variadic
 	Implicit  bool
-	Addrtaken bool // address taken, even if not moved to heap
-	Assigned  bool // is the variable ever assigned to
-	Likely    int8 // likeliness of if statement
-	Hasbreak  bool // has break statement
-	hasVal    int8 // +1 for Val, -1 for Opt, 0 for not yet set
+	Addrtaken bool  // address taken, even if not moved to heap
+	Assigned  bool  // is the variable ever assigned to
+	Likely    int8  // likeliness of if statement
+	hasVal    int8  // +1 for Val, -1 for Opt, 0 for not yet set
+	flags     uint8 // TODO: store more bool fields in this flag field
+}
+
+const (
+	hasBreak = 1 << iota
+	notLiveAtEnd
+)
+
+func (n *Node) HasBreak() bool {
+	return n.flags&hasBreak != 0
+}
+func (n *Node) SetHasBreak(b bool) {
+	if b {
+		n.flags |= hasBreak
+	} else {
+		n.flags &^= hasBreak
+	}
+}
+func (n *Node) NotLiveAtEnd() bool {
+	return n.flags&notLiveAtEnd != 0
+}
+func (n *Node) SetNotLiveAtEnd(b bool) {
+	if b {
+		n.flags |= notLiveAtEnd
+	} else {
+		n.flags &^= notLiveAtEnd
+	}
 }
 
 // Val returns the Val for the node.

src/cmd/compile/internal/gc/typecheck.go

@@ -3786,12 +3786,12 @@ func markbreak(n *Node, implicit *Node) {
 	case OBREAK:
 		if n.Left == nil {
 			if implicit != nil {
-				implicit.Hasbreak = true
+				implicit.SetHasBreak(true)
 			}
 		} else {
 			lab := n.Left.Sym.Label
 			if lab != nil {
-				lab.Def.Hasbreak = true
+				lab.Def.SetHasBreak(true)
 			}
 		}
 
@@ -3867,7 +3867,7 @@ func (n *Node) isterminating() bool {
 		if n.Left != nil {
 			return false
 		}
-		if n.Hasbreak {
+		if n.HasBreak() {
 			return false
 		}
 		return true
@@ -3876,7 +3876,7 @@ func (n *Node) isterminating() bool {
 		return n.Nbody.isterminating() && n.Rlist.isterminating()
 
 	case OSWITCH, OTYPESW, OSELECT:
-		if n.Hasbreak {
+		if n.HasBreak() {
 			return false
 		}
 		def := 0

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

@@ -382,9 +382,9 @@ var genericOps = []opData{
 	{name: "ComplexImag", argLength: 1}, // imag(arg0)
 
 	// Strings
-	{name: "StringMake", argLength: 2}, // arg0=ptr, arg1=len
-	{name: "StringPtr", argLength: 1},  // ptr(arg0)
-	{name: "StringLen", argLength: 1},  // len(arg0)
+	{name: "StringMake", argLength: 2},                // arg0=ptr, arg1=len
+	{name: "StringPtr", argLength: 1, typ: "BytePtr"}, // ptr(arg0)
+	{name: "StringLen", argLength: 1, typ: "Int"},     // len(arg0)
 
 	// Interfaces
 	{name: "IMake", argLength: 2},                // arg0=itab, arg1=data
@@ -407,14 +407,15 @@ var genericOps = []opData{
 	{name: "LoadReg", argLength: 1},
 
 	// Used during ssa construction. Like Copy, but the arg has not been specified yet.
-	{name: "FwdRef"},
+	{name: "FwdRef", aux: "Sym"},
 
 	// Unknown value. Used for Values whose values don't matter because they are dead code.
 	{name: "Unknown"},
 
 	{name: "VarDef", argLength: 1, aux: "Sym", typ: "Mem"}, // aux is a *gc.Node of a variable that is about to be initialized.  arg0=mem, returns mem
 	{name: "VarKill", argLength: 1, aux: "Sym"},            // aux is a *gc.Node of a variable that is known to be dead.  arg0=mem, returns mem
 	{name: "VarLive", argLength: 1, aux: "Sym"},            // aux is a *gc.Node of a variable that must be kept live.  arg0=mem, returns mem
+	{name: "KeepAlive", argLength: 2, typ: "Mem"},          // arg[0] is a value that must be kept alive until this mark.  arg[1]=mem, returns mem
 }
 
 //     kind           control    successors       implicit exit

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

@@ -21,7 +21,7 @@ func checkLower(f *Func) {
 				continue // lowered
 			}
 			switch v.Op {
-			case OpSP, OpSB, OpInitMem, OpArg, OpPhi, OpVarDef, OpVarKill, OpVarLive:
+			case OpSP, OpSB, OpInitMem, OpArg, OpPhi, OpVarDef, OpVarKill, OpVarLive, OpKeepAlive:
 				continue // ok not to lower
 			}
 			s := "not lowered: " + v.Op.String() + " " + v.Type.SimpleString()

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

@@ -617,6 +617,7 @@ const (
 	OpVarDef
 	OpVarKill
 	OpVarLive
+	OpKeepAlive
 )
 
 var opcodeTable = [...]opInfo{
@@ -5357,6 +5358,7 @@ var opcodeTable = [...]opInfo{
 	},
 	{
 		name:    "FwdRef",
+		auxType: auxSym,
 		argLen:  0,
 		generic: true,
 	},
@@ -5383,6 +5385,11 @@ var opcodeTable = [...]opInfo{
 		argLen:  1,
 		generic: true,
 	},
+	{
+		name:    "KeepAlive",
+		argLen:  2,
+		generic: true,
+	},
 }
 
 func (o Op) Asm() obj.As    { return opcodeTable[o].asm }

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

@@ -941,11 +941,28 @@ func (s *regAllocState) regalloc(f *Func) {
 				s.advanceUses(v)
 				continue
 			}
+			if v.Op == OpKeepAlive {
+				// Make sure the argument to v is still live here.
+				s.advanceUses(v)
+				vi := &s.values[v.Args[0].ID]
+				if vi.spillUsed {
+					// Use the spill location.
+					v.SetArg(0, vi.spill)
+					b.Values = append(b.Values, v)
+				} else {
+					// No need to keep unspilled values live.
+					// These are typically rematerializeable constants like nil,
+					// or values of a variable that were modified since the last call.
+					v.Args[0].Uses--
+				}
+				continue
+			}
 			regspec := opcodeTable[v.Op].reg
 			if len(regspec.inputs) == 0 && len(regspec.outputs) == 0 {
 				// No register allocation required (or none specified yet)
 				s.freeRegs(regspec.clobbers)
 				b.Values = append(b.Values, v)
+				s.advanceUses(v)
 				continue
 			}
 

test/fixedbugs/issue15277.go

@@ -0,0 +1,37 @@
+// run
+
+// Copyright 2016 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 "runtime"
+
+type big [10 << 20]byte
+
+func f(x *big, start int64) {
+	if delta := inuse() - start; delta < 9<<20 {
+		println("after alloc: expected delta at least 9MB, got: ", delta)
+	}
+	x = nil
+	if delta := inuse() - start; delta > 1<<20 {
+		println("after drop: expected delta below 1MB, got: ", delta)
+	}
+	x = new(big)
+	if delta := inuse() - start; delta < 9<<20 {
+		println("second alloc: expected delta at least 9MB, got: ", delta)
+	}
+}
+
+func main() {
+	x := inuse()
+	f(new(big), x)
+}
+
+func inuse() int64 {
+	runtime.GC()
+	var st runtime.MemStats
+	runtime.ReadMemStats(&st)
+	return int64(st.Alloc)
+}