cmd/compile: handle unsafe.Pointer(f()) correctly

Previously statements like

    f(unsafe.Pointer(g()), int(h()))

would be reordered into a sequence of statements like

    autotmp_g := g()
    autotmp_h := h()
    f(unsafe.Pointer(autotmp_g), int(autotmp_h))

which can leave g's temporary value on the stack as a uintptr, rather
than an unsafe.Pointer. Instead, recognize uintptr-to-unsafe.Pointer
conversions when reordering function calls to instead produce:

    autotmp_g := unsafe.Pointer(g())
    autotmp_h := h()
    f(autotmp_g, int(autotmp_h))

Fixes #15329.

Change-Id: I2cdbd89d233d0d5c94791513a9fd5fd958d11ed5
Reviewed-on: https://go-review.googlesource.com/22273
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>

diff --git a/src/cmd/compile/internal/gc/order.go b/src/cmd/compile/internal/gc/order.go
index 7026ad79efa383caf6b33a102c7ef6a21ff7b5dd..da334a1558cfd4c5d9bf97eb27e3830e02c6a034 100644 (file)
--- a/src/cmd/compile/internal/gc/order.go
+++ b/src/cmd/compile/internal/gc/order.go
@@ -1082,6 +1082,20 @@ func orderexpr(n *Node, order *Order, lhs *Node) *Node {
                        n.Left = orderaddrtemp(n.Left, order)
                }
 
+       case OCONVNOP:
+               if n.Type.IsKind(TUNSAFEPTR) && n.Left.Type.IsKind(TUINTPTR) && (n.Left.Op == OCALLFUNC || n.Left.Op == OCALLINTER || n.Left.Op == OCALLMETH) {
+                       // When reordering unsafe.Pointer(f()) into a separate
+                       // statement, the conversion and function call must stay
+                       // together. See golang.org/issue/15329.
+                       orderinit(n.Left, order)
+                       ordercall(n.Left, order)
+                       if lhs == nil || lhs.Op != ONAME || instrumenting {
+                               n = ordercopyexpr(n, n.Type, order, 0)
+                       }
+               } else {
+                       n.Left = orderexpr(n.Left, order, nil)
+               }
+
        case OANDAND, OOROR:
                mark := marktemp(order)
                n.Left = orderexpr(n.Left, order, nil)

diff --git a/test/fixedbugs/issue15329.go b/test/fixedbugs/issue15329.go
new file mode 100644 (file)
index 0000000..30fbf13
--- /dev/null
+++ b/test/fixedbugs/issue15329.go
@@ -0,0 +1,79 @@
+// 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.
+
+// Previously, cmd/compile would rewrite
+//
+//     check(unsafe.Pointer(testMeth(1).Pointer()), unsafe.Pointer(testMeth(2).Pointer()))
+//
+// to
+//
+//     var autotmp_1 uintptr = testMeth(1).Pointer()
+//     var autotmp_2 uintptr = testMeth(2).Pointer()
+//     check(unsafe.Pointer(autotmp_1), unsafe.Pointer(autotmp_2))
+//
+// However, that means autotmp_1 is the only reference to the int
+// variable containing the value "1", but it's not a pointer type,
+// so it was at risk of being garbage collected by the evaluation of
+// testMeth(2).Pointer(), even though package unsafe's documentation
+// says the original code was allowed.
+//
+// Now cmd/compile rewrites it to
+//
+//     var autotmp_1 unsafe.Pointer = unsafe.Pointer(testMeth(1).Pointer())
+//     var autotmp_2 unsafe.Pointer = unsafe.Pointer(testMeth(2).Pointer())
+//     check(autotmp_1, autotmp_2)
+//
+// to ensure the pointed-to variables are visible to the GC.
+
+package main
+
+import (
+       "fmt"
+       "reflect"
+       "runtime"
+       "unsafe"
+)
+
+func main() {
+       // Test all the different ways we can invoke reflect.Value.Pointer.
+
+       // Direct method invocation.
+       check(unsafe.Pointer(testMeth(1).Pointer()), unsafe.Pointer(testMeth(2).Pointer()))
+
+       // Invocation via method expression.
+       check(unsafe.Pointer(reflect.Value.Pointer(testMeth(1))), unsafe.Pointer(reflect.Value.Pointer(testMeth(2))))
+
+       // Invocation via interface.
+       check(unsafe.Pointer(testInter(1).Pointer()), unsafe.Pointer(testInter(2).Pointer()))
+
+       // Invocation via method value.
+       check(unsafe.Pointer(testFunc(1)()), unsafe.Pointer(testFunc(2)()))
+}
+
+func check(p, q unsafe.Pointer) {
+       a, b := *(*int)(p), *(*int)(q)
+       if a != 1 || b != 2 {
+               fmt.Printf("got %v, %v; expected 1, 2\n", a, b)
+       }
+}
+
+func testMeth(x int) reflect.Value {
+       // Force GC to run.
+       runtime.GC()
+       return reflect.ValueOf(&x)
+}
+
+type Pointerer interface {
+       Pointer() uintptr
+}
+
+func testInter(x int) Pointerer {
+       return testMeth(x)
+}
+
+func testFunc(x int) func() uintptr {
+       return testMeth(x).Pointer
+}