From 3a60d3564ca9355e090a15de825a55a909c86692 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Thu, 17 Dec 2015 15:03:22 -0500 Subject: [PATCH] [coop] Checked build GC critical regions. For local handles in coop suspend configuartions we need the idea of a "critical region": a situation when transitioning from GC Unsafe (running) to GC Safe (blocking) mode constitutes a bug. In checked builds, we'd like to assert when that happens. This PR adds MONO_PREPARE_GC_CRITICAL_REGION, MONO_FINISH_GC_CRITICAL_REGION and MONO_REQ_GC_NOT_CRITICAL. In regular build, these are nops. In checked build, they use a new thread-local flag to prevent transitions from running to blocking. --- mono/metadata/handle.h | 26 ++++++++++++-------------- mono/utils/checked-build.c | 29 +++++++++++++++++++++++++++++ mono/utils/checked-build.h | 28 ++++++++++++++++++++++++++++ mono/utils/mono-threads-coop.h | 3 +++ 4 files changed, 72 insertions(+), 14 deletions(-) diff --git a/mono/metadata/handle.h b/mono/metadata/handle.h index 7f8039786d6..bf94296791f 100644 --- a/mono/metadata/handle.h +++ b/mono/metadata/handle.h @@ -45,16 +45,14 @@ struct _MonoHandleStorage { #else static inline void -mono_handle_check_in_critical_section (const gchar* file, const gint lineno) +mono_handle_check_in_critical_section () { - MonoThreadInfo *info = (MonoThreadInfo*) mono_thread_info_current_unchecked (); - if (info && !info->inside_critical_region) - g_error ("Assertion at %s:%d: mono_handle_check_in_critical_section failed", file, lineno); + MONO_REQ_GC_UNSAFE_MODE; } -#define mono_handle_obj(handle) (mono_handle_check_in_critical_section (__FILE__, __LINE__), (handle)->obj) +#define mono_handle_obj(handle) (mono_handle_check_in_critical_section (), (handle)->obj) -#define mono_handle_assign(handle,rawptr) do { mono_handle_check_in_critical_section (__FILE__, __LINE__); (handle)->obj = (rawptr); } while (0) +#define mono_handle_assign(handle,rawptr) do { mono_handle_check_in_critical_section (); (handle)->obj = (rawptr); } while (0) #endif @@ -84,31 +82,31 @@ mono_handle_domain (MonoHandle handle) #define MONO_HANDLE_SETREF(handle,fieldname,value) \ do { \ MonoHandle __value = (MonoHandle) (value); \ - MONO_PREPARE_CRITICAL; \ + MONO_PREPARE_GC_CRITICAL_REGION; \ MONO_OBJECT_SETREF (mono_handle_obj ((handle)), fieldname, mono_handle_obj (__value)); \ - MONO_FINISH_CRITICAL; \ + MONO_FINISH_GC_CRITICAL_REGION; \ } while (0) #define MONO_HANDLE_SET(handle,fieldname,value) \ do { \ - MONO_PREPARE_CRITICAL; \ + MONO_PREPARE_GC_CRITICAL_REGION; \ mono_handle_obj ((handle))->fieldname = (value); \ - MONO_FINISH_CRITICAL; \ + MONO_FINISH_GC_CRITICAL_REGION; \ } while (0) #define MONO_HANDLE_ARRAY_SETREF(handle,index,value) \ do { \ MonoHandle __value = (MonoHandle) (value); \ - MONO_PREPARE_CRITICAL; \ + MONO_PREPARE_GC_CRITICAL_REGION; \ mono_array_setref (mono_handle_obj ((handle)), (index), mono_handle_obj (__value)); \ - MONO_FINISH_CRITICAL; \ + MONO_FINISH_GC_CRITICAL_REGION; \ } while (0) #define MONO_HANDLE_ARRAY_SET(handle,type,index,value) \ do { \ - MONO_PREPARE_CRITICAL; \ + MONO_PREPARE_GC_CRITICAL_REGION; \ mono_array_set (mono_handle_obj ((handle)), (type), (index), (value)); \ - MONO_FINISH_CRITICAL; \ + MONO_FINISH_GC_CRITICAL_REGION; \ } while (0) /* handle arena specific functions */ diff --git a/mono/utils/checked-build.c b/mono/utils/checked-build.c index 9690d9d030d..5c4fd4249e9 100644 --- a/mono/utils/checked-build.c +++ b/mono/utils/checked-build.c @@ -81,6 +81,7 @@ translate_backtrace (gpointer native_trace[], int size) typedef struct { GPtrArray *transitions; + gboolean in_gc_critical_region; } CheckState; typedef struct { @@ -230,6 +231,34 @@ assert_gc_neutral_mode (void) } } +void * +critical_gc_region_begin(void) +{ + CheckState *state = get_state (); + state->in_gc_critical_region = TRUE; + return state; +} + + +void +critical_gc_region_end(void* token) +{ + CheckState *state = get_state(); + g_assert (state == token); + state->in_gc_critical_region = FALSE; +} + +void +assert_not_in_gc_critical_region(void) +{ + CheckState *state = get_state(); + if (state->in_gc_critical_region) { + MonoThreadInfo *cur = mono_thread_info_current(); + state = mono_thread_info_current_state(cur); + assertion_fail("Expected GC Unsafe mode, but was in %s state", mono_thread_state_name(state)); + } +} + // check_metadata_store et al: The goal of these functions is to verify that if there is a pointer from one mempool into // another, that the pointed-to memory is protected by the reference count mechanism for MonoImages. // diff --git a/mono/utils/checked-build.h b/mono/utils/checked-build.h index 8fac45b018f..83efd67c054 100644 --- a/mono/utils/checked-build.h +++ b/mono/utils/checked-build.h @@ -78,6 +78,24 @@ Functions that can be called from both coop or preept modes. assert_gc_neutral_mode (); \ } while (0); +/* In a GC critical region, the thread is not allowed to switch to GC safe mode. + * For example if the thread is about to call a method that will manipulate managed objects. + * The GC critical region must only occur in unsafe mode. + */ +#define MONO_PREPARE_GC_CRITICAL_REGION \ + MON_REQ_GC_UNSAFE_MODE \ + do { \ + void* __critical_gc_region_cookie = critical_gc_region_begin() + +#define MONO_FINISH_GC_CRITICAL_REGION \ + critical_gc_region_end(__critical_gc_region_cookie); \ + } while(0) + +/* Verify that the thread is not currently in a GC critical region. */ +#define MONO_REQ_GC_NOT_CRITICAL do { \ + assert_not_in_gc_critical_region(); \ + } while(0) + // Use when writing a pointer from one image or imageset to another. #define CHECKED_METADATA_WRITE_PTR(ptr, val) do { \ check_metadata_store (&(ptr), (val)); \ @@ -116,6 +134,10 @@ void assert_gc_safe_mode (void); void assert_gc_unsafe_mode (void); void assert_gc_neutral_mode (void); +void* critical_gc_region_begin(void); +void critical_gc_region_end(void* token); +void assert_not_in_gc_critical_region(void); + void checked_build_init (void); void checked_build_thread_transition(const char *transition, void *info, int from_state, int suspend_count, int next_state, int suspend_count_delta); @@ -132,6 +154,12 @@ void check_metadata_store_local(void *from, void *to); #define MONO_REQ_API_ENTRYPOINT #define MONO_REQ_RUNTIME_ENTRYPOINT +#define MONO_PREPARE_GC_CRITICAL_REGION +#define MONO_FINISH_GC_CRITICAL_REGION + +#define MONO_REQ_GC_NOT_CRITICAL + + #define CHECKED_MONO_INIT() #define CHECKED_BUILD_THREAD_TRANSITION(transition, info, from_state, suspend_count, next_state, suspend_count_delta) diff --git a/mono/utils/mono-threads-coop.h b/mono/utils/mono-threads-coop.h index 91f3cf811ec..52efd8309f3 100644 --- a/mono/utils/mono-threads-coop.h +++ b/mono/utils/mono-threads-coop.h @@ -13,6 +13,8 @@ #include #include +#include "checked-build.h" + G_BEGIN_DECLS /* JIT specific interface */ @@ -58,6 +60,7 @@ mono_threads_safepoint (void) } #define MONO_PREPARE_BLOCKING \ + MONO_REQ_GC_NOT_CRITICAL; \ do { \ gpointer __dummy; \ gpointer __blocking_cookie = mono_threads_prepare_blocking (&__dummy) -- 2.25.1