cmd/compile: fix liveness computation for heap-escaped parameters

The liveness computation of parameters generally was never
correct, but forcing all parameters to be live throughout the
function covered up that problem. The new SSA back end is
too clever: even though it currently keeps the parameter values live
throughout the function, it may find optimizations that mean
the current values are not written back to the original parameter
stack slots immediately or ever (for example if a parameter is set
to nil, SSA constant propagation may replace all later uses of the
parameter with a constant nil, eliminating the need to write the nil
value back to the stack slot), so the liveness code must now
track the actual operations on the stack slots, exposing these
problems.

One small problem in the handling of arguments is that nodarg
can return ONAME PPARAM nodes with adjusted offsets, so that
there are actually multiple *Node pointers for the same parameter
in the instruction stream. This might be possible to correct, but
not in this CL. For now, we fix this by using n.Orig instead of n
when considering PPARAM and PPARAMOUT nodes.

The major problem in the handling of arguments is general
confusion in the liveness code about the meaning of PPARAM|PHEAP
and PPARAMOUT|PHEAP nodes, especially as contrasted with PAUTO|PHEAP.
The difference between these two is that when a local variable "moves"
to the heap, it's really just allocated there to start with; in contrast,
when an argument moves to the heap, the actual data has to be copied
there from the stack at the beginning of the function, and when a
result "moves" to the heap the value in the heap has to be copied
back to the stack when the function returns
This general confusion is also present in the SSA back end.

The PHEAP bit worked decently when I first introduced it 7 years ago (!)
in 391425a. The back end did nothing sophisticated, and in particular
there was no analysis at all: no escape analysis, no liveness analysis,
and certainly no SSA back end. But the complications caused in the
various downstream consumers suggest that this should be a detail
kept mainly in the front end.

This CL therefore eliminates both the PHEAP bit and even the idea of
"heap variables" from the back ends.

First, it replaces the PPARAM|PHEAP, PPARAMOUT|PHEAP, and PAUTO|PHEAP
variable classes with the single PAUTOHEAP, a pseudo-class indicating
a variable maintained on the heap and available by indirecting a
local variable kept on the stack (a plain PAUTO).

Second, walkexpr replaces all references to PAUTOHEAP variables
with indirections of the corresponding PAUTO variable.
The back ends and the liveness code now just see plain indirected
variables. This may actually produce better code, but the real goal
here is to eliminate these little-used and somewhat suspect code
paths in the back end analyses.

The OPARAM node type goes away too.

A followup CL will do the same to PPARAMREF. I'm not sure that
the back ends (SSA in particular) are handling those right either,
and with the framework established in this CL that change is trivial
and the result clearly more correct.

Fixes #15747.

Change-Id: I2770b1ce3cbc93981bfc7166be66a9da12013d74
Reviewed-on: https://go-review.googlesource.com/23393
Reviewed-by: Keith Randall <[email protected]>
Run-TryBot: Russ Cox <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>

Author: rsc

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

@@ -116,7 +116,7 @@ func ginscmp(op gc.Op, t *gc.Type, n1, n2 *gc.Node, likely int) *obj.Prog {
 		base = n1.Left
 	}
 
