]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/compile: additional paranoia and checking in plive.go
authorRuss Cox <rsc@golang.org>
Wed, 25 May 2016 14:01:58 +0000 (10:01 -0400)
committerRuss Cox <rsc@golang.org>
Fri, 27 May 2016 14:13:11 +0000 (14:13 +0000)
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 <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
src/cmd/compile/internal/gc/closure.go
src/cmd/compile/internal/gc/dcl.go
src/cmd/compile/internal/gc/fmt.go
src/cmd/compile/internal/gc/gsubr.go
src/cmd/compile/internal/gc/plive.go
src/cmd/compile/internal/gc/type.go
src/cmd/compile/internal/gc/universe.go
src/cmd/compile/internal/gc/walk.go

index 238280f68adb75840fc75ef80c6a30bb93646f8f..29ee981ad91fd5e9285a69eb036b177b09efe05c 100644 (file)
@@ -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
index 3b1822ffd97e3367152d16beff90678440dc92fc..b22b6cdde5f0b763f7d922954795c726ca731687 100644 (file)
@@ -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())
 
index 02d93e2e470e3ab2b645568bef91725ef26c591f..ee88eedcf3efa2d41a84436f91238a13b8501043 100644 (file)
@@ -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)
        }
 
index 8f4da74150034dce69e5effa3a2af2f0c115fddc..4943d9dddeda49eb27f0692d3dfc5dc962f8ffce 100644 (file)
@@ -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
 }
 
index 333cc9786a3b1f42025669cd802d9bec71040852..85138c9fcde140c7f856144eb9bc2fdb0b48814c 100644 (file)
@@ -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.
index 9ae05f7ff1f1a5e5b7af11dd3ac0ffb6168e0fcf..ab13df6eba598118d51ae4bea1739286b2a3f913 100644 (file)
@@ -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 {
index 84df22502fff93b677d3a9b9801ffd0d4e88ccac..b55af7e25a0a7b4ac8fcdd26d5a1f9c0a3db7c6e 100644 (file)
@@ -362,16 +362,16 @@ func lexinit1() {
        // t = interface { Error() string }
 
        rcvr := typ(TSTRUCT)
-       rcvr.StructType().Funarg = true
+       rcvr.StructType().Funarg = FunargRcvr
        field := newField()
        field.Type = Ptrto(typ(TSTRUCT))
        rcvr.SetFields([]*Field{field})
 
        in := typ(TSTRUCT)
-       in.StructType().Funarg = true
+       in.StructType().Funarg = FunargParams
 
        out := typ(TSTRUCT)
-       out.StructType().Funarg = true
+       out.StructType().Funarg = FunargResults
        field = newField()
        field.Type = Types[TSTRING]
        out.SetFields([]*Field{field})
index 30fb170e500cf2983edfb383fc0f1942c99593c1..66eb7e97ac6c6dc0a2d2dfbd4fae6586e9f2ab94 100644 (file)
@@ -2569,16 +2569,16 @@ func vmatch1(l *Node, r *Node) bool {
 // and to copy non-result prameters' values from the stack.
 // If out is true, then code is also produced to zero-initialize their
 // stack memory addresses.
-func paramstoheap(params *Type, out bool) []*Node {
+func paramstoheap(params *Type) []*Node {
        var nn []*Node
        for _, t := range params.Fields().Slice() {
                // For precise stacks, the garbage collector assumes results
                // are always live, so zero them always.
-               if out {
+               if params.StructType().Funarg == FunargResults {
                        // Defer might stop a panic and show the
                        // return values as they exist at the time of panic.
                        // Make sure to zero them on entry to the function.
-                       nn = append(nn, Nod(OAS, nodarg(t, -1), nil))
+                       nn = append(nn, Nod(OAS, nodarg(t, 1), nil))
                }
 
                v := t.Nname
@@ -2623,9 +2623,9 @@ func returnsfromheap(params *Type) []*Node {
 func heapmoves() {
        lno := lineno
        lineno = Curfn.Lineno
-       nn := paramstoheap(Curfn.Type.Recvs(), false)
-       nn = append(nn, paramstoheap(Curfn.Type.Params(), false)...)
-       nn = append(nn, paramstoheap(Curfn.Type.Results(), true)...)
+       nn := paramstoheap(Curfn.Type.Recvs())
+       nn = append(nn, paramstoheap(Curfn.Type.Params())...)
+       nn = append(nn, paramstoheap(Curfn.Type.Results())...)
        Curfn.Func.Enter.Append(nn...)
        lineno = Curfn.Func.Endlineno
        Curfn.Func.Exit.Append(returnsfromheap(Curfn.Type.Results())...)