]> Cypherpunks.ru repositories - gostls13.git/commitdiff
cmd/compile: recognize Syscall-like functions for liveness analysis
authorRuss Cox <rsc@golang.org>
Wed, 13 Jan 2016 05:46:28 +0000 (00:46 -0500)
committerRuss Cox <rsc@golang.org>
Thu, 14 Jan 2016 01:16:45 +0000 (01:16 +0000)
Consider this code:

func f(*int)

func g() {
p := new(int)
f(p)
}

where f is an assembly function.
In general liveness analysis assumes that during the call to f, p is dead
in this frame. If f has retained p, p will be found alive in f's frame and keep
the new(int) from being garbage collected. This is all correct and works.
We use the Go func declaration for f to give the assembly function
liveness information (the arguments are assumed live for the entire call).

Now consider this code:

func h1() {
p := new(int)
syscall.Syscall(1, 2, 3, uintptr(unsafe.Pointer(p)))
}

Here syscall.Syscall is taking the place of f, but because its arguments
are uintptr, the liveness analysis and the garbage collector ignore them.
Since p is no longer live in h once the call starts, if the garbage collector
scans the stack while the system call is blocked, it will find no reference
to the new(int) and reclaim it. If the kernel is going to write to *p once
the call finishes, reclaiming the memory is a mistake.

We can't change the arguments or the liveness information for
syscall.Syscall itself, both for compatibility and because sometimes the
arguments really are integers, and the garbage collector will get quite upset
if it finds an integer where it expects a pointer. The problem is that
these arguments are fundamentally untyped.

The solution we have taken in the syscall package's wrappers in past
releases is to insert a call to a dummy function named "use", to make
it look like the argument is live during the call to syscall.Syscall:

func h2() {
p := new(int)
syscall.Syscall(1, 2, 3, uintptr(unsafe.Pointer(p)))
use(unsafe.Pointer(p))
}

Keeping p alive during the call means that if the garbage collector
scans the stack during the system call now, it will find the reference to p.

