Merge pull request #3284 from xmcclure/tarjan-crash-2-4.6.0pango-cfretain-font

Backport PR#3279 to 4.6.0

5 files changed, 109 insertions, 26 deletions

diff --git a/mono/metadata/sgen-bridge-internals.h b/mono/metadata/sgen-bridge-internals.h
index 0a80441910e..c5ad090b6c8 100644
--- a/mono/metadata/sgen-bridge-internals.h
+++ b/mono/metadata/sgen-bridge-internals.h
@@ -62,7 +62,7 @@ void sgen_new_bridge_init (SgenBridgeProcessor *collector);
 void sgen_tarjan_bridge_init (SgenBridgeProcessor *collector);
 void sgen_set_bridge_implementation (const char *name);
 void sgen_bridge_set_dump_prefix (const char *prefix);
-void sgen_init_bridge_processor();
+void sgen_init_bridge (void);
 
 #endif
 
diff --git a/mono/metadata/sgen-bridge.c b/mono/metadata/sgen-bridge.c
index a1f0f51fbbb..006e68c5012 100644
--- a/mono/metadata/sgen-bridge.c
+++ b/mono/metadata/sgen-bridge.c
@@ -21,12 +21,29 @@
 #include "sgen/sgen-qsort.h"
 #include "utils/mono-logger-internals.h"
 
+typedef enum {
+	BRIDGE_PROCESSOR_INVALID,
+	BRIDGE_PROCESSOR_OLD,
+	BRIDGE_PROCESSOR_NEW,
+	BRIDGE_PROCESSOR_TARJAN,
+	BRIDGE_PROCESSOR_DEFAULT = BRIDGE_PROCESSOR_TARJAN
+} BridgeProcessorSelection;
+
+// Bridge processor type pending / in use
+static BridgeProcessorSelection bridge_processor_selection = BRIDGE_PROCESSOR_DEFAULT;
+// Most recently requested callbacks
+static MonoGCBridgeCallbacks pending_bridge_callbacks;
+// Currently-in-use callbacks
 MonoGCBridgeCallbacks bridge_callbacks;
+
+// Bridge processor state
 static SgenBridgeProcessor bridge_processor;
+// This is used for a special debug feature
 static SgenBridgeProcessor compare_to_bridge_processor;
 
 volatile gboolean bridge_processing_in_progress = FALSE;
 
+// FIXME: The current usage pattern for this function is unsafe. Bridge processing could start immediately after unlock
 void
 mono_gc_wait_for_bridge_processing (void)
 {
@@ -45,40 +62,95 @@ mono_gc_register_bridge_callbacks (MonoGCBridgeCallbacks *callbacks)
 	if (callbacks->bridge_version != SGEN_BRIDGE_VERSION)
 		g_error ("Invalid bridge callback version. Expected %d but got %d\n", SGEN_BRIDGE_VERSION, callbacks->bridge_version);
 
-	bridge_callbacks = *callbacks;
+	// Defer assigning to bridge_callbacks until we have the gc lock.
+	// Note: This line is unsafe if we are on a separate thread from the one the runtime was initialized on.
+	pending_bridge_callbacks = *callbacks;
+
+	// If sgen has started, will assign bridge callbacks and init bridge
+	sgen_init_bridge ();
 }
 
-static gboolean
-init_bridge_processor_by_name (SgenBridgeProcessor *processor, const char *name)
+static BridgeProcessorSelection
+bridge_processor_name (const char *name)
 {
 	if (!strcmp ("old", name)) {
-		memset (processor, 0, sizeof (SgenBridgeProcessor));
-		sgen_old_bridge_init (processor);
+		return BRIDGE_PROCESSOR_OLD;
 	} else if (!strcmp ("new", name)) {
-		memset (processor, 0, sizeof (SgenBridgeProcessor));
-		sgen_new_bridge_init (processor);
+		return BRIDGE_PROCESSOR_NEW;
 	} else if (!strcmp ("tarjan", name)) {
-		memset (processor, 0, sizeof (SgenBridgeProcessor));
-		sgen_tarjan_bridge_init (processor);
+		return BRIDGE_PROCESSOR_TARJAN;
 	} else {
-		return FALSE;
+		return BRIDGE_PROCESSOR_INVALID;
+	}
+}
+
+// Initialize a single bridge processor
+static void
+init_bridge_processor (SgenBridgeProcessor *processor, BridgeProcessorSelection selection)
+{
+	memset (processor, 0, sizeof (SgenBridgeProcessor));
+
+	switch (selection) {
+		case BRIDGE_PROCESSOR_OLD:
+			sgen_old_bridge_init (processor);
+			break;
+		case BRIDGE_PROCESSOR_NEW:
+			sgen_new_bridge_init (processor);
+			break;
+		case BRIDGE_PROCESSOR_TARJAN:
+			sgen_tarjan_bridge_init (processor);
+			break;
+		default:
+			g_assert_not_reached ();
 	}
-	return TRUE;
 }
 
