runtime: prevent unnecessary zeroing of large objects with pointers

CL 614257 refactored mallocgc but lost an optimization: if a span for a
large object is already backed by memory fresh from the OS (and thus
zeroed), we don't need to zero it. CL 614257 unconditionally zeroed
spans for large objects that contain pointers.

This change restores the optimization from before CL 614257, which seems
to matter in some real-world programs.

While we're here, let's also fix a hole with the garbage collector being
able to observe uninitialized memory of the large object is observed
by the conservative scanner before being published. The gory details are
in a comment in heapSetTypeLarge. In short, this change makes
span.largeType an atomic variable, such that the GC can only observe
initialized memory if span.largeType != nil.

Fixes #72991.

Change-Id: I2048aeb220ab363d252ffda7d980b8788e9674dc
Reviewed-on: https://go-review.googlesource.com/c/go/+/659956
Reviewed-by: Keith Randall <khr@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: Felix Geisendörfer <felix.geisendoerfer@datadoghq.com>

1 files changed, 29 insertions, 17 deletions

diff --git a/src/runtime/malloc.go b/src/runtime/malloc.go
index 7f85dfd0ed..46200037e2 100644
--- a/src/runtime/malloc.go
+++ b/src/runtime/malloc.go
@@ -1548,12 +1548,13 @@ func mallocgcLarge(size uintptr, typ *_type, needzero bool) (unsafe.Pointer, uin
 	size = span.elemsize
 	x := unsafe.Pointer(span.base())
 
-	// Ensure that the stores above that initialize x to
-	// type-safe memory and set the heap bits occur before
-	// the caller can make x observable to the garbage
-	// collector. Otherwise, on weakly ordered machines,
-	// the garbage collector could follow a pointer to x,
-	// but see uninitialized memory or stale heap bits.
+	// Ensure that the store above that sets largeType to
+	// nil happens before the caller can make x observable
+	// to the garbage collector.
+	//
+	// Otherwise, on weakly ordered machines, the garbage
+	// collector could follow a pointer to x, but see a stale
+	// largeType value.
 	publicationBarrier()
 
 	if writeBarrier.enabled {
@@ -1596,22 +1597,33 @@ func mallocgcLarge(size uintptr, typ *_type, needzero bool) (unsafe.Pointer, uin
 	}
 
 	// Objects can be zeroed late in a context where preemption can occur.
-	// If the object contains pointers, its pointer data must be cleared
-	// or otherwise indicate that the GC shouldn't scan it.
+	//
 	// x will keep the memory alive.
-	if noscan := typ == nil || !typ.Pointers(); !noscan || (needzero && span.needzero != 0) {
+	if needzero && span.needzero != 0 {
 		// N.B. size == fullSize always in this case.
 		memclrNoHeapPointersChunked(size, x) // This is a possible preemption point: see #47302
+	}
 
-		// Finish storing the type information for this case.
-		mp := acquirem()
-		if !noscan {
-			getMCache(mp).scanAlloc += heapSetTypeLarge(uintptr(x), size, typ, span)
-		}
-		// Publish the object with the now-zeroed memory.
-		publicationBarrier()
-		releasem(mp)
+	// Set the type and run the publication barrier while non-preemptible. We need to make
+	// sure that between heapSetTypeLarge and publicationBarrier we cannot get preempted,
+	// otherwise the GC could potentially observe non-zeroed memory but largeType set on weak
+	// memory architectures.
+	//
+	// The GC can also potentially observe non-zeroed memory if conservative scanning spuriously
+	// observes a partially-allocated object, see the freeIndexForScan update above. This case is
+	// handled by synchronization inside heapSetTypeLarge.
+	mp = acquirem()
+	if typ != nil && typ.Pointers() {
+		// Finish storing the type information, now that we're certain the memory is zeroed.
+		getMCache(mp).scanAlloc += heapSetTypeLarge(uintptr(x), size, typ, span)
 	}
+	// Publish the object again, now with zeroed memory and initialized type information.
+	//
+	// Even if we didn't update any type information, this is necessary to ensure that, for example,
+	// x written to a global without any synchronization still results in other goroutines observing
+	// zeroed memory.
+	publicationBarrier()
+	releasem(mp)
 	return x, size
 }