runtime: avoid race checking for preemption

Moving the "don't really preempt" check up earlier in the function
introduced a race where gp.stackguard0 might change between
the early check and the later one. Since the later one is missing the
"don't really preempt" logic, it could decide to preempt incorrectly.
Pull the result of the check into a local variable and use an atomic
to access stackguard0, to eliminate the race.

I believe this will fix the broken OS X and Solaris builders.

Change-Id: I238350dd76560282b0c15a3306549cbcf390dbff
Reviewed-on: https://go-review.googlesource.com/2823
Reviewed-by: Austin Clements <austin@google.com>

diff --git a/src/runtime/stack1.go b/src/runtime/stack1.go
index 2c12cd73f30992a12324190a869387ace087279d..743e1073c62d38625c2191969d35abfb50450498 100644 (file)
--- a/src/runtime/stack1.go
+++ b/src/runtime/stack1.go
@@ -642,6 +642,11 @@ func newstack() {
        thisg.m.morebuf.g = 0
        rewindmorestack(&gp.sched)
 
+       // NOTE: stackguard0 may change underfoot, if another thread
+       // is about to try to preempt gp. Read it just once and use that same
+       // value now and below.
+       preempt := atomicloaduintptr(&gp.stackguard0) == stackPreempt
+
        // Be conservative about where we preempt.
        // We are interested in preempting user Go code, not runtime code.
        // If we're holding locks, mallocing, or GCing, don't preempt.
@@ -653,7 +658,7 @@ func newstack() {
        // If the GC is in some way dependent on this goroutine (for example,
        // it needs a lock held by the goroutine), that small preemption turns
        // into a real deadlock.
-       if gp.stackguard0 == stackPreempt {
+       if preempt {
                if thisg.m.locks != 0 || thisg.m.mallocing != 0 || thisg.m.gcing != 0 || thisg.m.p.status != _Prunning {
                        // Let the goroutine keep running for now.
                        // gp->preempt is set, so it will be preempted next time.
@@ -694,7 +699,7 @@ func newstack() {
                writebarrierptr_nostore((*uintptr)(unsafe.Pointer(&gp.sched.ctxt)), uintptr(gp.sched.ctxt))
        }
 
-       if gp.stackguard0 == stackPreempt {
+       if preempt {
                if gp == thisg.m.g0 {
                        throw("runtime: preempt g0")
                }