cmd/compile: additional paranoia and checking in plive.go

The main check here is that liveness now crashes if it finds an instruction
using a variable that should be tracked but is not.

Comments and adjustments in nodarg to explain what's going on and
to remove the "-1" argument added a few months ago, plus a sketch
of a future simplification.

The need for n.Orig in the earlier CL seems to have been an intermediate
problem rather than fundamental: the new explanations in nodarg make
clear that nodarg is not causing the problem I thought, and in fact now
using n instead of n.Orig works fine in plive.go.

Change-Id: I3f5cf9f6e4438a6d27abac7d490e7521545cd552
Reviewed-on: https://go-review.googlesource.com/23450
Run-TryBot: Russ Cox <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: David Chase <[email protected]>

Author: rsc

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

@@ -303,7 +303,7 @@ func transformclosure(xfunc *Node) {
 				continue
 			}
 			fld := newField()
-			fld.Funarg = true
+			fld.Funarg = FunargParams
 			if v.Name.Byval {
 				// If v is captured by value, we merely downgrade it to PPARAM.
 				v.Class = PPARAM

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

@@ -828,14 +828,14 @@ func tostruct0(t *Type, l []*Node) {
 	}
 }
 
-func tofunargs(l []*Node) *Type {
+func tofunargs(l []*Node, funarg Funarg) *Type {
 	t := typ(TSTRUCT)
-	t.StructType().Funarg = true
+	t.StructType().Funarg = funarg
 
 	fields := make([]*Field, len(l))
 	for i, n := range l {
 		f := structfield(n)
-		f.Funarg = true
+		f.Funarg = funarg
 
 		// esc.go needs to find f given a PPARAM to add the tag.
 		if n.Left != nil && n.Left.Class == PPARAM {
@@ -1026,9 +1026,9 @@ func functype0(t *Type, this *Node, in, out []*Node) {
 	if this != nil {
 		rcvr = []*Node{this}
 	}
-	*t.RecvsP() = tofunargs(rcvr)
-	*t.ResultsP() = tofunargs(out)
-	*t.ParamsP() = tofunargs(in)
+	*t.RecvsP() = tofunargs(rcvr, FunargRcvr)
+	*t.ResultsP() = tofunargs(out, FunargResults)
+	*t.ParamsP() = tofunargs(in, FunargParams)
 
 	checkdupfields("argument", t.Recvs(), t.Results(), t.Params())
 

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

@@ -1659,7 +1659,7 @@ func Fldconv(f *Field, flag FmtFlag) string {
 		}
 
 		if s != nil && f.Embedded == 0 {
-			if f.Funarg {
+			if f.Funarg != FunargNone {
 				name = Nconv(f.Nname, 0)
 			} else if flag&FmtLong != 0 {
 				name = sconv(s, FmtShort|FmtByte) // qualify non-exported names (used on structs, not on funarg)
@@ -1692,7 +1692,7 @@ func Fldconv(f *Field, flag FmtFlag) string {
 	// (The escape analysis tags do not apply to func vars.)
 	// But it must not suppress struct field tags.
 	// See golang.org/issue/13777 and golang.org/issue/14331.
-	if flag&FmtShort == 0 && (!fmtbody || !f.Funarg) && f.Note != "" {
+	if flag&FmtShort == 0 && (!fmtbody || f.Funarg == FunargNone) && f.Note != "" {
 		str += " " + strconv.Quote(f.Note)
 	}
 

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

@@ -515,25 +515,36 @@ func newplist() *obj.Plist {
 	return pl
 }
 
-// nodarg does something that depends on the value of
-// fp (this was previously completely undocumented).
+// nodarg returns a Node for the function argument denoted by t,
+// which is either the entire function argument or result struct (t is a  struct *Type)
+// or a specific argument (t is a *Field within a struct *Type).
 //
-// fp=1 corresponds to input args
-// fp=0 corresponds to output args
-// fp=-1 is a special case of output args for a
-// specific call from walk that previously (and
-// incorrectly) passed a 1; the behavior is exactly
-// the same as it is for 1, except that PARAMOUT is
-// generated instead of PARAM.
+// If fp is 0, the node is for use by a caller invoking the given
+// function, preparing the arguments before the call
+// or retrieving the results after the call.
+// In this case, the node will correspond to an outgoing argument
+// slot like 8(SP).
+//
+// If fp is 1, the node is for use by the function itself
+// (the callee), to retrieve its arguments or write its results.
+// In this case the node will be an ONAME with an appropriate
+// type and offset.
 func nodarg(t interface{}, fp int) *Node {
 	var n *Node
 
+	var funarg Funarg
 	switch t := t.(type) {
+	default:
+		Fatalf("bad nodarg %T(%v)", t, t)
+
 	case *Type:
-		// entire argument struct, not just one arg
+		// Entire argument struct, not just one arg
 		if !t.IsFuncArgStruct() {
 			Fatalf("nodarg: bad type %v", t)
 		}
+		funarg = t.StructType().Funarg
+
+		// Build fake variable name for whole arg struct.
 		n = Nod(ONAME, nil, nil)
 		n.Sym = Lookup(".args")
 		n.Type = t
@@ -546,15 +557,43 @@ func nodarg(t interface{}, fp int) *Node {
 		}
 		n.Xoffset = first.Offset
 		n.Addable = true
+
 	case *Field:
-		if fp == 1 || fp == -1 {
+		funarg = t.Funarg
+		if fp == 1 {
+			// NOTE(rsc): This should be using t.Nname directly,
+			// except in the case where t.Nname.Sym is the blank symbol and
+			// so the assignment would be discarded during code generation.
+			// In that case we need to make a new node, and there is no harm
+			// in optimization passes to doing so. But otherwise we should
+			// definitely be using the actual declaration and not a newly built node.
+			// The extra Fatalf checks here are verifying that this is the case,
+			// without changing the actual logic (at time of writing, it's getting
+			// toward time for the Go 1.7 beta).
+			// At some quieter time (assuming we've never seen these Fatalfs happen)
+			// we could change this code to use "expect" directly.
+			expect := t.Nname
+			if expect.isParamHeapCopy() {
+				expect = expect.Name.Param.Stackcopy
+			}
+
 			for _, n := range Curfn.Func.Dcl {
 				if (n.Class == PPARAM || n.Class == PPARAMOUT) && !isblanksym(t.Sym) && n.Sym == t.Sym {
+					if n != expect {
+						Fatalf("nodarg: unexpected node: %v (%p %v) vs %v (%p %v)", n, n, n.Op, t.Nname, t.Nname, t.Nname.Op)
+					}
 					return n
 				}
 			}
+
+			if !isblanksym(expect.Sym) {
+				Fatalf("nodarg: did not find node in dcl list: %v", expect)
+			}
 		}
 
+		// Build fake name for individual variable.
+		// This is safe because if there was a real declared name
+		// we'd have used it above.
 		n = Nod(ONAME, nil, nil)
 		n.Type = t.Type
 		n.Sym = t.Sym
@@ -564,8 +603,6 @@ func nodarg(t interface{}, fp int) *Node {
 		n.Xoffset = t.Offset
 		n.Addable = true
 		n.Orig = t.Nname
-	default:
-		panic("unreachable")
 	}
 
 	// Rewrite argument named _ to __,
@@ -576,23 +613,23 @@ func nodarg(t interface{}, fp int) *Node {
 	}
 
 	switch fp {
-	case 0: // output arg
-		n.Op = OINDREG
+	default:
+		Fatalf("bad fp")
 
+	case 0: // preparing arguments for call
+		n.Op = OINDREG
 		n.Reg = int16(Thearch.REGSP)
 		n.Xoffset += Ctxt.FixedFrameSize()
 
-	case 1: // input arg
+	case 1: // reading arguments inside call
 		n.Class = PPARAM
-
-	case -1: // output arg from paramstoheap
-		n.Class = PPARAMOUT
-
-	case 2: // offset output arg
-		Fatalf("shouldn't be used")
+		if funarg == FunargResults {
+			n.Class = PPARAMOUT
+		}
 	}
 
 	n.Typecheck = 1
+	n.Addrtaken = true // keep optimizers at bay
 	return n
 }
 

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

@@ -197,62 +197,41 @@ func blockany(bb *BasicBlock, f func(*obj.Prog) bool) bool {
 	return false
 }
 
-// Collects and returns a slice of *Nodes for functions arguments and local
-// variables.
-func getvariables(fn *Node) []*Node {
-	var result []*Node
-	for _, ln := range fn.Func.Dcl {
-		if ln.Op == ONAME {
-			switch ln.Class {
-			case PAUTO, PPARAM, PPARAMOUT, PFUNC, PAUTOHEAP:
-				// ok
-			default:
-				Dump("BAD NODE", ln)
-				Fatalf("getvariables")
-			}
+// livenessShouldTrack reports whether the liveness analysis
+// should track the variable n.
+// We don't care about variables that have no pointers,
+// nor do we care about non-local variables,
+// nor do we care about empty structs (handled by the pointer check),
+// nor do we care about the fake PAUTOHEAP variables.
+func livenessShouldTrack(n *Node) bool {
+	return n.Op == ONAME && (n.Class == PAUTO || n.Class == PPARAM || n.Class == PPARAMOUT) && haspointers(n.Type)
+}
 
-			// In order for GODEBUG=gcdead=1 to work, each bitmap needs
-			// to contain information about all variables covered by the bitmap.
-			// For local variables, the bitmap only covers the stkptrsize
-			// bytes in the frame where variables containing pointers live.
-			// For arguments and results, the bitmap covers all variables,
-			// so we must include all the variables, even the ones without
-			// pointers.
-			//
+// getvariables returns the list of on-stack variables that we need to track.
+func getvariables(fn *Node) []*Node {
+	var vars []*Node
+	for _, n := range fn.Func.Dcl {
+		if n.Op == ONAME {
 			// The Node.opt field is available for use by optimization passes.
-			// We use it to hold the index of the node in the variables array, plus 1
-			// (so that 0 means the Node is not in the variables array).
-			// Each pass should clear opt when done, but you never know,
-			// so clear them all ourselves too.
+			// We use it to hold the index of the node in the variables array
+			// (nil means the Node is not in the variables array).
 			// The Node.curfn field is supposed to be set to the current function
 			// already, but for some compiler-introduced names it seems not to be,
 			// so fix that here.
 			// Later, when we want to find the index of a node in the variables list,
-			// we will check that n.curfn == curfn and n.opt > 0. Then n.opt - 1
+			// we will check that n.Curfn == Curfn and n.Opt() != nil. Then n.Opt().(int32)
 			// is the index in the variables list.
-			ln.SetOpt(nil)
-
-			// The compiler doesn't emit initializations for zero-width parameters or results.
-			if ln.Type.Width == 0 {
-				continue
-			}
-
-			ln.Name.Curfn = Curfn
-			switch ln.Class {
-			case PAUTO:
-				if haspointers(ln.Type) {
-					ln.SetOpt(int32(len(result)))
-					result = append(result, ln)
-				}
+			n.SetOpt(nil)
+			n.Name.Curfn = Curfn
+		}
 
-			case PPARAM, PPARAMOUT:
-				ln.SetOpt(int32(len(result)))
-				result = append(result, ln)
-			}
+		if livenessShouldTrack(n) {
+			n.SetOpt(int32(len(vars)))
+			vars = append(vars, n)
 		}
 	}
 
-	return result
+	return vars
 }
 
 // A pretty printer for control flow graphs. Takes a slice of *BasicBlocks.
@@ -617,17 +596,9 @@ func progeffects(prog *obj.Prog, vars []*Node, uevar bvec, varkill bvec, avarini
 
 	if prog.Info.Flags&(LeftRead|LeftWrite|LeftAddr) != 0 {
 		from := &prog.From
-		if from.Node != nil && from.Sym != nil && ((from.Node).(*Node)).Name.Curfn == Curfn {
-			switch ((from.Node).(*Node)).Class {
-			case PAUTO, PPARAM, PPARAMOUT:
-				n := from.Node.(*Node).Orig // orig needed for certain nodarg results
-				pos, ok := n.Opt().(int32) // index in vars
-				if !ok {
-					break
-				}
-				if pos >= int32(len(vars)) || vars[pos] != n {
-					Fatalf("bad bookkeeping in liveness %v %d", Nconv(n, 0), pos)
-				}
+		if from.Node != nil && from.Sym != nil {
+			n := from.Node.(*Node)
+			if pos := liveIndex(n, vars); pos >= 0 {
 				if n.Addrtaken {
 					bvset(avarinit, pos)
 				} else {
@@ -646,17 +617,9 @@ func progeffects(prog *obj.Prog, vars []*Node, uevar bvec, varkill bvec, avarini
 
 	if prog.Info.Flags&(RightRead|RightWrite|RightAddr) != 0 {
 		to := &prog.To
-		if to.Node != nil && to.Sym != nil && ((to.Node).(*Node)).Name.Curfn == Curfn {
-			switch ((to.Node).(*Node)).Class {
-			case PAUTO, PPARAM, PPARAMOUT:
-				n := to.Node.(*Node).Orig // orig needed for certain nodarg results
-				pos, ok := n.Opt().(int32) // index in vars
-				if !ok {
-					return
-				}
-				if pos >= int32(len(vars)) || vars[pos] != n {
-					Fatalf("bad bookkeeping in liveness %v %d", Nconv(n, 0), pos)
-				}
+		if to.Node != nil && to.Sym != nil {
+			n := to.Node.(*Node)
+			if pos := liveIndex(n, vars); pos >= 0 {
 				if n.Addrtaken {
 					if prog.As != obj.AVARKILL {
 						bvset(avarinit, pos)
@@ -687,6 +650,24 @@ func progeffects(prog *obj.Prog, vars []*Node, uevar bvec, varkill bvec, avarini
 	}
 }
 
+// liveIndex returns the index of n in the set of tracked vars.
+// If n is not a tracked var, liveIndex returns -1.
+// If n is not a tracked var but should be tracked, liveIndex crashes.
+func liveIndex(n *Node, vars []*Node) int32 {
+	if n.Name.Curfn != Curfn || !livenessShouldTrack(n) {
+		return -1
+	}
+
+	pos, ok := n.Opt().(int32) // index in vars
+	if !ok {
+		Fatalf("lost track of variable in liveness: %v (%p, %p)", n, n, n.Orig)
+	}
+	if pos >= int32(len(vars)) || vars[pos] != n {
+		Fatalf("bad bookkeeping in liveness: %v (%p, %p)", n, n, n.Orig)
+	}
+	return pos
+}
+
 // Constructs a new liveness structure used to hold the global state of the
 // liveness computation. The cfg argument is a slice of *BasicBlocks and the
 // vars argument is a slice of *Nodes.

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

@@ -223,10 +223,20 @@ type StructType struct {
 	// Map links such structs back to their map type.
 	Map *Type
 
-	Funarg      bool  // whether this struct represents function parameters
-	Haspointers uint8 // 0 unknown, 1 no, 2 yes
+	Funarg      Funarg // type of function arguments for arg struct
+	Haspointers uint8  // 0 unknown, 1 no, 2 yes
 }
 
+// Fnstruct records the kind of function argument
+type Funarg uint8
+
+const (
+	FunargNone    Funarg = iota
+	FunargRcvr           // receiver
+	FunargParams         // input parameters
+	FunargResults        // output results
+)
+
 // StructType returns t's extra struct-specific fields.
 func (t *Type) StructType() *StructType {
 	t.wantEtype(TSTRUCT)
@@ -287,7 +297,7 @@ type SliceType struct {
 type Field struct {
 	Nointerface bool
 	Embedded    uint8 // embedded field
-	Funarg      bool
+	Funarg      Funarg
 	Broke       bool // broken field definition
 	Isddd       bool // field is ... argument
 
@@ -786,7 +796,7 @@ func (t *Type) SetNname(n *Node) {
 
 // IsFuncArgStruct reports whether t is a struct representing function parameters.
 func (t *Type) IsFuncArgStruct() bool {
-	return t.Etype == TSTRUCT && t.Extra.(*StructType).Funarg
+	return t.Etype == TSTRUCT && t.Extra.(*StructType).Funarg != FunargNone
 }
 
 func (t *Type) Methods() *Fields {