runtime: hold sched.lock over traceThreadDestroy in dropm

This is required by traceThreadDestroy, though it's not strictly
necessary in this case. The requirement to hold sched.lock comes from
the assumption that traceThreadDestroy is getting called when the thread
leaves the tracer's view, but in this case the extra m that dropm is
dropping never leaves the allm list. Nevertheless, traceThreadDestroy
requires it just as a safety measure, and that's reasonable. dropm is
generally rare on pthread platforms, so the extra lock acquire over this
short critical section (and only when tracing is enabled) is fine.

Change-Id: Ib631820963c74f2f087d14a0067d0441d75d6785
Reviewed-on: https://go-review.googlesource.com/c/go/+/544396
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

diff --git a/src/runtime/proc.go b/src/runtime/proc.go
index e7605729064f11dacd1e0bf030dbab9a0082af8c..7a2283e3606e8cb04858ac8cc8123ee673b9edc7 100644 (file)
--- a/src/runtime/proc.go
+++ b/src/runtime/proc.go
@@ -2428,8 +2428,20 @@ func dropm() {
        // Flush all the M's buffers. This is necessary because the M might
        // be used on a different thread with a different procid, so we have
        // to make sure we don't write into the same buffer.
-       if traceEnabled() || traceShuttingDown() {
+       //
+       // N.B. traceThreadDestroy is a no-op in the old tracer, so avoid the
+       // unnecessary acquire/release of the lock.
+       if goexperiment.ExecTracer2 && (traceEnabled() || traceShuttingDown()) {
+               // Acquire sched.lock across thread destruction. One of the invariants of the tracer
+               // is that a thread cannot disappear from the tracer's view (allm or freem) without
+               // it noticing, so it requires that sched.lock be held over traceThreadDestroy.
+               //
+               // This isn't strictly necessary in this case, because this thread never leaves allm,
+               // but the critical section is short and dropm is rare on pthread platforms, so just
+               // take the lock and play it safe. traceThreadDestroy also asserts that the lock is held.
+               lock(&sched.lock)
                traceThreadDestroy(mp)
+               unlock(&sched.lock)
        }
        mp.isExtraInSig = false