runtime: fix comments on the behavior of SetGCPercent

Fixes for #49680, #49695, #45867, and #49370 all assumed that
SetGCPercent(-1) doesn't block until the GC's mark phase is done, but
it actually does. The cause of 3 of those 4 failures comes from the fact
that at the beginning of the sweep phase, the GC does try to preempt
every P once, and this may run concurrently with test code. In the
fourth case, the issue was likely that only *one* of the debug_test.go
tests was missing a call to SetGCPercent(-1). Just to be safe, leave a
TODO there for now to remove the extraneous runtime.GC calls, but leave
the calls in.

Updates #49680, #49695, #45867, and #49370.

Change-Id: Ibf4e64addfba18312526968bcf40f1f5d54eb3f1
Reviewed-on: https://go-review.googlesource.com/c/go/+/369815
Reviewed-by: Austin Clements <austin@google.com>
Trust: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

1 files changed, 18 insertions, 5 deletions

diff --git a/src/runtime/debug_test.go b/src/runtime/debug_test.go
index 44585b1744..5bb0c5cee3 100644
--- a/src/runtime/debug_test.go
+++ b/src/runtime/debug_test.go
@@ -34,10 +34,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). A GC
-	// could also actively be in progress (see issue #49370), so we
-	// need to call runtime.GC to block until it has complete. We
-	// use 8 Ps so there's room for the debug call worker,
+	// tries to interrupt an atomic loop (see issue #10958). Execute
+	// an extra GC to ensure even the sweep phase is done (out of
+	// caution to prevent #49370 from happening).
+	// TODO(mknyszek): This extra GC cycle is likely unnecessary
+	// because preemption (which may happen during the sweep phase)
+	// isn't much of an issue anymore thanks to asynchronous preemption.
+	// The biggest risk is having a write barrier in the debug call
+	// injection test code fire, because it runs in a signal handler
+	// and may not have a P.
+	//
+	// 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)
@@ -270,8 +277,14 @@ func TestDebugCallPanic(t *testing.T) {
 	// progress. Wait until the current GC is done, and turn it off.
 	//
 	// See #10958 and #49370.
-	runtime.GC()
 	defer debug.SetGCPercent(debug.SetGCPercent(-1))
+	// TODO(mknyszek): This extra GC cycle is likely unnecessary
+	// because preemption (which may happen during the sweep phase)
+	// isn't much of an issue anymore thanks to asynchronous preemption.
+	// The biggest risk is having a write barrier in the debug call
+	// injection test code fire, because it runs in a signal handler
+	// and may not have a P.
+	runtime.GC()
 
 	ready := make(chan *runtime.G)
 	var stop uint32