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 <drchase@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
diff --git a/src/cmd/compile/internal/amd64/ssa.go b/src/cmd/compile/internal/amd64/ssa.go
index 54d878d..756bcec 100644
--- a/src/cmd/compile/internal/amd64/ssa.go
+++ b/src/cmd/compile/internal/amd64/ssa.go
@@ -878,6 +878,18 @@
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.
diff --git a/src/cmd/compile/internal/gc/plive.go b/src/cmd/compile/internal/gc/plive.go
index 87f4a11..cf5359e 100644
--- a/src/cmd/compile/internal/gc/plive.go
+++ b/src/cmd/compile/internal/gc/plive.go
@@ -569,7 +569,9 @@
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 @@
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.
diff --git a/src/cmd/compile/internal/gc/ssa.go b/src/cmd/compile/internal/gc/ssa.go
index fdf040d..7bae8b4 100644
--- a/src/cmd/compile/internal/gc/ssa.go
+++ b/src/cmd/compile/internal/gc/ssa.go
@@ -161,6 +161,10 @@
// 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 @@
// 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 @@
// 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 @@
// 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 @@
// 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 @@
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 {
diff --git a/src/cmd/compile/internal/gc/syntax.go b/src/cmd/compile/internal/gc/syntax.go
index 8a675ac..0135061 100644
--- a/src/cmd/compile/internal/gc/syntax.go
+++ b/src/cmd/compile/internal/gc/syntax.go
@@ -68,11 +68,37 @@
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¬LiveAtEnd != 0
+}
+func (n *Node) SetNotLiveAtEnd(b bool) {
+ if b {
+ n.flags |= notLiveAtEnd
+ } else {
+ n.flags &^= notLiveAtEnd
+ }
}
// Val returns the Val for the node.
diff --git a/src/cmd/compile/internal/gc/typecheck.go b/src/cmd/compile/internal/gc/typecheck.go
index 7fccbe1..5c23d08 100644
--- a/src/cmd/compile/internal/gc/typecheck.go
+++ b/src/cmd/compile/internal/gc/typecheck.go
@@ -3786,12 +3786,12 @@
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 @@
if n.Left != nil {
return false
}
- if n.Hasbreak {
+ if n.HasBreak() {
return false
}
return true
@@ -3876,7 +3876,7 @@
return n.Nbody.isterminating() && n.Rlist.isterminating()
case OSWITCH, OTYPESW, OSELECT:
- if n.Hasbreak {
+ if n.HasBreak() {
return false
}
def := 0
diff --git a/src/cmd/compile/internal/ssa/gen/genericOps.go b/src/cmd/compile/internal/ssa/gen/genericOps.go
index 8ea04c4..8388ea8 100644
--- a/src/cmd/compile/internal/ssa/gen/genericOps.go
+++ b/src/cmd/compile/internal/ssa/gen/genericOps.go
@@ -382,9 +382,9 @@
{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 @@
{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 @@
{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
diff --git a/src/cmd/compile/internal/ssa/lower.go b/src/cmd/compile/internal/ssa/lower.go
index af0ee4c..e271ed4 100644
--- a/src/cmd/compile/internal/ssa/lower.go
+++ b/src/cmd/compile/internal/ssa/lower.go
@@ -21,7 +21,7 @@
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()
diff --git a/src/cmd/compile/internal/ssa/opGen.go b/src/cmd/compile/internal/ssa/opGen.go
index 2795d97..383f1ae5 100644
--- a/src/cmd/compile/internal/ssa/opGen.go
+++ b/src/cmd/compile/internal/ssa/opGen.go
@@ -617,6 +617,7 @@
OpVarDef
OpVarKill
OpVarLive
+ OpKeepAlive
)
var opcodeTable = [...]opInfo{
@@ -5357,6 +5358,7 @@
},
{
name: "FwdRef",
+ auxType: auxSym,
argLen: 0,
generic: true,
},
@@ -5383,6 +5385,11 @@
argLen: 1,
generic: true,
},
+ {
+ name: "KeepAlive",
+ argLen: 2,
+ generic: true,
+ },
}
func (o Op) Asm() obj.As { return opcodeTable[o].asm }
diff --git a/src/cmd/compile/internal/ssa/regalloc.go b/src/cmd/compile/internal/ssa/regalloc.go
index c9ef0d3..c05e9ad 100644
--- a/src/cmd/compile/internal/ssa/regalloc.go
+++ b/src/cmd/compile/internal/ssa/regalloc.go
@@ -941,11 +941,28 @@
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
}
