From 76b32f3ee38e7b29cdbaa657a4826b65579f4e93 Mon Sep 17 00:00:00 2001
From: =?utf8?q?Aleksey=20Kliger=20=28=CE=BBgeek=29?= <akliger@gmail.com>
Date: Tue, 27 Sep 2016 17:02:18 -0400
Subject: [PATCH] [coop handles] Add some memory fences (#3617)

When running with async suspend (ie, not coop), a user thread might be
suspended while its manipulating the coop handle stack.  In that case,
we want to present a consitent view to mono_handle_stack_scan () by
adding memory fences to code that updates the stack chunks.  In
particular we want to ensure that the chunk size is increased after
the new slot has a valid (null) value.  Also when creating new chunks
make sure their size is initialized before the chunk stack is updated.

Fixes https://bugzilla.xamarin.com/show_bug.cgi?id=43921 (at least the
one stack trace observed in [comment 1](https://bugzilla.xamarin.com/show_bug.cgi?id=43921#c1)).
---
 mono/metadata/handle.c | 51 +++++++++++++++++++++++++++++++++++++++---
 mono/metadata/handle.h |  3 ++-
 2 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/mono/metadata/handle.c b/mono/metadata/handle.c
index 4be4c632d10..44327aed131 100644
--- a/mono/metadata/handle.c
+++ b/mono/metadata/handle.c
@@ -45,6 +45,29 @@ Combine: MonoDefaults, GENERATE_GET_CLASS_WITH_CACHE, TYPED_HANDLE_DECL and frie
 	We could then generate neat type safe wrappers.
 */
 
+/*
+ * NOTE: Async suspend
+ * 
+ * If we are running with cooperative GC, all the handle stack
+ * manipulation will complete before a GC thread scans the handle
+ * stack. If we are using async suspend, however, a thread may be
+ * trying to allocate a new handle, or unwind the handle stack when
+ * the GC stops the world.
+ *
+ * In particular, we need to ensure that if the mutator thread is
+ * suspended while manipulating the handle stack, the stack is in a
+ * good enough state to be scanned.  In particular, the size of each
+ * chunk should be updated before an object is written into the
+ * handle, and chunks to be scanned (between bottom and top) should
+ * always be valid.
+ *
+ * Note that the handle stack is scanned PRECISELY (see
+ * sgen_client_scan_thread_data ()).  That means there should not be
+ * stale objects scanned.  So when we manipulate the size of a chunk,
+ * wemust ensure that the newly scannable slot is either null or
+ * points to a valid value.
+ */
+
 const MonoObjectHandle mono_null_value_handle = NULL;
 
 #define THIS_IS_AN_OK_NUMBER_OF_HANDLES 100
@@ -59,13 +82,28 @@ mono_handle_new (MonoObject *object)
 
 retry:
 	if (G_LIKELY (top->size < OBJECTS_PER_HANDLES_CHUNK)) {
-		MonoObject **h = &top->objects [top->size++];
+		int idx = top->size;
+		/* can be interrupted anywhere here, so:
+		 * 1. make sure the new slot is null
+		 * 2. make the new slot scannable (increment size)
+		 * 3. put a valid object in there
+		 *
+		 * (have to do 1 then 3 so that if we're interrupted
+		 * between 1 and 2, the object is still live)
+		 */
+		top->objects [idx] = NULL;
+		mono_memory_write_barrier ();
+		top->size++;
+		mono_memory_write_barrier ();
+		MonoObject **h = &top->objects [idx];
 		*h = object;
 		return h;
 	}
 	if (G_LIKELY (top->next)) {
+		top->next->size = 0;
+		/* make sure size == 0 is visible to a GC thread before it sees the new top */
+		mono_memory_write_barrier ();
 		top = top->next;
-		top->size = 0;
 		handles->top = top;
 		goto retry;
 	}
@@ -73,6 +111,8 @@ retry:
 	new_chunk->size = 0;
 	new_chunk->prev = top;
 	new_chunk->next = NULL;
+	/* make sure size == 0 before new chunk is visible */
+	mono_memory_write_barrier ();
 	top->next = new_chunk;
 	handles->top = new_chunk;
 	goto retry;
@@ -86,9 +126,10 @@ mono_handle_stack_alloc (void)
 	HandleStack *stack = g_new (HandleStack, 1);
 	HandleChunk *chunk = g_new (HandleChunk, 1);
 
-	stack->top = stack->bottom = chunk;
 	chunk->size = 0;
 	chunk->prev = chunk->next = NULL;
+	mono_memory_write_barrier ();
+	stack->top = stack->bottom = chunk;
 	return stack;
 }
 
@@ -98,6 +139,8 @@ mono_handle_stack_free (HandleStack *stack)
 	if (!stack)
 		return;
 	HandleChunk *c = stack->bottom;
+	stack->top = stack->bottom = NULL;
+	mono_memory_write_barrier ();
 	while (c) {
 		HandleChunk *next = c->next;
 		g_free (c);
@@ -110,6 +153,8 @@ mono_handle_stack_free (HandleStack *stack)
 void
 mono_handle_stack_scan (HandleStack *stack, GcScanFunc func, gpointer gc_data)
 {
+	/* if we're running, we know the world is stopped.
+	 */
 	HandleChunk *cur = stack->bottom;
 	HandleChunk *last = stack->top;
 
diff --git a/mono/metadata/handle.h b/mono/metadata/handle.h
index 102f9f4da72..76a43969eba 100644
--- a/mono/metadata/handle.h
+++ b/mono/metadata/handle.h
@@ -91,8 +91,9 @@ static inline void
 mono_stack_mark_pop (MonoThreadInfo *info, HandleStackMark *stackmark)
 {
 	HandleStack *handles = (HandleStack *)info->handle_stack;
-	handles->top = stackmark->chunk;
 	handles->top->size = stackmark->size;
+	mono_memory_write_barrier ();
+	handles->top = stackmark->chunk;
 }
 
 /*
-- 
2.25.1