+/*
+ * Initializing the sgen bridge consists of setting the bridge callbacks,
+ * and initializing the bridge processor. Init should follow these rules:
+ *
+ *   - Init happens only after sgen is initialized (because we don't
+ *     know which bridge processor to initialize until then, and also
+ *     to allow bridge processor init to interact with sgen if it wants)
+ *
+ *   - Init happens only after mono_gc_register_bridge_callbacks is called
+ *
+ *   - Init should not happen concurrently with a GC (because a GC will
+ *     call sgen_need_bridge_processing at various times)
+ *
+ *   - Initializing the bridge processor should happen only once
+ *
+ * We call sgen_init_bridge when the callbacks are set, and also when sgen
+ * is done initing. Actual initialization then only occurs if it is ready.
+ */
 void
-sgen_init_bridge_processor()
+sgen_init_bridge ()
 {
-	// If a bridge was registered but there is no bridge processor yet, init defaults
-	if (bridge_callbacks.cross_references && !bridge_processor.reset_data)
-		sgen_tarjan_bridge_init (&bridge_processor);
+	if (sgen_gc_initialized ()) {
+		// This lock is not initialized until the GC is
+		sgen_gc_lock ();
+
+		bridge_callbacks = pending_bridge_callbacks;
+
+		// If a bridge was registered but there is no bridge processor yet
+		if (bridge_callbacks.cross_references && !bridge_processor.reset_data)
+			init_bridge_processor (&bridge_processor, bridge_processor_selection);
+
+		sgen_gc_unlock ();
+	}
 }
 
 void
 sgen_set_bridge_implementation (const char *name)
 {
-	if (!init_bridge_processor_by_name (&bridge_processor, name))
-		g_warning ("Invalid value for bridge implementation, valid values are: 'new', 'old' and 'tarjan'.");
+	BridgeProcessorSelection selection = bridge_processor_name (name);
+
+	if (selection == BRIDGE_PROCESSOR_INVALID)
+		g_warning ("Invalid value for bridge processor implementation, valid values are: 'new', 'old' and 'tarjan'.");
+	else if (bridge_processor.reset_data)
+		g_warning ("Cannot set bridge processor implementation once bridge has already started");
+	else
+		bridge_processor_selection = selection;
 }
 
 gboolean
@@ -593,11 +665,10 @@ sgen_bridge_handle_gc_debug (const char *opt)
 		set_dump_prefix (prefix);
 	} else if (g_str_has_prefix (opt, "bridge-compare-to=")) {
 		const char *name = strchr (opt, '=') + 1;
-		if (init_bridge_processor_by_name (&compare_to_bridge_processor, name)) {
-			if (compare_to_bridge_processor.reset_data == bridge_processor.reset_data) {
-				g_warning ("Cannot compare bridge implementation to itself - ignoring.");
-				memset (&compare_to_bridge_processor, 0, sizeof (SgenBridgeProcessor));
-			}
+		BridgeProcessorSelection selection = bridge_processor_name (name);
+
+		if (selection != BRIDGE_PROCESSOR_INVALID) {
+			init_bridge_processor (&compare_to_bridge_processor, selection);
 		} else {
 			g_warning ("Invalid bridge implementation to compare against - ignoring.");
 		}
diff --git a/mono/metadata/sgen-bridge.h b/mono/metadata/sgen-bridge.h
index dcdcbddca9c..ea952efe10a 100644
--- a/mono/metadata/sgen-bridge.h
+++ b/mono/metadata/sgen-bridge.h
@@ -88,7 +88,11 @@ typedef struct {
 	void (*cross_references) (int num_sccs, MonoGCBridgeSCC **sccs, int num_xrefs, MonoGCBridgeXRef *xrefs);
 } MonoGCBridgeCallbacks;
 
-// Clients should call before initializing runtime.
+/*
+ * Note: This may be called at any time, but cannot be called concurrently
+ * with (during and on a separate thread from) sgen init. Callers are
+ * responsible for enforcing this.
+ */
 MONO_API void mono_gc_register_bridge_callbacks (MonoGCBridgeCallbacks *callbacks);
 
 MONO_API void mono_gc_wait_for_bridge_processing (void);
diff --git a/mono/sgen/sgen-gc.c b/mono/sgen/sgen-gc.c
index 6d21adc4d70..34c4d0d34c5 100644
--- a/mono/sgen/sgen-gc.c
+++ b/mono/sgen/sgen-gc.c
@@ -3139,9 +3139,15 @@ sgen_gc_init (void)
 
 	sgen_register_root (NULL, 0, sgen_make_user_root_descriptor (sgen_mark_normal_gc_handles), ROOT_TYPE_NORMAL, MONO_ROOT_SOURCE_GC_HANDLE, "normal gc handles");
 
-	sgen_init_bridge_processor();
-
 	gc_initialized = 1;
+
+	sgen_init_bridge ();
+}
+
+gboolean
+sgen_gc_initialized ()
+{
+	return gc_initialized > 0;
 }
 
 NurseryClearPolicy
diff --git a/mono/sgen/sgen-gc.h b/mono/sgen/sgen-gc.h
index 2d6923c8398..ce9e739411b 100644
--- a/mono/sgen/sgen-gc.h
+++ b/mono/sgen/sgen-gc.h
@@ -278,6 +278,8 @@ void sgen_check_section_scan_starts (GCMemSection *section);
 
 void sgen_conservatively_pin_objects_from (void **start, void **end, void *start_nursery, void *end_nursery, int pin_type);
 
+gboolean sgen_gc_initialized (void);
+
 /* Keep in sync with description_for_type() in sgen-internal.c! */
 enum {
 	INTERNAL_MEM_PIN_QUEUE,