runtime: drop function context from traceback

Currently, gentraceback tracks the closure context of the outermost
frame. This used to be important for "unstarted" calls to reflect
function stubs, where "unstarted" calls are either deferred functions
or the entry-point of a goroutine that hasn't run. Because reflect
function stubs have a dynamic argument map, we have to reach into
their closure context to fetch to map, and how to do this differs
depending on whether the function has started. This was discovered in
issue #25897.

However, as part of the register ABI, "go" and "defer" were made much
simpler, and any "go" or "defer" of a function that takes arguments or
returns results gets wrapped in a closure that provides those
arguments (and/or discards the results). Hence, we'll see that closure
instead of a direct call to a reflect stub, and can get its static
argument map without any trouble.

The one case where we may still see an unstarted reflect stub is if
the function takes no arguments and has no results, in which case the
compiler can optimize away the wrapper closure. But in this case we
know the argument map is empty: the compiler can apply this
optimization precisely because the target function has no argument
frame.

As a result, we no longer need to track the closure context during
traceback, so this CL drops all of that mechanism.

We still have to be careful about the unstarted case because we can't
reach into the function's locals frame to pull out its context
(because it has no locals frame). We double-check that in this case
we're at the function entry.

I would prefer to do this with some in-code PCDATA annotations of
where to find the dynamic argument map, but that's a lot of mechanism
to introduce for just this. It might make sense to consider this along
with #53609.

