diff options
author | Austin Clements <austin@google.com> | 2018-12-14 11:55:44 -0500 |
---|---|---|
committer | Austin Clements <austin@google.com> | 2018-12-17 03:00:28 +0000 |
commit | a27f3d5cfbed01dd56a23647330a42dcf4bf6ea9 (patch) | |
tree | 87ebcd445c265fa73778069daffc93d78b26a06b | |
parent | e167587dd62a7e1d8683982e2b6eaa1c1cff9c67 (diff) | |
download | go-a27f3d5cfbed01dd56a23647330a42dcf4bf6ea9.tar.gz go-a27f3d5cfbed01dd56a23647330a42dcf4bf6ea9.zip |
runtime: fix hangs in TestDebugCall*
This fixes a few different issues that led to hangs and general
flakiness in the TestDebugCall* tests.
1. This fixes missing wake-ups in two error paths of the SIGTRAP
signal handler. If the goroutine was in an unknown state, or if
there was an unknown debug call status, we currently don't wake the
injection coordinator. These are terminal states, so this resulted
in a hang.
2. This adds a retry if the target goroutine is in a transient state
that prevents us from injecting a call. The most common failure
mode here is that the target goroutine is in _Grunnable, but this
was previously masked because it deadlocked the test.
3. Related to 2, this switches the "ready" signal from the target
goroutine from a blocking channel send to a non-blocking channel
send. This makes it much less likely that we'll catch this
goroutine while it's in the runtime performing that send.
4. This increases GOMAXPROCS from 2 to 8 during these tests. With the
current setting of 2, we can have at most the non-preemptible
goroutine we're injecting a call in to and the goroutine that's
trying to make it exit. If anything else comes along, it can
deadlock. One particular case I observed was in TestDebugCallGC,
where runtime.GC() returns before the forEachP that prepares
sweeping on all goroutines has finished. When this happens, the
forEachP blocks on the non-preemptible loop, which means we now
have at least three goroutines that need to run.
Fixes #25519.
Updates #29124.
Change-Id: I7bc41dc0b865b7d0bb379cb654f9a1218bc37428
Reviewed-on: https://go-review.googlesource.com/c/154112
Run-TryBot: Austin Clements <austin@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
-rw-r--r-- | src/runtime/debug_test.go | 20 | ||||
-rw-r--r-- | src/runtime/export_debug_test.go | 46 |
2 files changed, 48 insertions, 18 deletions
diff --git a/src/runtime/debug_test.go b/src/runtime/debug_test.go index 37dcafd145..f77a373d13 100644 --- a/src/runtime/debug_test.go +++ b/src/runtime/debug_test.go @@ -33,11 +33,17 @@ func startDebugCallWorker(t *testing.T) (g *runtime.G, after func()) { skipUnderDebugger(t) // This can deadlock if there aren't enough threads or if a GC - // tries to interrupt an atomic loop (see issue #10958). - ogomaxprocs := runtime.GOMAXPROCS(2) + // tries to interrupt an atomic loop (see issue #10958). We + // use 8 Ps so there's room for the debug call worker, + // something that's trying to preempt the call worker, and the + // goroutine that's trying to stop the call worker. + ogomaxprocs := runtime.GOMAXPROCS(8) ogcpercent := debug.SetGCPercent(-1) - ready := make(chan *runtime.G) + // ready is a buffered channel so debugCallWorker won't block + // on sending to it. This makes it less likely we'll catch + // debugCallWorker while it's in the runtime. + ready := make(chan *runtime.G, 1) var stop uint32 done := make(chan error) go debugCallWorker(ready, &stop, done) @@ -67,6 +73,10 @@ func debugCallWorker(ready chan<- *runtime.G, stop *uint32, done chan<- error) { close(done) } +// Don't inline this function, since we want to test adjusting +// pointers in the arguments. +// +//go:noinline func debugCallWorker2(stop *uint32, x *int) { for atomic.LoadUint32(stop) == 0 { // Strongly encourage x to live in a register so we @@ -193,7 +203,7 @@ func TestDebugCallUnsafePoint(t *testing.T) { // This can deadlock if there aren't enough threads or if a GC // tries to interrupt an atomic loop (see issue #10958). - defer runtime.GOMAXPROCS(runtime.GOMAXPROCS(2)) + defer runtime.GOMAXPROCS(runtime.GOMAXPROCS(8)) defer debug.SetGCPercent(debug.SetGCPercent(-1)) // Test that the runtime refuses call injection at unsafe points. @@ -215,7 +225,7 @@ func TestDebugCallPanic(t *testing.T) { skipUnderDebugger(t) // This can deadlock if there aren't enough threads. - defer runtime.GOMAXPROCS(runtime.GOMAXPROCS(2)) + defer runtime.GOMAXPROCS(runtime.GOMAXPROCS(8)) ready := make(chan *runtime.G) var stop uint32 diff --git a/src/runtime/export_debug_test.go b/src/runtime/export_debug_test.go index 74f8855de6..e97dd52f20 100644 --- a/src/runtime/export_debug_test.go +++ b/src/runtime/export_debug_test.go @@ -50,19 +50,31 @@ func InjectDebugCall(gp *g, fn, args interface{}, tkill func(tid int) error) (in h.gp = gp h.fv, h.argp, h.argSize = fv, argp, argSize h.handleF = h.handle // Avoid allocating closure during signal - noteclear(&h.done) defer func() { testSigtrap = nil }() - testSigtrap = h.inject - if err := tkill(tid); err != nil { - return nil, err - } - // Wait for completion. - notetsleepg(&h.done, -1) - if len(h.err) != 0 { - return nil, h.err + for i := 0; ; i++ { + testSigtrap = h.inject + noteclear(&h.done) + h.err = "" + + if err := tkill(tid); err != nil { + return nil, err + } + // Wait for completion. + notetsleepg(&h.done, -1) + if h.err != "" { + switch h.err { + case "retry _Grunnable", "executing on Go runtime stack": + // These are transient states. Try to get out of them. + if i < 100 { + Gosched() + continue + } + } + return nil, h.err + } + return h.panic, nil } - return h.panic, nil } type debugCallHandler struct { @@ -99,12 +111,18 @@ func (h *debugCallHandler) inject(info *siginfo, ctxt *sigctxt, gp2 *g) bool { h.savedRegs.fpstate = nil // Set PC to debugCallV1. ctxt.set_rip(uint64(funcPC(debugCallV1))) + // Call injected. Switch to the debugCall protocol. + testSigtrap = h.handleF + case _Grunnable: + // Ask InjectDebugCall to pause for a bit and then try + // again to interrupt this goroutine. + h.err = plainError("retry _Grunnable") + notewakeup(&h.done) default: h.err = plainError("goroutine in unexpected state at call inject") - return true + notewakeup(&h.done) } - // Switch to the debugCall protocol and resume execution. - testSigtrap = h.handleF + // Resume execution. return true } @@ -149,6 +167,7 @@ func (h *debugCallHandler) handle(info *siginfo, ctxt *sigctxt, gp2 *g) bool { sp := ctxt.rsp() reason := *(*string)(unsafe.Pointer(uintptr(sp))) h.err = plainError(reason) + // Don't wake h.done. We need to transition to status 16 first. case 16: // Restore all registers except RIP and RSP. rip, rsp := ctxt.rip(), ctxt.rsp() @@ -162,6 +181,7 @@ func (h *debugCallHandler) handle(info *siginfo, ctxt *sigctxt, gp2 *g) bool { notewakeup(&h.done) default: h.err = plainError("unexpected debugCallV1 status") + notewakeup(&h.done) } // Resume execution. return true |