runtime: allow the tracer to be reentrant

This change allows the tracer to be reentrant by restructuring the
internals such that writing an event is atomic with respect to stack
growth. Essentially, a core set of functions that are involved in
acquiring a trace buffer and writing to it are all marked nosplit.

Stack growth is currently the only hidden place where the tracer may be
accidentally reentrant, preventing the tracer from being used
everywhere. It already lacks write barriers, lacks allocations, and is
non-preemptible. This change thus makes the tracer fully reentrant,
since the only reentry case it needs to handle is stack growth.

Since the invariants needed to attain this are subtle, this change also
extends the debugTraceReentrancy debug mode to check these invariants as
well. Specifically, the invariants are checked by setting the throwsplit
flag.

A side benefit of this change is it simplifies the trace event writing
API a good bit: there's no need to actually thread the event writer
through things, and most callsites look a bit simpler.

Change-Id: I7c329fb7a6cb936bd363c44cf882ea0a925132f3
Reviewed-on: https://go-review.googlesource.com/c/go/+/587599
Reviewed-by: Austin Clements <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

src/runtime/mheap.go

@@ -1368,7 +1368,7 @@ HaveSpan:
 
 	// Trace the span alloc.
 	if traceAllocFreeEnabled() {
-		trace := traceTryAcquire()
+		trace := traceAcquire()
 		if trace.ok() {
 			trace.SpanAlloc(s)
 			traceRelease(trace)
@@ -1556,7 +1556,7 @@ func (h *mheap) freeSpan(s *mspan) {
 	systemstack(func() {
 		// Trace the span free.
 		if traceAllocFreeEnabled() {
-			trace := traceTryAcquire()
+			trace := traceAcquire()
 			if trace.ok() {
 				trace.SpanFree(s)
 				traceRelease(trace)
@@ -1595,7 +1595,7 @@ func (h *mheap) freeSpan(s *mspan) {
 func (h *mheap) freeManual(s *mspan, typ spanAllocType) {
 	// Trace the span free.
 	if traceAllocFreeEnabled() {
-		trace := traceTryAcquire()
+		trace := traceAcquire()
 		if trace.ok() {
 			trace.SpanFree(s)
 			traceRelease(trace)

src/runtime/stack.go

@@ -416,7 +416,7 @@ func stackalloc(n uint32) stack {
 	}
 
 	if traceAllocFreeEnabled() {
-		trace := traceTryAcquire()
+		trace := traceAcquire()
 		if trace.ok() {
 			trace.GoroutineStackAlloc(uintptr(v), uintptr(n))
 			traceRelease(trace)
@@ -466,7 +466,7 @@ func stackfree(stk stack) {
 		return
 	}
 	if traceAllocFreeEnabled() {
-		trace := traceTryAcquire()
+		trace := traceAcquire()
 		if trace.ok() {
 			trace.GoroutineStackFree(uintptr(v))
 			traceRelease(trace)

src/runtime/traceallocfree.go

@@ -89,17 +89,17 @@ func traceSpanTypeAndClass(s *mspan) traceArg {
 
 // SpanExists records an event indicating that the span exists.
 func (tl traceLocker) SpanExists(s *mspan) {
-	tl.eventWriter(traceGoRunning, traceProcRunning).commit(traceEvSpan, traceSpanID(s), traceArg(s.npages), traceSpanTypeAndClass(s))
+	tl.eventWriter(traceGoRunning, traceProcRunning).event(traceEvSpan, traceSpanID(s), traceArg(s.npages), traceSpanTypeAndClass(s))
 }
 
 // SpanAlloc records an event indicating that the span has just been allocated.
 func (tl traceLocker) SpanAlloc(s *mspan) {
-	tl.eventWriter(traceGoRunning, traceProcRunning).commit(traceEvSpanAlloc, traceSpanID(s), traceArg(s.npages), traceSpanTypeAndClass(s))
+	tl.eventWriter(traceGoRunning, traceProcRunning).event(traceEvSpanAlloc, traceSpanID(s), traceArg(s.npages), traceSpanTypeAndClass(s))
 }
 
 // SpanFree records an event indicating that the span is about to be freed.
 func (tl traceLocker) SpanFree(s *mspan) {
-	tl.eventWriter(traceGoRunning, traceProcRunning).commit(traceEvSpanFree, traceSpanID(s))
+	tl.eventWriter(traceGoRunning, traceProcRunning).event(traceEvSpanFree, traceSpanID(s))
 }
 
 // traceSpanID creates a trace ID for the span s for the trace.
@@ -111,19 +111,19 @@ func traceSpanID(s *mspan) traceArg {
 // The type is optional, and the size of the slot occupied the object is inferred from the
 // span containing it.
 func (tl traceLocker) HeapObjectExists(addr uintptr, typ *abi.Type) {
-	tl.eventWriter(traceGoRunning, traceProcRunning).commit(traceEvHeapObject, traceHeapObjectID(addr), tl.rtype(typ))
+	tl.eventWriter(traceGoRunning, traceProcRunning).event(traceEvHeapObject, traceHeapObjectID(addr), tl.rtype(typ))
 }
 
 // HeapObjectAlloc records that an object was newly allocated at addr with the provided type.
 // The type is optional, and the size of the slot occupied the object is inferred from the
 // span containing it.
 func (tl traceLocker) HeapObjectAlloc(addr uintptr, typ *abi.Type) {
-	tl.eventWriter(traceGoRunning, traceProcRunning).commit(traceEvHeapObjectAlloc, traceHeapObjectID(addr), tl.rtype(typ))
+	tl.eventWriter(traceGoRunning, traceProcRunning).event(traceEvHeapObjectAlloc, traceHeapObjectID(addr), tl.rtype(typ))
 }
 
 // HeapObjectFree records that an object at addr is about to be freed.
 func (tl traceLocker) HeapObjectFree(addr uintptr) {
-	tl.eventWriter(traceGoRunning, traceProcRunning).commit(traceEvHeapObjectFree, traceHeapObjectID(addr))
+	tl.eventWriter(traceGoRunning, traceProcRunning).event(traceEvHeapObjectFree, traceHeapObjectID(addr))
 }
 
 // traceHeapObjectID creates a trace ID for a heap object at address addr.
@@ -134,18 +134,18 @@ func traceHeapObjectID(addr uintptr) traceArg {
 // GoroutineStackExists records that a goroutine stack already exists at address base with the provided size.
 func (tl traceLocker) GoroutineStackExists(base, size uintptr) {
 	order := traceCompressStackSize(size)
-	tl.eventWriter(traceGoRunning, traceProcRunning).commit(traceEvGoroutineStack, traceGoroutineStackID(base), order)
+	tl.eventWriter(traceGoRunning, traceProcRunning).event(traceEvGoroutineStack, traceGoroutineStackID(base), order)
 }
 
 // GoroutineStackAlloc records that a goroutine stack was newly allocated at address base with the provided size..
 func (tl traceLocker) GoroutineStackAlloc(base, size uintptr) {
 	order := traceCompressStackSize(size)
-	tl.eventWriter(traceGoRunning, traceProcRunning).commit(traceEvGoroutineStackAlloc, traceGoroutineStackID(base), order)
+	tl.eventWriter(traceGoRunning, traceProcRunning).event(traceEvGoroutineStackAlloc, traceGoroutineStackID(base), order)
 }
 
 // GoroutineStackFree records that a goroutine stack at address base is about to be freed.
 func (tl traceLocker) GoroutineStackFree(base uintptr) {
-	tl.eventWriter(traceGoRunning, traceProcRunning).commit(traceEvGoroutineStackFree, traceGoroutineStackID(base))
+	tl.eventWriter(traceGoRunning, traceProcRunning).event(traceEvGoroutineStackFree, traceGoroutineStackID(base))
 }
 
 // traceGoroutineStackID creates a trace ID for the goroutine stack from its base address.

src/runtime/tracebuf.go

@@ -27,8 +27,26 @@ type traceWriter struct {
 	*traceBuf
 }
 
-// write returns an a traceWriter that writes into the current M's stream.
+// writer returns an a traceWriter that writes into the current M's stream.
+//
+// Once this is called, the caller must guard against stack growth until
+// end is called on it. Therefore, it's highly recommended to use this
+// API in a "fluent" style, for example tl.writer().event(...).end().
+// Better yet, callers just looking to write events should use eventWriter
+// when possible, which is a much safer wrapper around this function.
+//
+// nosplit to allow for safe reentrant tracing from stack growth paths.
+//
+//go:nosplit
 func (tl traceLocker) writer() traceWriter {
+	if debugTraceReentrancy {
+		// Checks that the invariants of this function are being upheld.
+		gp := getg()
+		if gp == gp.m.curg {
+			tl.mp.trace.oldthrowsplit = gp.throwsplit
+			gp.throwsplit = true
+		}
+	}
 	return traceWriter{traceLocker: tl, traceBuf: tl.mp.trace.buf[tl.gen%2]}
 }
 
@@ -38,24 +56,74 @@ func (tl traceLocker) writer() traceWriter {
 // - Another traceLocker is held.
 // - trace.gen is prevented from advancing.
 //
+// This does not have the same stack growth restrictions as traceLocker.writer.
+//
 // buf may be nil.
 func unsafeTraceWriter(gen uintptr, buf *traceBuf) traceWriter {
 	return traceWriter{traceLocker: traceLocker{gen: gen}, traceBuf: buf}
 }
 
+// event writes out the bytes of an event into the event stream.
+//
+// nosplit because it's part of writing an event for an M, which must not
+// have any stack growth.
+//
+//go:nosplit
+func (w traceWriter) event(ev traceEv, args ...traceArg) traceWriter {
+	// N.B. Everything in this call must be nosplit to maintain
+	// the stack growth related invariants for writing events.
+
+	// Make sure we have room.
+	w, _ = w.ensure(1 + (len(args)+1)*traceBytesPerNumber)
+
+	// Compute the timestamp diff that we'll put in the trace.
+	ts := traceClockNow()
+	if ts <= w.traceBuf.lastTime {
+		ts = w.traceBuf.lastTime + 1
+	}
+	tsDiff := uint64(ts - w.traceBuf.lastTime)
+	w.traceBuf.lastTime = ts
+
+	// Write out event.
+	w.byte(byte(ev))
+	w.varint(tsDiff)
+	for _, arg := range args {
+		w.varint(uint64(arg))
+	}
+	return w
+}
+
 // end writes the buffer back into the m.
+//
+// nosplit because it's part of writing an event for an M, which must not
+// have any stack growth.
+//
+//go:nosplit
 func (w traceWriter) end() {
 	if w.mp == nil {
 		// Tolerate a nil mp. It makes code that creates traceWriters directly
 		// less error-prone.
 		return
 	}
 	w.mp.trace.buf[w.gen%2] = w.traceBuf
+	if debugTraceReentrancy {
+		// The writer is no longer live, we can drop throwsplit (if it wasn't
+		// already set upon entry).
+		gp := getg()
+		if gp == gp.m.curg {
+			gp.throwsplit = w.mp.trace.oldthrowsplit
+		}
+	}
 }
 
 // ensure makes sure that at least maxSize bytes are available to write.
 //
 // Returns whether the buffer was flushed.
+//
+// nosplit because it's part of writing an event for an M, which must not
+// have any stack growth.
+//
+//go:nosplit
 func (w traceWriter) ensure(maxSize int) (traceWriter, bool) {
 	refill := w.traceBuf == nil || !w.available(maxSize)
 	if refill {
@@ -65,6 +133,11 @@ func (w traceWriter) ensure(maxSize int) (traceWriter, bool) {
 }
 
 // flush puts w.traceBuf on the queue of full buffers.
+//
+// nosplit because it's part of writing an event for an M, which must not
+// have any stack growth.
+//
+//go:nosplit
 func (w traceWriter) flush() traceWriter {
 	systemstack(func() {
 		lock(&trace.lock)
@@ -80,6 +153,11 @@ func (w traceWriter) flush() traceWriter {
 // refill puts w.traceBuf on the queue of full buffers and refresh's w's buffer.
 //
 // exp indicates whether the refilled batch should be EvExperimentalBatch.
+//
+// nosplit because it's part of writing an event for an M, which must not
+// have any stack growth.
+//
+//go:nosplit
 func (w traceWriter) refill(exp traceExperiment) traceWriter {
 	systemstack(func() {
 		lock(&trace.lock)
@@ -179,12 +257,22 @@ type traceBuf struct {
 }
 
 // byte appends v to buf.
+//
+// nosplit because it's part of writing an event for an M, which must not
+// have any stack growth.
+//
+//go:nosplit
 func (buf *traceBuf) byte(v byte) {
 	buf.arr[buf.pos] = v
 	buf.pos++
 }
 
 // varint appends v to buf in little-endian-base-128 encoding.
+//
+// nosplit because it's part of writing an event for an M, which must not
+// have any stack growth.
+//
+//go:nosplit
 func (buf *traceBuf) varint(v uint64) {
 	pos := buf.pos
 	arr := buf.arr[pos : pos+traceBytesPerNumber]
@@ -203,17 +291,31 @@ func (buf *traceBuf) varint(v uint64) {
 // varintReserve reserves enough space in buf to hold any varint.
 //
 // Space reserved this way can be filled in with the varintAt method.
+//
+// nosplit because it's part of writing an event for an M, which must not
+// have any stack growth.
+//
+//go:nosplit
 func (buf *traceBuf) varintReserve() int {
 	p := buf.pos
 	buf.pos += traceBytesPerNumber
 	return p
 }
 
 // stringData appends s's data directly to buf.
+//
+// nosplit because it's part of writing an event for an M, which must not
+// have any stack growth.
+//
+//go:nosplit
 func (buf *traceBuf) stringData(s string) {
 	buf.pos += copy(buf.arr[buf.pos:], s)
 }
 
+// nosplit because it's part of writing an event for an M, which must not
+// have any stack growth.
+//
+//go:nosplit
 func (buf *traceBuf) available(size int) bool {
 	return len(buf.arr)-buf.pos >= size
 }
@@ -222,6 +324,11 @@ func (buf *traceBuf) available(size int) bool {
 // consumes traceBytesPerNumber bytes. This is intended for when the caller
 // needs to reserve space for a varint but can't populate it until later.
 // Use varintReserve to reserve this space.
+//
+// nosplit because it's part of writing an event for an M, which must not
+// have any stack growth.
+//
+//go:nosplit
 func (buf *traceBuf) varintAt(pos int, v uint64) {
 	for i := 0; i < traceBytesPerNumber; i++ {
 		if i < traceBytesPerNumber-1 {

src/runtime/traceevent.go

@@ -102,7 +102,7 @@ type traceArg uint64
 // See the comment on traceWriter about style for more details as to why
 // this type and its methods are structured the way they are.
 type traceEventWriter struct {
-	w traceWriter
+	tl traceLocker
 }
 
 // eventWriter creates a new traceEventWriter. It is the main entrypoint for writing trace events.
@@ -119,54 +119,18 @@ type traceEventWriter struct {
 //
 // In this case, the default status should be traceGoBad or traceProcBad to help identify bugs sooner.
 func (tl traceLocker) eventWriter(goStatus traceGoStatus, procStatus traceProcStatus) traceEventWriter {
-	w := tl.writer()
 	if pp := tl.mp.p.ptr(); pp != nil && !pp.trace.statusWasTraced(tl.gen) && pp.trace.acquireStatus(tl.gen) {
-		w = w.writeProcStatus(uint64(pp.id), procStatus, pp.trace.inSweep)
+		tl.writer().writeProcStatus(uint64(pp.id), procStatus, pp.trace.inSweep).end()
 	}
 	if gp := tl.mp.curg; gp != nil && !gp.trace.statusWasTraced(tl.gen) && gp.trace.acquireStatus(tl.gen) {
-		w = w.writeGoStatus(uint64(gp.goid), int64(tl.mp.procid), goStatus, gp.inMarkAssist, 0 /* no stack */)
+		tl.writer().writeGoStatus(uint64(gp.goid), int64(tl.mp.procid), goStatus, gp.inMarkAssist, 0 /* no stack */).end()
 	}
-	return traceEventWriter{w}
+	return traceEventWriter{tl}
 }
 
-// commit writes out a trace event and calls end. It's a helper to make the
-// common case of writing out a single event less error-prone.
-func (e traceEventWriter) commit(ev traceEv, args ...traceArg) {
-	e = e.write(ev, args...)
-	e.end()
-}
-
-// write writes an event into the trace.
-func (e traceEventWriter) write(ev traceEv, args ...traceArg) traceEventWriter {
-	e.w = e.w.event(ev, args...)
-	return e
-}
-
-// end finishes writing to the trace. The traceEventWriter must not be used after this call.
-func (e traceEventWriter) end() {
-	e.w.end()
-}
-
-// traceEventWrite is the part of traceEvent that actually writes the event.
-func (w traceWriter) event(ev traceEv, args ...traceArg) traceWriter {
-	// Make sure we have room.
-	w, _ = w.ensure(1 + (len(args)+1)*traceBytesPerNumber)
-
-	// Compute the timestamp diff that we'll put in the trace.
-	ts := traceClockNow()
-	if ts <= w.traceBuf.lastTime {
-		ts = w.traceBuf.lastTime + 1
-	}
-	tsDiff := uint64(ts - w.traceBuf.lastTime)
-	w.traceBuf.lastTime = ts
-
-	// Write out event.
-	w.byte(byte(ev))
-	w.varint(tsDiff)
-	for _, arg := range args {
-		w.varint(uint64(arg))
-	}
-	return w
+// event writes out a trace event.
+func (e traceEventWriter) event(ev traceEv, args ...traceArg) {
+	e.tl.writer().event(ev, args...).end()
 }
 
 // stack takes a stack trace skipping the provided number of frames.

src/runtime/traceexp.go

@@ -18,6 +18,8 @@ type traceExpWriter struct {
 // - Another traceLocker is held.
 // - trace.gen is prevented from advancing.
 //
+// This does not have the same stack growth restrictions as traceLocker.writer.
+//
 // buf may be nil.
 func unsafeTraceExpWriter(gen uintptr, buf *traceBuf, exp traceExperiment) traceExpWriter {
 	return traceExpWriter{traceWriter{traceLocker: traceLocker{gen: gen}, traceBuf: buf}, exp}