]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/compile: keep pointer input arguments live throughout function
authorKeith Randall <khr@golang.org>
Fri, 22 Apr 2016 02:28:28 +0000 (19:28 -0700)
committerKeith Randall <khr@golang.org>
Wed, 18 May 2016 19:25:27 +0000 (19:25 +0000)
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 <drchase@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
src/cmd/compile/internal/amd64/ssa.go
src/cmd/compile/internal/gc/plive.go
src/cmd/compile/internal/gc/ssa.go
src/cmd/compile/internal/gc/syntax.go
src/cmd/compile/internal/gc/typecheck.go
src/cmd/compile/internal/ssa/gen/genericOps.go
src/cmd/compile/internal/ssa/lower.go
src/cmd/compile/internal/ssa/opGen.go
src/cmd/compile/internal/ssa/regalloc.go
test/fixedbugs/issue15277.go [new file with mode: 0644]

index 54d878d92bd0afb6a197f28300661a4e97e41d1f..756bcec75c88ee3e641d6ad49e6682235772976e 100644 (file)
@@ -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.
index 87f4a11c0077c33e757f5024ea37c30a99b61a03..cf5359ecdf82ac3102c2db7e80d72abcc5d7ec0d 100644 (file)
@@ -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.
index fdf040d5af93bd719d03ba382df9ceb938828264..7bae8b46728ca43fcbdb1b5dad0ca92afac7031a 100644 (file)
@@ -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 {
index 8a675ac1579881f7efb70eb3bba83e81ec240397..0135061e684bbdb04570616ea49e6fb712a30d71 100644 (file)
@@ -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.
index 7fccbe1a52b945b39d9db5436c7523fb6e69738e..5c23d08cf36cccb0cb1813e6bbd289153c227e72 100644 (file)
@@ -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
index 8ea04c4fe532ef154e5ac588dd847e526b6e0b3a..8388ea8946873045f5bce6ad1fdfa03c1a8c12b0 100644 (file)
@@ -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,7 +407,7 @@ 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"},
@@ -415,6 +415,7 @@ var genericOps = []opData{
        {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
index af0ee4cccf07299354ff4b2d8bed887c8fba2c92..e271ed4ef6b9abd865a2c56dceaa5b889e5937c6 100644 (file)
@@ -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()
index 2795d973336543923f6d276417fe1041c36d89a4..383f1ae5f3d16550b601d86033022fea1ec60fa2 100644 (file)
@@ -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 }
index c9ef0d30174a04c2205d3f772dca26e7b2bf96a4..c05e9ade7791418d04e48e681b775cc010ee4dab 100644 (file)
@@ -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
                        }
 
diff --git a/test/fixedbugs/issue15277.go b/test/fixedbugs/issue15277.go
new file mode 100644 (file)
index 0000000..a3acc61
--- /dev/null
@@ -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)
+}