From 7ff9eeb5abc4e69cddbd77f149a918acb36dbbd8 Mon Sep 17 00:00:00 2001 From: Jay Krell Date: Sun, 8 Dec 2019 06:30:48 -0800 Subject: [PATCH] Add some missing barriers when dealing with global caches (mono/mono#18032) * Add some missing barriers when dealing with global caches accessed from multiple threads without locks. This is based on earlier PR https://github.com/mono/mono/pull/17849. Read-side barriers are removed. More analysis is needed to determine if they are needed (which pointer read vs. written). Write-side barriers are compiler barrier + store-release instead of full barrier. Note all candidates are converted to new form yet. Cases that do motivate read-wide barriers are skipped. e.g. `mono_arch_start_dyn_call` `get_agent_domain_info` `init_jit_info_dbg_attrs` * PR: Remove 'Try one more time..' Commit migrated from https://github.com/mono/mono/commit/a6c4eafee8a264ce786d00c6035b0acc7fa97a8b --- src/mono/mono/metadata/appdomain.c | 50 ++++++++++++++------------ src/mono/mono/metadata/assembly-load-context.c | 30 ++++++++-------- src/mono/mono/metadata/boehm-gc.c | 7 ++-- src/mono/mono/metadata/class-init.c | 1 + src/mono/mono/metadata/class-internals.h | 42 +++++++++++++++++++++- 5 files changed, 89 insertions(+), 41 deletions(-) diff --git a/src/mono/mono/metadata/appdomain.c b/src/mono/mono/metadata/appdomain.c index b5e1e0e..774e0b9 100644 --- a/src/mono/mono/metadata/appdomain.c +++ b/src/mono/mono/metadata/appdomain.c @@ -814,13 +814,14 @@ mono_domain_has_type_resolve (MonoDomain *domain) #ifdef ENABLE_NETCORE return FALSE; #else - static MonoClassField *field = NULL; MonoObject *o; - if (field == NULL) { + MONO_STATIC_POINTER_INIT (MonoClassField, field) + field = mono_class_get_field_from_name_full (mono_defaults.appdomain_class, "TypeResolve", NULL); g_assert (field); - } + + MONO_STATIC_POINTER_INIT_END (MonoClassField, field) /*pedump doesn't create an appdomin, so the domain object doesn't exist.*/ if (!domain->domain) @@ -876,15 +877,14 @@ exit: static MonoMethod * mono_class_get_appdomain_do_type_resolve_method (MonoError *error) { - static MonoMethod *method; // cache - - if (method) - return method; + MONO_STATIC_POINTER_INIT (MonoMethod, method) // not cached yet, fill cache under caller's lock method = mono_class_get_method_from_name_checked (mono_class_get_appdomain_class (), "DoTypeResolve", -1, 0, error); + MONO_STATIC_POINTER_INIT_END (MonoMethod, method) + if (method == NULL) g_warning ("%s method AppDomain.DoTypeResolve not found. %s\n", __func__, mono_error_get_message (error)); @@ -899,15 +899,14 @@ mono_class_get_appdomain_do_type_resolve_method (MonoError *error) static MonoMethod * mono_class_get_appdomain_do_type_builder_resolve_method (MonoError *error) { - static MonoMethod *method; // cache - - if (method) - return method; + MONO_STATIC_POINTER_INIT (MonoMethod, method) // not cached yet, fill cache under caller's lock method = mono_class_get_method_from_name_checked (mono_class_get_appdomain_class (), "DoTypeBuilderResolve", -1, 0, error); + MONO_STATIC_POINTER_INIT_END (MonoMethod, method) + if (method == NULL) g_warning ("%s method AppDomain.DoTypeBuilderResolve not found. %s\n", __func__, mono_error_get_message (error)); @@ -1366,17 +1365,20 @@ mono_try_assembly_resolve_handle (MonoAssemblyLoadContext *alc, MonoStringHandle HANDLE_FUNCTION_ENTER (); MonoAssembly *ret = NULL; MonoDomain *domain = mono_alc_domain (alc); - static MonoMethod *method; if (mono_runtime_get_no_exec ()) goto leave; #ifndef ENABLE_NETCORE + + static MonoMethod *method; MonoBoolean isrefonly; gpointer params [3]; g_assert (domain != NULL && !MONO_HANDLE_IS_NULL (fname)); + // FIXME cache? + method = mono_class_get_method_from_name_checked (mono_class_get_appdomain_class (), "DoAssemblyResolve", -1, 0, error); g_assert (method != NULL); @@ -1407,14 +1409,16 @@ mono_try_assembly_resolve_handle (MonoAssemblyLoadContext *alc, MonoStringHandle goto leave; } #else - if (!method) { + MONO_STATIC_POINTER_INIT (MonoMethod, method) + ERROR_DECL (local_error); MonoClass *alc_class = mono_class_get_assembly_load_context_class (); g_assert (alc_class); - MonoMethod *found = mono_class_get_method_from_name_checked (alc_class, "OnAssemblyResolve", -1, 0, local_error); + method = mono_class_get_method_from_name_checked (alc_class, "OnAssemblyResolve", -1, 0, local_error); mono_error_assert_ok (local_error); - method = found; - } + + MONO_STATIC_POINTER_INIT_END (MonoMethod, method) + g_assert (method); MonoReflectionAssemblyHandle requesting_handle; @@ -1546,8 +1550,6 @@ static void mono_domain_fire_assembly_load (MonoAssemblyLoadContext *alc, MonoAssembly *assembly, gpointer user_data, MonoError *error_out) { HANDLE_FUNCTION_ENTER (); - static MonoClassField *assembly_load_field; - static MonoMethod *assembly_load_method; ERROR_DECL (error); MonoDomain *domain = mono_alc_domain (alc); MonoClass *klass; @@ -1572,10 +1574,12 @@ mono_domain_fire_assembly_load (MonoAssemblyLoadContext *alc, MonoAssembly *asse add_assembly_to_alc (alc, assembly); #endif - if (assembly_load_field == NULL) { + MONO_STATIC_POINTER_INIT (MonoClassField, assembly_load_field) + assembly_load_field = mono_class_get_field_from_name_full (klass, "AssemblyLoad", NULL); g_assert (assembly_load_field); - } + + MONO_STATIC_POINTER_INIT_END (MonoClassField, assembly_load_field) if (!MONO_HANDLE_GET_FIELD_BOOL (appdomain, MonoObject*, assembly_load_field)) goto leave; // No events waiting to be triggered @@ -1584,10 +1588,12 @@ mono_domain_fire_assembly_load (MonoAssemblyLoadContext *alc, MonoAssembly *asse reflection_assembly = mono_assembly_get_object_handle (domain, assembly, error); mono_error_assert_ok (error); - if (assembly_load_method == NULL) { + MONO_STATIC_POINTER_INIT (MonoMethod, assembly_load_method) + assembly_load_method = mono_class_get_method_from_name_checked (klass, "DoAssemblyLoad", -1, 0, error); g_assert (assembly_load_method); - } + + MONO_STATIC_POINTER_INIT_END (MonoMethod, assembly_load_method) void *params [1]; params [0] = MONO_HANDLE_RAW (reflection_assembly); diff --git a/src/mono/mono/metadata/assembly-load-context.c b/src/mono/mono/metadata/assembly-load-context.c index 708293c..133ebb8 100644 --- a/src/mono/mono/metadata/assembly-load-context.c +++ b/src/mono/mono/metadata/assembly-load-context.c @@ -152,16 +152,16 @@ leave: static MonoAssembly* mono_alc_invoke_resolve_using_load (MonoAssemblyLoadContext *alc, MonoAssemblyName *aname, MonoError *error) { - static MonoMethod *resolve; + MONO_STATIC_POINTER_INIT (MonoMethod, resolve) - if (!resolve) { ERROR_DECL (local_error); MonoClass *alc_class = mono_class_get_assembly_load_context_class (); g_assert (alc_class); - MonoMethod *m = mono_class_get_method_from_name_checked (alc_class, "MonoResolveUsingLoad", -1, 0, local_error); + resolve = mono_class_get_method_from_name_checked (alc_class, "MonoResolveUsingLoad", -1, 0, local_error); mono_error_assert_ok (local_error); - resolve = m; - } + + MONO_STATIC_POINTER_INIT_END (MonoMethod, resolve) + g_assert (resolve); return invoke_resolve_method (resolve, alc, aname, error); @@ -185,16 +185,16 @@ mono_alc_invoke_resolve_using_load_nofail (MonoAssemblyLoadContext *alc, MonoAss static MonoAssembly* mono_alc_invoke_resolve_using_resolving_event (MonoAssemblyLoadContext *alc, MonoAssemblyName *aname, MonoError *error) { - static MonoMethod *resolve; + MONO_STATIC_POINTER_INIT (MonoMethod, resolve) - if (!resolve) { ERROR_DECL (local_error); MonoClass *alc_class = mono_class_get_assembly_load_context_class (); g_assert (alc_class); - MonoMethod *m = mono_class_get_method_from_name_checked (alc_class, "MonoResolveUsingResolvingEvent", -1, 0, local_error); + resolve = mono_class_get_method_from_name_checked (alc_class, "MonoResolveUsingResolvingEvent", -1, 0, local_error); mono_error_assert_ok (local_error); - resolve = m; - } + + MONO_STATIC_POINTER_INIT_END (MonoMethod, resolve) + g_assert (resolve); return invoke_resolve_method (resolve, alc, aname, error); @@ -218,16 +218,16 @@ mono_alc_invoke_resolve_using_resolving_event_nofail (MonoAssemblyLoadContext *a static MonoAssembly* mono_alc_invoke_resolve_using_resolve_satellite (MonoAssemblyLoadContext *alc, MonoAssemblyName *aname, MonoError *error) { - static MonoMethod *resolve; + MONO_STATIC_POINTER_INIT (MonoMethod, resolve) - if (!resolve) { ERROR_DECL (local_error); MonoClass *alc_class = mono_class_get_assembly_load_context_class (); g_assert (alc_class); - MonoMethod *m = mono_class_get_method_from_name_checked (alc_class, "MonoResolveUsingResolveSatelliteAssembly", -1, 0, local_error); + resolve = mono_class_get_method_from_name_checked (alc_class, "MonoResolveUsingResolveSatelliteAssembly", -1, 0, local_error); mono_error_assert_ok (local_error); - resolve = m; - } + + MONO_STATIC_POINTER_INIT_END (MonoMethod, resolve) + g_assert (resolve); return invoke_resolve_method (resolve, alc, aname, error); diff --git a/src/mono/mono/metadata/boehm-gc.c b/src/mono/mono/metadata/boehm-gc.c index 74aead5..177b956 100644 --- a/src/mono/mono/metadata/boehm-gc.c +++ b/src/mono/mono/metadata/boehm-gc.c @@ -1229,13 +1229,14 @@ mono_gc_toggleref_register_callback (MonoToggleRefStatus (*proccess_toggleref) ( static MonoToggleRefStatus test_toggleref_callback (MonoObject *obj) { - static MonoClassField *mono_toggleref_test_field; MonoToggleRefStatus status = MONO_TOGGLE_REF_DROP; - if (!mono_toggleref_test_field) { + MONO_STATIC_POINTER_INIT (MonoClassField, mono_toggleref_test_field) + mono_toggleref_test_field = mono_class_get_field_from_name_full (mono_object_class (obj), "__test", NULL); g_assert (mono_toggleref_test_field); - } + + MONO_STATIC_POINTER_INIT_END (MonoClassField*, mono_toggleref_test_field) mono_field_get_value_internal (obj, mono_toggleref_test_field, &status); printf ("toggleref-cb obj %d\n", status); diff --git a/src/mono/mono/metadata/class-init.c b/src/mono/mono/metadata/class-init.c index 7bdd00a..0ee253c 100644 --- a/src/mono/mono/metadata/class-init.c +++ b/src/mono/mono/metadata/class-init.c @@ -2717,6 +2717,7 @@ print_unimplemented_interface_method_info (MonoClass *klass, MonoClass *ic, Mono static MonoMethod* mono_class_get_virtual_methods (MonoClass* klass, gpointer *iter) { + // FIXME move state to caller gboolean static_iter = FALSE; if (!iter) diff --git a/src/mono/mono/metadata/class-internals.h b/src/mono/mono/metadata/class-internals.h index 92bdc67..db28fe8 100644 --- a/src/mono/mono/metadata/class-internals.h +++ b/src/mono/mono/metadata/class-internals.h @@ -15,6 +15,7 @@ #include "mono/utils/mono-error.h" #include "mono/sgen/gc-internal-agnostic.h" #include "mono/utils/mono-error-internals.h" +#include "mono/utils/mono-memory-model.h" #define MONO_CLASS_IS_ARRAY(c) (m_class_get_rank (c)) @@ -1004,6 +1005,10 @@ MonoClass* mono_class_get_##shortname##_class (void); #define GENERATE_TRY_GET_CLASS_WITH_CACHE_DECL(shortname) \ MonoClass* mono_class_try_get_##shortname##_class (void); +// GENERATE_GET_CLASS_WITH_CACHE attempts mono_class_load_from_name whenever +// its cache is null. i.e. potentially repeatedly, though it is expected to succeed +// the first time. +// #define GENERATE_GET_CLASS_WITH_CACHE(shortname,name_space,name) \ MonoClass* \ mono_class_get_##shortname##_class (void) \ @@ -1012,12 +1017,18 @@ mono_class_get_##shortname##_class (void) \ MonoClass *klass = tmp_class; \ if (!klass) { \ klass = mono_class_load_from_name (mono_defaults.corlib, name_space, name); \ - mono_memory_barrier (); \ + mono_memory_barrier (); /* FIXME excessive? */ \ tmp_class = klass; \ } \ return klass; \ } +// GENERATE_TRY_GET_CLASS_WITH_CACHE attempts mono_class_load_from_name approximately +// only once. i.e. if it fails, it will return null and not retry. +// In a race it might try a few times, but not indefinitely. +// +// FIXME This maybe has excessive volatile/barriers. +// #define GENERATE_TRY_GET_CLASS_WITH_CACHE(shortname,name_space,name) \ MonoClass* \ mono_class_try_get_##shortname##_class (void) \ @@ -1507,6 +1518,35 @@ mono_method_is_constructor (MonoMethod *method); gboolean mono_class_has_default_constructor (MonoClass *klass, gboolean public_only); +// There are many ways to do on-demand initialization. +// Some allow multiple concurrent initializations. Some do not. +// Some allow multiple concurrent writes to the global. Some do not. +// +// Booleans or names capturing these factors would be desirable. +// RacyInit? +// +// This form allows both such races, on the understanding that, +// even if the initialization occurs multiple times, every result is equivalent, +// and the goal is not to initialize no more than once, but for the steady state +// to stop rerunning the initialization. +// +// It may be desirable to replace this with mono_lazy_initialize, etc. +// +// These macros cannot be wrapped in do/while as they inject "name" into invoking scope. +// +#define MONO_STATIC_POINTER_INIT(type, name) \ + static type *static_ ## name; \ + type *name; \ + name = static_ ## name; \ + if (!name) { \ + /* Custom code here to initialize name */ +#define MONO_STATIC_POINTER_INIT_END(type, name) \ + if (name) { \ + /* Success, commit to static. */ \ + mono_atomic_store_seq (&static_ ## name, name); \ + } \ + } \ + // Enum and static storage for JIT icalls. #include "jit-icall-reg.h" -- 2.7.4