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 <[email protected]>
Reviewed-by: Michael Knyszek <[email protected]>

Author: aclements

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

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