[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)).

diff --git a/mono/metadata/handle.c b/mono/metadata/handle.c
index 4be4c632d10e53115c39384faec8f8aa2d0ab2e6..44327aed131a7f85595486d2465e19293180d58f 100644 (file)
--- 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 102f9f4da7244fbb8aac1b0909d239bd1a49b190..76a43969eba0dd99bd64b78cf7b5674444699784 100644 (file)
--- 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;
 }
 
 /*