-	if base.Op == gc.ONAME && base.Class&gc.PHEAP == 0 || n1.Op == gc.OINDREG {
+	if base.Op == gc.ONAME && base.Class != gc.PAUTOHEAP || n1.Op == gc.OINDREG {
 		r1 = *n1
 	} else {
 		gc.Regalloc(&r1, t, n1)
@@ -229,6 +229,8 @@ func gmove(f *gc.Node, t *gc.Node) {
 
 	switch uint32(ft)<<16 | uint32(tt) {
 	default:
+		gc.Dump("f", f)
+		gc.Dump("t", t)
 		gc.Fatalf("gmove %v -> %v", gc.Tconv(f.Type, gc.FmtLong), gc.Tconv(t.Type, gc.FmtLong))
 
 		/*

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

@@ -55,12 +55,15 @@ func widstruct(errtype *Type, t *Type, o int64, flag int) int64 {
 		}
 		f.Offset = o
 		if f.Nname != nil {
-			// this same stackparam logic is in addrescapes
-			// in typecheck.go.  usually addrescapes runs after
-			// widstruct, in which case we could drop this,
+			// addrescapes has similar code to update these offsets.
+			// Usually addrescapes runs after widstruct,
+			// in which case we could drop this,
 			// but function closure functions are the exception.
-			if f.Nname.Name.Param.Stackparam != nil {
-				f.Nname.Name.Param.Stackparam.Xoffset = o
+			// NOTE(rsc): This comment may be stale.
+			// It's possible the ordering has changed and this is
+			// now the common case. I'm not sure.
+			if f.Nname.Name.Param.Stackcopy != nil {
+				f.Nname.Name.Param.Stackcopy.Xoffset = o
 				f.Nname.Xoffset = 0
 			} else {
 				f.Nname.Xoffset = o

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

@@ -1408,8 +1408,8 @@ func (p *exporter) stmt(n *Node) {
 	switch op := n.Op; op {
 	case ODCL:
 		p.op(ODCL)
-		switch n.Left.Class &^ PHEAP {
-		case PPARAM, PPARAMOUT, PAUTO:
+		switch n.Left.Class {
+		case PPARAM, PPARAMOUT, PAUTO, PAUTOHEAP:
 			// TODO(gri) when is this not PAUTO?
 			// Also, originally this didn't look like
 			// the default case. Investigate.

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

@@ -519,7 +519,7 @@ func cgen_wb(n, res *Node, wb bool) {
 		ODOTPTR,
 		OINDEX,
 		OIND,
-		ONAME: // PHEAP or PPARAMREF var
+		ONAME: // PPARAMREF var
 		var n1 Node
 		Igen(n, &n1, res)
 
@@ -1579,7 +1579,7 @@ func Agen(n *Node, res *Node) {
 		}
 
 		// should only get here for heap vars or paramref
-		if n.Class&PHEAP == 0 && n.Class != PPARAMREF {
+		if n.Class != PPARAMREF {
 			Dump("bad agen", n)
 			Fatalf("agen: bad ONAME class %#x", n.Class)
 		}
@@ -1646,7 +1646,7 @@ func Igen(n *Node, a *Node, res *Node) {
 
 	switch n.Op {
 	case ONAME:
-		if (n.Class&PHEAP != 0) || n.Class == PPARAMREF {
+		if n.Class == PPARAMREF {
 			break
 		}
 		*a = *n

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

@@ -405,7 +405,7 @@ func Complexgen(n *Node, res *Node) {
 		ODOTPTR,
 		OINDEX,
 		OIND,
-		ONAME, // PHEAP or PPARAMREF var
+		ONAME, // PPARAMREF var
 		OCALLFUNC,
 		OCALLMETH,
 		OCALLINTER:

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

@@ -1068,7 +1068,6 @@ func escassign(e *EscState, dst, src *Node, step *EscStep) {
 		OIND,    // dst = *x
 		ODOTPTR, // dst = (*x).f
 		ONAME,
-		OPARAM,
 		ODDDARG,
 		OPTRLIT,
 		OARRAYLIT,
@@ -1835,20 +1834,20 @@ func escwalkBody(e *EscState, level Level, dst *Node, src *Node, step *EscStep,
 		}
 		if leaks {
 			src.Esc = EscHeap
-			addrescapes(src.Left)
 			if Debug['m'] != 0 && osrcesc != src.Esc {
 				p := src
 				if p.Left.Op == OCLOSURE {
 					p = p.Left // merely to satisfy error messages in tests
 				}
 				if Debug['m'] > 2 {
-					Warnl(src.Lineno, "%v escapes to heap, level=%v, dst.eld=%v, src.eld=%v",
-						Nconv(p, FmtShort), level, dstE.Escloopdepth, modSrcLoopdepth)
+					Warnl(src.Lineno, "%v escapes to heap, level=%v, dst=%v dst.eld=%v, src.eld=%v",
+						Nconv(p, FmtShort), level, dst, dstE.Escloopdepth, modSrcLoopdepth)
 				} else {
 					Warnl(src.Lineno, "%v escapes to heap", Nconv(p, FmtShort))
 					step.describe(src)
 				}
 			}
+			addrescapes(src.Left)
 			escwalkBody(e, level.dec(), dst, src.Left, e.stepWalk(dst, src.Left, why, step), modSrcLoopdepth)
 			extraloopdepth = modSrcLoopdepth // passes to recursive case, seems likely a no-op
 		} else {

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

@@ -121,7 +121,7 @@ func reexportdep(n *Node) {
 	//print("reexportdep %+hN\n", n);
 	switch n.Op {
 	case ONAME:
-		switch n.Class &^ PHEAP {
+		switch n.Class {
 		// methods will be printed along with their type
 		// nodes for T.Method expressions
 		case PFUNC:

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

@@ -218,6 +218,7 @@ var classnames = []string{
 	"Pxxx",
 	"PEXTERN",
 	"PAUTO",
+	"PAUTOHEAP",
 	"PPARAM",
 	"PPARAMOUT",
 	"PPARAMREF",
@@ -251,14 +252,10 @@ func jconv(n *Node, flag FmtFlag) string {
 	}
 
 	if n.Class != 0 {
-		s := ""
-		if n.Class&PHEAP != 0 {
-			s = ",heap"
-		}
-		if int(n.Class&^PHEAP) < len(classnames) {
-			fmt.Fprintf(&buf, " class(%s%s)", classnames[n.Class&^PHEAP], s)
+		if int(n.Class) < len(classnames) {
+			fmt.Fprintf(&buf, " class(%s)", classnames[n.Class])
 		} else {
-			fmt.Fprintf(&buf, " class(%d?%s)", n.Class&^PHEAP, s)
+			fmt.Fprintf(&buf, " class(%d?)", n.Class)
 		}
 	}
 
@@ -798,8 +795,8 @@ func stmtfmt(n *Node) string {
 	switch n.Op {
 	case ODCL:
 		if fmtmode == FExp {
-			switch n.Left.Class &^ PHEAP {
-			case PPARAM, PPARAMOUT, PAUTO:
+			switch n.Left.Class {
+			case PPARAM, PPARAMOUT, PAUTO, PAUTOHEAP:
 				f += fmt.Sprintf("var %v %v", n.Left, n.Left.Type)
 				goto ret
 			}

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

@@ -43,52 +43,39 @@ func addrescapes(n *Node) {
 			break
 		}
 
-		switch n.Class {
-		case PPARAMREF:
+		// A PPARAMREF is a closure reference.
+		// Mark the thing it refers to as escaping.
+		if n.Class == PPARAMREF {
 			addrescapes(n.Name.Defn)
+			break
+		}
 
-		// if func param, need separate temporary
-		// to hold heap pointer.
-		// the function type has already been checked
-		// (we're in the function body)
-		// so the param already has a valid xoffset.
-
-		// expression to refer to stack copy
-		case PPARAM, PPARAMOUT:
-			n.Name.Param.Stackparam = Nod(OPARAM, n, nil)
-
-			n.Name.Param.Stackparam.Type = n.Type
-			n.Name.Param.Stackparam.Addable = true
-			if n.Xoffset == BADWIDTH {
-				Fatalf("addrescapes before param assignment")
-			}
-			n.Name.Param.Stackparam.Xoffset = n.Xoffset
-			fallthrough
-
-		case PAUTO:
-			n.Class |= PHEAP
-
-			n.Addable = false
-			n.Ullman = 2
-			n.Xoffset = 0
-
-			// create stack variable to hold pointer to heap
-			oldfn := Curfn
+		if n.Class != PPARAM && n.Class != PPARAMOUT && n.Class != PAUTO {
+			break
+		}
 
-			Curfn = n.Name.Curfn
-			if Curfn.Func.Closure != nil && Curfn.Op == OCLOSURE {
-				Curfn = Curfn.Func.Closure
-			}
-			n.Name.Heapaddr = temp(Ptrto(n.Type))
-			buf := fmt.Sprintf("&%v", n.Sym)
-			n.Name.Heapaddr.Sym = Lookup(buf)
-			n.Name.Heapaddr.Orig.Sym = n.Name.Heapaddr.Sym
-			n.Esc = EscHeap
-			if Debug['m'] != 0 {
-				fmt.Printf("%v: moved to heap: %v\n", n.Line(), n)
-			}
-			Curfn = oldfn
+		// This is a plain parameter or local variable that needs to move to the heap,
+		// but possibly for the function outside the one we're compiling.
+		// That is, if we have:
+		//
+		//	func f(x int) {
+		//		func() {
+		//			global = &x
+		//		}
+		//	}
+		//
+		// then we're analyzing the inner closure but we need to move x to the
+		// heap in f, not in the inner closure. Flip over to f before calling moveToHeap.
+		oldfn := Curfn
+		Curfn = n.Name.Curfn
+		if Curfn.Func.Closure != nil && Curfn.Op == OCLOSURE {
+			Curfn = Curfn.Func.Closure
 		}
+		ln := lineno
+		lineno = Curfn.Lineno
+		moveToHeap(n)
+		Curfn = oldfn
+		lineno = ln
 
 	case OIND, ODOTPTR:
 		break
@@ -105,6 +92,110 @@ func addrescapes(n *Node) {
 	}
 }
 
+// isParamStackCopy reports whether this is the on-stack copy of a
+// function parameter that moved to the heap.
+func (n *Node) isParamStackCopy() bool {
+	return n.Op == ONAME && (n.Class == PPARAM || n.Class == PPARAMOUT) && n.Name.Heapaddr != nil
+}
+
+// isParamHeapCopy reports whether this is the on-heap copy of
+// a function parameter that moved to the heap.
+func (n *Node) isParamHeapCopy() bool {
+	return n.Op == ONAME && n.Class == PAUTOHEAP && n.Name.Param.Stackcopy != nil
+}
+
+// paramClass reports the parameter class (PPARAM or PPARAMOUT)
+// of the node, which may be an unmoved on-stack parameter
+// or the on-heap or on-stack copy of a parameter that moved to the heap.
+// If the node is not a parameter, paramClass returns Pxxx.
+func (n *Node) paramClass() Class {
+	if n.Op != ONAME {
+		return Pxxx
+	}
+	if n.Class == PPARAM || n.Class == PPARAMOUT {
+		return n.Class
+	}
+	if n.isParamHeapCopy() {
+		return n.Name.Param.Stackcopy.Class
+	}
+	return Pxxx
+}
+
+// moveToHeap records the parameter or local variable n as moved to the heap.
+func moveToHeap(n *Node) {
+	if Debug['r'] != 0 {
+		Dump("MOVE", n)
+	}
+	if compiling_runtime {
+		Yyerror("%v escapes to heap, not allowed in runtime.", n)
+	}
+	if n.Class == PAUTOHEAP {
+		Dump("n", n)
+		Fatalf("double move to heap")
+	}
+
+	// Allocate a local stack variable to hold the pointer to the heap copy.
+	// temp will add it to the function declaration list automatically.
+	heapaddr := temp(Ptrto(n.Type))
+	heapaddr.Sym = Lookup("&" + n.Sym.Name)
+	heapaddr.Orig.Sym = heapaddr.Sym
+
+	// Parameters have a local stack copy used at function start/end
+	// in addition to the copy in the heap that may live longer than
+	// the function.
+	if n.Class == PPARAM || n.Class == PPARAMOUT {
+		if n.Xoffset == BADWIDTH {
+			Fatalf("addrescapes before param assignment")
+		}
+
+		// We rewrite n below to be a heap variable (indirection of heapaddr).
+		// Preserve a copy so we can still write code referring to the original,
+		// and substitute that copy into the function declaration list
+		// so that analyses of the local (on-stack) variables use it.
+		stackcopy := Nod(ONAME, nil, nil)
+		stackcopy.Sym = n.Sym
+		stackcopy.Type = n.Type
+		stackcopy.Xoffset = n.Xoffset
+		stackcopy.Class = n.Class
+		stackcopy.Name.Heapaddr = heapaddr
+		if n.Class == PPARAM {
+			stackcopy.SetNotLiveAtEnd(true)
+		}
+		n.Name.Param.Stackcopy = stackcopy
+
+		// Substitute the stackcopy into the function variable list so that
+		// liveness and other analyses use the underlying stack slot
+		// and not the now-pseudo-variable n.
+		found := false
+		for i, d := range Curfn.Func.Dcl {
+			if d == n {
+				Curfn.Func.Dcl[i] = stackcopy
+				found = true
+				break
+			}
+			// Parameters are before locals, so can stop early.
+			// This limits the search even in functions with many local variables.
+			if d.Class == PAUTO {
+				break
+			}
+		}
+		if !found {
+			Fatalf("cannot find %v in local variable list", n)
+		}
+		Curfn.Func.Dcl = append(Curfn.Func.Dcl, n)
+	}
+
+	// Modify n in place so that uses of n now mean indirection of the heapaddr.
+	n.Class = PAUTOHEAP
+	n.Ullman = 2
+	n.Xoffset = 0
+	n.Name.Heapaddr = heapaddr
+	n.Esc = EscHeap
+	if Debug['m'] != 0 {
+		fmt.Printf("%v: moved to heap: %v\n", n.Line(), n)
+	}
+}
+
 func clearlabels() {
 	for _, l := range labellist {
 		l.Sym.Label = nil
@@ -243,16 +334,9 @@ func cgen_dcl(n *Node) {
 		Fatalf("cgen_dcl")
 	}
 
-	if n.Class&PHEAP == 0 {
-		return
+	if n.Class == PAUTOHEAP {
+		Fatalf("cgen_dcl %v", n)
 	}
-	if compiling_runtime {
-		Yyerror("%v escapes to heap, not allowed in runtime.", n)
-	}
-	if prealloc[n] == nil {
-		prealloc[n] = callnew(n.Type)
-	}
-	Cgen_as(n.Name.Heapaddr, prealloc[n])
 }
 
 // generate discard of value
@@ -263,7 +347,7 @@ func cgen_discard(nr *Node) {
 
 	switch nr.Op {
 	case ONAME:
-		if nr.Class&PHEAP == 0 && nr.Class != PEXTERN && nr.Class != PFUNC && nr.Class != PPARAMREF {
+		if nr.Class != PAUTOHEAP && nr.Class != PEXTERN && nr.Class != PFUNC && nr.Class != PPARAMREF {
 			gused(nr)
 		}
 
@@ -908,11 +992,6 @@ func Cgen_as_wb(nl, nr *Node, wb bool) {
 	}
 
 	if nr == nil || iszero(nr) {
-		// heaps should already be clear
-		if nr == nil && (nl.Class&PHEAP != 0) {
-			return
-		}
-
 		tl := nl.Type
 		if tl == nil {
 			return