Finally, we beef up the test for this so it more reliably forces the
runtime down this path. It's fundamentally probabilistic, but this
tweak makes it better. Scheduler testing hooks (#54475) would make it
possible to write a reliable test for this.

For #54466, but it's a nice clean-up all on its own.

Change-Id: I16e4f2364ba2ea4b1fec1e27f971b06756e7b09f
Reviewed-on: https://go-review.googlesource.com/c/go/+/424254
Run-TryBot: Austin Clements <austin@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
Auto-Submit: Austin Clements <austin@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>

diff --git a/src/cmd/compile/internal/escape/call.go b/src/cmd/compile/internal/escape/call.go
index 880d789aa11e49fef92b77ab9372a1367095debe..4f602ca15fe77948e94bad7dca0a0a6bf524e880 100644 (file)
--- a/src/cmd/compile/internal/escape/call.go
+++ b/src/cmd/compile/internal/escape/call.go
@@ -223,6 +223,10 @@ func (e *escape) goDeferStmt(n *ir.GoDeferStmt) {
 
        // If the function is already a zero argument/result function call,
        // just escape analyze it normally.
+       //
+       // Note that the runtime is aware of this optimization for
+       // "go" statements that start in reflect.makeFuncStub or
+       // reflect.methodValueCall.
        if call, ok := call.(*ir.CallExpr); ok && call.Op() == ir.OCALLFUNC {
                if sig := call.X.Type(); sig.NumParams()+sig.NumResults() == 0 {
                        if clo, ok := call.X.(*ir.ClosureExpr); ok && n.Op() == ir.OGO {

diff --git a/src/runtime/traceback.go b/src/runtime/traceback.go
index 599141af94c0e5106312aac4439cb2ff8a7f97b8..27ac7b097729285c5fa38231aa6f07f51feb340b 100644 (file)
--- a/src/runtime/traceback.go
+++ b/src/runtime/traceback.go
@@ -53,8 +53,6 @@ func gentraceback(pc0, sp0, lr0 uintptr, gp *g, skip int, pcbuf *uintptr, max in
        }
        level, _, _ := gotraceback()
 
-       var ctxt *funcval // Context pointer for unstarted goroutines. See issue #25897.
-
        if pc0 == ^uintptr(0) && sp0 == ^uintptr(0) { // Signal to fetch saved values from gp.
                if gp.syscallsp != 0 {
                        pc0 = gp.syscallpc
@@ -68,7 +66,6 @@ func gentraceback(pc0, sp0, lr0 uintptr, gp *g, skip int, pcbuf *uintptr, max in
                        if usesLR {
                                lr0 = gp.sched.lr
                        }
-                       ctxt = (*funcval)(gp.sched.ctxt)
                }
        }
 
@@ -297,10 +294,9 @@ func gentraceback(pc0, sp0, lr0 uintptr, gp *g, skip int, pcbuf *uintptr, max in
                        var ok bool
                        frame.arglen, frame.argmap, ok = getArgInfoFast(f, callback != nil)
                        if !ok {
-                               frame.arglen, frame.argmap = getArgInfo(&frame, f, callback != nil, ctxt)
+                               frame.arglen, frame.argmap = getArgInfo(&frame, f, callback != nil)
                        }
                }
-               ctxt = nil // ctxt is only needed to get arg maps for the topmost frame
 
                // Determine frame's 'continuation PC', where it can continue.
                // Normally this is the return address on the stack, but if sigpanic
@@ -683,40 +679,46 @@ func getArgInfoFast(f funcInfo, needArgMap bool) (arglen uintptr, argmap *bitvec
 
 // getArgInfo returns the argument frame information for a call to f
 // with call frame frame.
-//
-// This is used for both actual calls with active stack frames and for
-// deferred calls or goroutines that are not yet executing. If this is an actual
-// call, ctxt must be nil (getArgInfo will retrieve what it needs from
-// the active stack frame). If this is a deferred call or unstarted goroutine,
-// ctxt must be the function object that was deferred or go'd.
-func getArgInfo(frame *stkframe, f funcInfo, needArgMap bool, ctxt *funcval) (arglen uintptr, argmap *bitvector) {
+func getArgInfo(frame *stkframe, f funcInfo, needArgMap bool) (arglen uintptr, argmap *bitvector) {
        arglen = uintptr(f.args)
        if needArgMap && f.args == _ArgsSizeUnknown {
                // Extract argument bitmaps for reflect stubs from the calls they made to reflect.
                switch funcname(f) {
                case "reflect.makeFuncStub", "reflect.methodValueCall":
                        // These take a *reflect.methodValue as their
-                       // context register.
-                       var mv *reflectMethodValue
-                       var retValid bool
-                       if ctxt != nil {
-                               // This is not an actual call, but a
-                               // deferred call or an unstarted goroutine.
-                               // The function value is itself the *reflect.methodValue.
-                               mv = (*reflectMethodValue)(unsafe.Pointer(ctxt))
-                       } else {
-                               // This is a real call that took the
-                               // *reflect.methodValue as its context
-                               // register and immediately saved it
-                               // to 0(SP). Get the methodValue from
-                               // 0(SP).
-                               arg0 := frame.sp + sys.MinFrameSize
-                               mv = *(**reflectMethodValue)(unsafe.Pointer(arg0))
-                               // Figure out whether the return values are valid.
-                               // Reflect will update this value after it copies
-                               // in the return values.
-                               retValid = *(*bool)(unsafe.Pointer(arg0 + 4*goarch.PtrSize))
+                       // context register and immediately save it to 0(SP).
+                       // Get the methodValue from 0(SP).
+                       arg0 := frame.sp + sys.MinFrameSize
+
+                       minSP := frame.fp
+                       if !usesLR {
+                               // The CALL itself pushes a word.
+                               // Undo that adjustment.
+                               minSP -= goarch.PtrSize
+                       }
+                       if arg0 >= minSP {
+                               // The function hasn't started yet.
+                               // This only happens if f was the
+                               // start function of a new goroutine
+                               // that hasn't run yet *and* f takes
+                               // no arguments and has no results
+                               // (otherwise it will get wrapped in a
+                               // closure). In this case, we can't
+                               // reach into its locals because it
+                               // doesn't have locals yet, but we
+                               // also know its argument map is
+                               // empty.
+                               if frame.pc != f.entry() {
+                                       print("runtime: confused by ", funcname(f), ": no frame (sp=", hex(frame.sp), " fp=", hex(frame.fp), ") at entry+", hex(frame.pc-f.entry()), "\n")
+                                       throw("reflect mismatch")
+                               }
+                               return 0, nil
                        }
+                       mv := *(**reflectMethodValue)(unsafe.Pointer(arg0))
+                       // Figure out whether the return values are valid.
+                       // Reflect will update this value after it copies
+                       // in the return values.
+                       retValid := *(*bool)(unsafe.Pointer(arg0 + 4*goarch.PtrSize))
                        if mv.fn != f.entry() {
                                print("runtime: confused by ", funcname(f), "\n")
                                throw("reflect mismatch")

diff --git a/test/fixedbugs/issue25897a.go b/test/fixedbugs/issue25897a.go
index 6a724a79a59eb73aaa80b5ceeeaaea61a976f204..d4fa6c82feb507beb4831abbde6b5c201d784279 100644 (file)
--- a/test/fixedbugs/issue25897a.go
+++ b/test/fixedbugs/issue25897a.go
@@ -18,17 +18,34 @@ const N = 100
 
 func main() {
        runtime.GOMAXPROCS(1)
+       // Run GC in a loop. This makes it more likely GC will catch
+       // an unstarted goroutine then if we were to GC after kicking
+       // everything off.
+       go func() {
+               for {
+                       runtime.GC()
+               }
+       }()
        c := make(chan bool, N)
        for i := 0; i < N; i++ {
+               // Test both with an argument and without because this
+               // affects whether the compiler needs to generate a
+               // wrapper closure for the "go" statement.
                f := reflect.MakeFunc(reflect.TypeOf(((func(*int))(nil))),
                        func(args []reflect.Value) []reflect.Value {
                                c <- true
                                return nil
                        }).Interface().(func(*int))
                go f(nil)
+
+               g := reflect.MakeFunc(reflect.TypeOf(((func())(nil))),
+                       func(args []reflect.Value) []reflect.Value {
+                               c <- true
+                               return nil
+                       }).Interface().(func())
+               go g()
        }
-       runtime.GC()
-       for i := 0; i < N; i++ {
+       for i := 0; i < N*2; i++ {
                <-c
        }
 }