Unfortunately, this approach is not available to users outside syscall,
because 'use' is unexported, and people also have to realize they need
to use it and do so. There is much existing code using syscall.Syscall
without a 'use'-like function. That code will fail very occasionally in
mysterious ways (see #13372).

This CL fixes all that existing code by making the compiler do the right
thing automatically, without any code modifications. That is, it takes h1
above, which is incorrect code today, and makes it correct code.

Specifically, if the compiler sees a foreign func definition (one
without a body) that has uintptr arguments, it marks those arguments
as "unsafe uintptrs". If it later sees the function being called
with uintptr(unsafe.Pointer(x)) as an argument, it arranges to mark x
as having escaped, and it makes sure to hold x in a live temporary
variable until the call returns, so that the garbage collector cannot
reclaim whatever heap memory x points to.

For now I am leaving the explicit calls to use in package syscall,
but they can be removed early in a future cycle (likely Go 1.7).

The rule has no effect on escape analysis, only on liveness analysis.

Fixes #13372.

Change-Id: I2addb83f70d08db08c64d394f9d06ff0a063c500
Reviewed-on: https://go-review.googlesource.com/18584
Reviewed-by: Ian Lance Taylor <iant@golang.org>
22 files changed:
doc/asm.html
src/cmd/compile/internal/amd64/prog.go
src/cmd/compile/internal/arm/prog.go
src/cmd/compile/internal/arm64/prog.go
src/cmd/compile/internal/gc/esc.go
src/cmd/compile/internal/gc/gen.go
src/cmd/compile/internal/gc/gsubr.go
src/cmd/compile/internal/gc/order.go
src/cmd/compile/internal/gc/pgen.go
src/cmd/compile/internal/gc/plive.go
src/cmd/compile/internal/gc/racewalk.go
src/cmd/compile/internal/gc/reg.go
src/cmd/compile/internal/gc/syntax.go
src/cmd/compile/internal/gc/typecheck.go
src/cmd/compile/internal/gc/walk.go
src/cmd/compile/internal/mips64/prog.go
src/cmd/compile/internal/ppc64/prog.go
src/cmd/compile/internal/x86/prog.go
src/cmd/internal/obj/link.go
src/cmd/internal/obj/util.go
src/syscall/syscall.go
test/live_syscall.go [new file with mode: 0644]

index 3459033f820fa7873f70879b05069f5124b2d766..2af2005143f65b09db5801983a129a6afec2631c 100644 (file)
@@ -510,6 +510,13 @@ the stack pointer may change during any function call:
 even pointers to stack data must not be kept in local variables.
 </p>
 
+<p>
+Assembly functions should always be given Go prototypes,
+both to provide pointer information for the arguments and results
+and to let <code>go</code> <code>vet</code> check that
+the offsets being used to access them are correct.
+</p>
+
 <h2 id="architectures">Architecture-specific details</h2>
 
 <p>
index 649b7062452ec90aa74461947a3f6a9071e57a04..b3724b4dd4993656e42422f81038a1b0fa5ca97e 100644 (file)
@@ -34,6 +34,7 @@ var progtable = [x86.ALAST]obj.ProgInfo{
        obj.ACHECKNIL: {Flags: gc.LeftRead},
        obj.AVARDEF:   {Flags: gc.Pseudo | gc.RightWrite},
        obj.AVARKILL:  {Flags: gc.Pseudo | gc.RightWrite},
+       obj.AVARLIVE:  {Flags: gc.Pseudo | gc.LeftRead},
 
        // NOP is an internal no-op that also stands
        // for USED and SET annotations, not the Intel opcode.
index 8a304e28937adffb77130832918ed817fd37b29b..81be77a5b09bb9b22d4bc4d068b85c8a65700d58 100644 (file)
@@ -33,6 +33,7 @@ var progtable = [arm.ALAST]obj.ProgInfo{
        obj.ACHECKNIL: {Flags: gc.LeftRead},
        obj.AVARDEF:   {Flags: gc.Pseudo | gc.RightWrite},
        obj.AVARKILL:  {Flags: gc.Pseudo | gc.RightWrite},
+       obj.AVARLIVE:  {Flags: gc.Pseudo | gc.LeftRead},
 
        // NOP is an internal no-op that also stands
        // for USED and SET annotations, not the Intel opcode.
index a4b8ebea723bf77b661dfdffe09b11f385f1137c..a8e8bc5f954aac5d1f2eb2fc2929215f85875fac 100644 (file)
@@ -34,6 +34,7 @@ var progtable = [arm64.ALAST]obj.ProgInfo{
        obj.ACHECKNIL: {Flags: gc.LeftRead},
        obj.AVARDEF:   {Flags: gc.Pseudo | gc.RightWrite},
        obj.AVARKILL:  {Flags: gc.Pseudo | gc.RightWrite},
+       obj.AVARLIVE:  {Flags: gc.Pseudo | gc.LeftRead},
 
        // NOP is an internal no-op that also stands
        // for USED and SET annotations, not the Power opcode.
index 7855db280bd39e770af578645b926172c51e921c..ff983e717edc722e3d2a9e4380d862277496f640 100644 (file)
@@ -1808,6 +1808,13 @@ recurse:
        e.pdepth--
 }
 
+// This special tag is applied to uintptr variables
+// that we believe may hold unsafe.Pointers for
+// calls into assembly functions.
+// It is logically a constant, but using a var
+// lets us take the address below to get a *string.
+var unsafeUintptrTag = "unsafe-uintptr"
+
 func esctag(e *EscState, func_ *Node) {
        func_.Esc = EscFuncTagged
 
@@ -1822,6 +1829,29 @@ func esctag(e *EscState, func_ *Node) {
                        }
                }
 
+               // Assume that uintptr arguments must be held live across the call.
+               // This is most important for syscall.Syscall.
+               // See golang.org/issue/13372.
+               // This really doesn't have much to do with escape analysis per se,
+               // but we are reusing the ability to annotate an individual function
+               // argument and pass those annotations along to importing code.
+               narg := 0
+               for t := getinargx(func_.Type).Type; t != nil; t = t.Down {
+                       narg++
+                       if t.Type.Etype == TUINTPTR {
+                               if Debug['m'] != 0 {
+                                       var name string
+                                       if t.Sym != nil {
+                                               name = t.Sym.Name
+                                       } else {
+                                               name = fmt.Sprintf("arg#%d", narg)
+                                       }
+                                       Warnl(int(func_.Lineno), "%v assuming %v is unsafe uintptr", funcSym(func_), name)
+                               }
+                               t.Note = &unsafeUintptrTag
+                       }
+               }
+
                return
        }
 
index 27737b7b7a5fe773140cbf4ef7685b20872e18f2..377aee8a1cf237f4d150bc7866afef44dd7cd65d 100644 (file)
@@ -605,6 +605,9 @@ func Tempname(nn *Node, t *Type) {
        n.Esc = EscNever
        n.Name.Curfn = Curfn
        Curfn.Func.Dcl = list(Curfn.Func.Dcl, n)
+       if Debug['h'] != 0 {
+               println("H", n, n.Orig, funcSym(Curfn).Name)
+       }
 
        dowidth(t)
        n.Xoffset = 0
@@ -868,6 +871,9 @@ func gen(n *Node) {
 
        case OVARKILL:
                gvarkill(n.Left)
+
+       case OVARLIVE:
+               gvarlive(n.Left)
        }
 
 ret:
index 14d4d3da8f6faa90e2b2311d0dc97613e9801270..30bf736e3eb961e97e2c4685e734a63d8c049a83 100644 (file)
@@ -185,7 +185,7 @@ func fixautoused(p *obj.Prog) {
                        continue
                }
 
-               if (p.As == obj.AVARDEF || p.As == obj.AVARKILL) && p.To.Node != nil && !((p.To.Node).(*Node)).Used {
+               if (p.As == obj.AVARDEF || p.As == obj.AVARKILL || p.As == obj.AVARLIVE) && p.To.Node != nil && !((p.To.Node).(*Node)).Used {
                        // Cannot remove VARDEF instruction, because - unlike TYPE handled above -
                        // VARDEFs are interspersed with other code, and a jump might be using the
                        // VARDEF as a target. Replace with a no-op instead. A later pass will remove
index 84b96c2d7b921a804a656ed529bbda3270a36ab1..a2e12284d064817c476f897108f933f5375c2d2a 100644 (file)
@@ -243,6 +243,12 @@ func cleantempnopop(mark *NodeList, order *Order, out **NodeList) {
        var kill *Node
 
        for l := order.temp; l != mark; l = l.Next {
+               if l.N.Name.Keepalive {
+                       l.N.Name.Keepalive = false
+                       kill = Nod(OVARLIVE, l.N, nil)
+                       typecheck(&kill, Etop)
+                       *out = list(*out, kill)
+               }
                kill = Nod(OVARKILL, l.N, nil)
                typecheck(&kill, Etop)
                *out = list(*out, kill)
@@ -375,6 +381,28 @@ func ordercall(n *Node, order *Order) {
        orderexpr(&n.Left, order, nil)
        orderexpr(&n.Right, order, nil) // ODDDARG temp
        ordercallargs(&n.List, order)
+
+       if n.Op == OCALLFUNC {
+               for l, t := n.List, getinargx(n.Left.Type).Type; l != nil && t != nil; l, t = l.Next, t.Down {
+                       // Check for "unsafe-uintptr" tag provided by escape analysis.
+                       // If present and the argument is really a pointer being converted
+                       // to uintptr, arrange for the pointer to be kept alive until the call
+                       // returns, by copying it into a temp and marking that temp
+                       // still alive when we pop the temp stack.
+                       if t.Note != nil && *t.Note == unsafeUintptrTag {
+                               xp := &l.N
+                               for (*xp).Op == OCONVNOP && !Isptr[(*xp).Type.Etype] {
+                                       xp = &(*xp).Left
+                               }
+                               x := *xp
+                               if Isptr[x.Type.Etype] {
+                                       x = ordercopyexpr(x, x.Type, order, 0)
+                                       x.Name.Keepalive = true
+                                       *xp = x
+                               }
+                       }
+               }
+       }
 }
 
 // Ordermapassign appends n to order->out, introducing temporaries
@@ -464,7 +492,7 @@ func orderstmt(n *Node, order *Order) {
        default:
                Fatalf("orderstmt %v", Oconv(int(n.Op), 0))
 
-       case OVARKILL:
+       case OVARKILL, OVARLIVE:
                order.out = list(order.out, n)
 
        case OAS:
index ea9b3687e121a0b70a1089d670c8be345a7721ce..ffc0ab9cfb81824845c41ea7f01f41b4a93a7124 100644 (file)
@@ -94,7 +94,11 @@ func gvardefx(n *Node, as int) {
 
        switch n.Class {
        case PAUTO, PPARAM, PPARAMOUT:
-               Thearch.Gins(as, nil, n)
+               if as == obj.AVARLIVE {
+                       Thearch.Gins(as, n, nil)
+               } else {
+                       Thearch.Gins(as, nil, n)
+               }
        }
 }
 
@@ -106,13 +110,17 @@ func gvarkill(n *Node) {
        gvardefx(n, obj.AVARKILL)
 }
 
+func gvarlive(n *Node) {
+       gvardefx(n, obj.AVARLIVE)
+}
+
 func removevardef(firstp *obj.Prog) {
        for p := firstp; p != nil; p = p.Link {
-               for p.Link != nil && (p.Link.As == obj.AVARDEF || p.Link.As == obj.AVARKILL) {
+               for p.Link != nil && (p.Link.As == obj.AVARDEF || p.Link.As == obj.AVARKILL || p.Link.As == obj.AVARLIVE) {
                        p.Link = p.Link.Link
                }
                if p.To.Type == obj.TYPE_BRANCH {
-                       for p.To.Val.(*obj.Prog) != nil && (p.To.Val.(*obj.Prog).As == obj.AVARDEF || p.To.Val.(*obj.Prog).As == obj.AVARKILL) {
+                       for p.To.Val.(*obj.Prog) != nil && (p.To.Val.(*obj.Prog).As == obj.AVARDEF || p.To.Val.(*obj.Prog).As == obj.AVARKILL || p.To.Val.(*obj.Prog).As == obj.AVARLIVE) {
                                p.To.Val = p.To.Val.(*obj.Prog).Link
                        }
                }
index 5af78d17bd9a4ff46a2466837a119013516e5699..feb66f625a550dfd88bac3edc829de2b8b7ca48f 100644 (file)
@@ -806,7 +806,7 @@ func checkauto(fn *Node, p *obj.Prog, n *Node) {
                return
        }
 
-       fmt.Printf("checkauto %v: %v (%p; class=%d) not found in %v\n", Curfn, n, n, n.Class, p)
+       fmt.Printf("checkauto %v: %v (%p; class=%d) not found in %p %v\n", funcSym(Curfn), n, n, n.Class, p, p)
        for l := fn.Func.Dcl; l != nil; l = l.Next {
                fmt.Printf("\t%v (%p; class=%d)\n", l.N, l.N, l.N.Class)
        }
index ec94042562053c534d6131483ff8b28fc3ab2ce8..8a6eba3964752c75d0a5e0c47458c7beffaf6060 100644 (file)
@@ -143,7 +143,7 @@ func instrumentnode(np **Node, init **NodeList, wr int, skip int) {
                goto ret
 
                // can't matter
-       case OCFUNC, OVARKILL:
+       case OCFUNC, OVARKILL, OVARLIVE:
                goto ret
 
        case OBLOCK:
index f575094389cf71569b03d12f0308cf1bf407bbe2..14dc03b5f5d16f146cf691ac94a76290b94d1b1f 100644 (file)
@@ -1073,6 +1073,9 @@ func regopt(firstp *obj.Prog) {
 
        for f := firstf; f != nil; f = f.Link {
                p := f.Prog
+               // AVARLIVE must be considered a use, do not skip it.
+               // Otherwise the variable will be optimized away,
+               // and the whole point of AVARLIVE is to keep it on the stack.
                if p.As == obj.AVARDEF || p.As == obj.AVARKILL {
                        continue
                }
index 993e2ae04868c769577d747e77e65dcae7a7ef05..a11b37e2ad083153099493c7b0b163cde5951f50 100644 (file)
@@ -128,6 +128,7 @@ type Name struct {
        Captured  bool // is the variable captured by a closure
        Byval     bool // is the variable captured by value or by reference
        Needzero  bool // if it contains pointers, needs to be zeroed on function entry
+       Keepalive bool // mark value live across unknown assembly call
 }
 
 type Param struct {
@@ -342,6 +343,7 @@ const (
        OCFUNC      // reference to c function pointer (not go func value)
        OCHECKNIL   // emit code to ensure pointer/interface not nil
        OVARKILL    // variable is dead
+       OVARLIVE    // variable is alive
 
        // thearch-specific registers
        OREGISTER // a register, such as AX.
index 8c1305f7f463569f05f97499c3065b5afd43cdbc..f74bb334aabf9271f68a4c89fd89292c2cdc8a72 100644 (file)
@@ -2023,7 +2023,8 @@ OpSwitch:
                OEMPTY,
                OGOTO,
                OXFALL,
-               OVARKILL:
+               OVARKILL,
+               OVARLIVE:
                ok |= Etop
                break OpSwitch
 
index 25cd828b9bac300a554035c44990421371c25ee1..e0083175620a1ff05ee2d85e1166e94b825edb7e 100644 (file)
@@ -216,7 +216,8 @@ func walkstmt(np **Node) {
                ODCLCONST,
                ODCLTYPE,
                OCHECKNIL,
-               OVARKILL:
+               OVARKILL,
+               OVARLIVE:
                break
 
        case OBLOCK:
index bf13d82a373c873f0927c9c28711813b9cafc99d..b07c7fe29f1c1a8303fd5ae2aa4a2b2b9a335f5a 100644 (file)
@@ -34,6 +34,7 @@ var progtable = [mips.ALAST]obj.ProgInfo{
        obj.ACHECKNIL: {Flags: gc.LeftRead},
        obj.AVARDEF:   {Flags: gc.Pseudo | gc.RightWrite},
        obj.AVARKILL:  {Flags: gc.Pseudo | gc.RightWrite},
+       obj.AVARLIVE:  {Flags: gc.Pseudo | gc.LeftRead},
 
        // NOP is an internal no-op that also stands
        // for USED and SET annotations, not the MIPS opcode.
index 92293be251348e2869f904f3c0aaf69da66c6425..6b482564b7bdad2649133c255d312214cb966bb9 100644 (file)
@@ -34,6 +34,7 @@ var progtable = [ppc64.ALAST]obj.ProgInfo{
        obj.ACHECKNIL: {Flags: gc.LeftRead},
        obj.AVARDEF:   {Flags: gc.Pseudo | gc.RightWrite},
        obj.AVARKILL:  {Flags: gc.Pseudo | gc.RightWrite},
+       obj.AVARLIVE:  {Flags: gc.Pseudo | gc.LeftRead},
 
        // NOP is an internal no-op that also stands
        // for USED and SET annotations, not the Power opcode.
index 22ee23db12c1032f6f2b651c389a027287d75f88..ccac290dc4063c2f0878a89dc689fdec4bf151c1 100644 (file)
@@ -40,6 +40,7 @@ var progtable = [x86.ALAST]obj.ProgInfo{
        obj.ACHECKNIL: {Flags: gc.LeftRead},
        obj.AVARDEF:   {Flags: gc.Pseudo | gc.RightWrite},
        obj.AVARKILL:  {Flags: gc.Pseudo | gc.RightWrite},
+       obj.AVARLIVE:  {Flags: gc.Pseudo | gc.LeftRead},
 
        // NOP is an internal no-op that also stands
        // for USED and SET annotations, not the Intel opcode.
index 511e4098d07b68a45521beba58de139957c311a1..f7f7662ee73e1d04b2d93dce825008b1cf0ce54e 100644 (file)
@@ -282,6 +282,7 @@ const (
        AUSEFIELD
        AVARDEF
        AVARKILL
+       AVARLIVE
        A_ARCHSPECIFIC
 )
 
index 3e29b58dac4adc1490d20d4236e98815df9a5b32..1a974297ff125f84d635874cd8d214776884dc82 100644 (file)
@@ -608,7 +608,7 @@ func RegisterOpcode(lo int, Anames []string) {
 }
 
 func Aconv(a int) string {
-       if a < A_ARCHSPECIFIC {
+       if 0 <= a && a < len(Anames) {
                return Anames[a]
        }
        for i := range aSpace {
@@ -639,6 +639,7 @@ var Anames = []string{
        "UNDEF",
        "USEFIELD",
        "VARDEF",
+       "VARLIVE",
        "VARKILL",
 }
 
index 791bcbbb678c7bee7466b1216970efdbc7ea940d..769e6b9fd53172fb04a2c14b2b74d0b220d68e82 100644 (file)
@@ -95,5 +95,8 @@ func (tv *Timeval) Nano() int64 {
 
 // use is a no-op, but the compiler cannot see that it is.
 // Calling use(p) ensures that p is kept live until that point.
+// This was needed until Go 1.6 to call syscall.Syscall correctly.
+// As of Go 1.6 the compiler handles that case automatically.
+// The uses and definition of use can be removed early in the Go 1.7 cycle.
 //go:noescape
 func use(p unsafe.Pointer)
diff --git a/test/live_syscall.go b/test/live_syscall.go
new file mode 100644 (file)
index 0000000..c9bf0f2
--- /dev/null
@@ -0,0 +1,28 @@
+// errorcheck -0 -m -live
+
+// +build !windows
+
+// Copyright 2015 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.
+
+// Test escape analysis and liveness inferred for syscall.Syscall-like functions.
+
+package p
+
+import (
+       "syscall"
+       "unsafe"
+)
+
+func f(uintptr) // ERROR "f assuming arg#1 is unsafe uintptr"
+
+func g() {
+       var t int
+       f(uintptr(unsafe.Pointer(&t))) // ERROR "live at call to f: autotmp" "g &t does not escape"
+}
+
+func h() {
+       var v int
+       syscall.Syscall(0, 1, uintptr(unsafe.Pointer(&v)), 2) // ERROR "live at call to Syscall: autotmp" "h &v does not escape"
+}