From 420c68dd68c648af6642dd7e5cf6dacf9f067f6e Mon Sep 17 00:00:00 2001
From: Michael Pratt <mpratt@google.com>
Date: Fri, 30 Oct 2020 15:22:52 -0400
Subject: [PATCH] runtime: tighten systemstack in lock assertions

We use systemstack on the locking path to avoid stack splits which could
cause locks to be recorded out of order (see comment on lockWithRank).

This concern is irrelevant on lock assertions, where we simply need to
see if a lock is held and don't care if another is taken in the
meantime. Thus we can simply drop these unless we actually need to
crash.

Updates #40677

Change-Id: I85d730913a59867753ee1ed0386f8c5efda5c432
Reviewed-on: https://go-review.googlesource.com/c/go/+/266718
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
Trust: Michael Pratt <mpratt@google.com>
---
 src/runtime/lockrank_on.go | 76 +++++++++++++++++++++++---------------
 1 file changed, 47 insertions(+), 29 deletions(-)

diff --git a/src/runtime/lockrank_on.go b/src/runtime/lockrank_on.go
index c25b3a4656..88ac95a004 100644
--- a/src/runtime/lockrank_on.go
+++ b/src/runtime/lockrank_on.go
@@ -95,7 +95,8 @@ func lockWithRank(l *mutex, rank lockRank) {
 	})
 }
 
-//go:systemstack
+// nosplit to ensure it can be called in as many contexts as possible.
+//go:nosplit
 func printHeldLocks(gp *g) {
 	if gp.m.locksHeldLen == 0 {
 		println("<none>")
@@ -113,7 +114,7 @@ func printHeldLocks(gp *g) {
 //go:nosplit
 func acquireLockRank(rank lockRank) {
 	gp := getg()
-	// Log the new class.
+	// Log the new class. See comment on lockWithRank.
 	systemstack(func() {
 		i := gp.m.locksHeldLen
 		if i >= len(gp.m.locksHeld) {
@@ -238,7 +239,8 @@ func lockWithRankMayAcquire(l *mutex, rank lockRank) {
 	})
 }
 
-//go:systemstack
+// nosplit to ensure it can be called in as many contexts as possible.
+//go:nosplit
 func checkLockHeld(gp *g, l *mutex) bool {
 	for i := gp.m.locksHeldLen - 1; i >= 0; i-- {
 		if gp.m.locksHeld[i].lockAddr == uintptr(unsafe.Pointer(l)) {
@@ -255,14 +257,18 @@ func checkLockHeld(gp *g, l *mutex) bool {
 func assertLockHeld(l *mutex) {
 	gp := getg()
 
+	held := checkLockHeld(gp, l)
+	if held {
+		return
+	}
+
+	// Crash from system stack to avoid splits that may cause
+	// additional issues.
 	systemstack(func() {
-		held := checkLockHeld(gp, l)
-		if !held {
-			printlock()
-			print("caller requires lock ", l, " (rank ", l.rank.String(), "), holding:\n")
-			printHeldLocks(gp)
-			throw("not holding required lock!")
-		}
+		printlock()
+		print("caller requires lock ", l, " (rank ", l.rank.String(), "), holding:\n")
+		printHeldLocks(gp)
+		throw("not holding required lock!")
 	})
 }
 
@@ -276,13 +282,15 @@ func assertLockHeld(l *mutex) {
 func assertRankHeld(r lockRank) {
 	gp := getg()
 
-	systemstack(func() {
-		for i := gp.m.locksHeldLen - 1; i >= 0; i-- {
-			if gp.m.locksHeld[i].rank == r {
-				return
-			}
+	for i := gp.m.locksHeldLen - 1; i >= 0; i-- {
+		if gp.m.locksHeld[i].rank == r {
+			return
 		}
+	}
 
+	// Crash from system stack to avoid splits that may cause
+	// additional issues.
+	systemstack(func() {
 		printlock()
 		print("caller requires lock with rank ", r.String(), "), holding:\n")
 		printHeldLocks(gp)
@@ -298,8 +306,10 @@ func assertRankHeld(r lockRank) {
 //go:nosplit
 func worldStopped() {
 	if stopped := atomic.Xadd(&worldIsStopped, 1); stopped != 1 {
-		print("world stop count=", stopped, "\n")
-		throw("recursive world stop")
+		systemstack(func() {
+			print("world stop count=", stopped, "\n")
+			throw("recursive world stop")
+		})
 	}
 }
 
@@ -311,8 +321,10 @@ func worldStopped() {
 //go:nosplit
 func worldStarted() {
 	if stopped := atomic.Xadd(&worldIsStopped, -1); stopped != 0 {
-		print("world stop count=", stopped, "\n")
-		throw("released non-stopped world stop")
+		systemstack(func() {
+			print("world stop count=", stopped, "\n")
+			throw("released non-stopped world stop")
+		})
 	}
 }
 
@@ -321,8 +333,10 @@ func worldStarted() {
 func checkWorldStopped() bool {
 	stopped := atomic.Load(&worldIsStopped)
 	if stopped > 1 {
-		print("inconsistent world stop count=", stopped, "\n")
-		throw("inconsistent world stop count")
+		systemstack(func() {
+			print("inconsistent world stop count=", stopped, "\n")
+			throw("inconsistent world stop count")
+		})
 	}
 
 	return stopped == 1
@@ -352,14 +366,18 @@ func assertWorldStoppedOrLockHeld(l *mutex) {
 	}
 
 	gp := getg()
+	held := checkLockHeld(gp, l)
+	if held {
+		return
+	}
+
+	// Crash from system stack to avoid splits that may cause
+	// additional issues.
 	systemstack(func() {
-		held := checkLockHeld(gp, l)
-		if !held {
-			printlock()
-			print("caller requires world stop or lock ", l, " (rank ", l.rank.String(), "), holding:\n")
-			println("<no world stop>")
-			printHeldLocks(gp)
-			throw("no world stop or required lock!")
-		}
+		printlock()
+		print("caller requires world stop or lock ", l, " (rank ", l.rank.String(), "), holding:\n")
+		println("<no world stop>")
+		printHeldLocks(gp)
+		throw("no world stop or required lock!")
 	})
 }
-- 
2.50.0