From 2993f6a34340b03f27b2e94859a9ca1625192622 Mon Sep 17 00:00:00 2001 From: Ryan Lucia Date: Sat, 3 Aug 2019 14:47:38 -0400 Subject: [PATCH] [netcore] Enable ALC loaded_assemblies (mono/mono#15850) * Typo * Enable loaded_assemblies and lock * Make mono_domain_assembly_open_internal ALC-aware * Populate loaded_assemblies Switch from prepend to append * Some notes * Exclude add_assembly_to_alc on non-netcore * Cleanup appdomain.c * Remove mono_alc_cleanup for now * Update comment * Disable using loaded_assemblies in the search hook * Adjust alc cleanup Commit migrated from https://github.com/mono/mono/commit/848811e4b26519ce62672886813f6eb73d572be5 --- src/mono/mono/metadata/appdomain.c | 62 +++++++++++++++++++++++++- src/mono/mono/metadata/appdomain.h | 2 +- src/mono/mono/metadata/assembly-load-context.c | 42 +++++++++++++++++ src/mono/mono/metadata/assembly.c | 3 +- src/mono/mono/metadata/domain-internals.h | 3 ++ src/mono/mono/metadata/domain.c | 24 +++++++--- src/mono/mono/metadata/loader-internals.h | 9 +++- src/mono/mono/metadata/sre.c | 7 +++ src/mono/mono/metadata/w32process.c | 2 + src/mono/mono/mini/driver.c | 4 +- 10 files changed, 145 insertions(+), 13 deletions(-) diff --git a/src/mono/mono/metadata/appdomain.c b/src/mono/mono/metadata/appdomain.c index faf4821..c41c1a4 100644 --- a/src/mono/mono/metadata/appdomain.c +++ b/src/mono/mono/metadata/appdomain.c @@ -128,6 +128,9 @@ mono_domain_asmctx_from_path (const char *fname, MonoAssembly *requesting_assemb static void add_assemblies_to_domain (MonoDomain *domain, MonoAssembly *ass, GHashTable *hash); +static void +add_assembly_to_alc (MonoAssemblyLoadContext *alc, MonoAssembly *ass); + static MonoAppDomainHandle mono_domain_create_appdomain_internal (char *friendly_name, MonoAppDomainSetupHandle setup, MonoError *error); @@ -1448,6 +1451,43 @@ add_assemblies_to_domain (MonoDomain *domain, MonoAssembly *ass, GHashTable *ht) g_hash_table_destroy (ht); } +#ifdef ENABLE_NETCORE +static void +add_assembly_to_alc (MonoAssemblyLoadContext *alc, MonoAssembly *ass) +{ + gint i; + GSList *tmp; + + g_assert (ass != NULL); + + if (!ass->aname.name) + return; + + mono_alc_assemblies_lock (alc); + + for (tmp = alc->loaded_assemblies; tmp; tmp = tmp->next) { + if (tmp->data == ass) { + mono_alc_assemblies_unlock (alc); + return; + } + } + + mono_assembly_addref (ass); + // Prepending here will break the test suite with frequent InvalidCastExceptions, so we have to append + alc->loaded_assemblies = g_slist_append (alc->loaded_assemblies, ass); + mono_trace (G_LOG_LEVEL_INFO, MONO_TRACE_ASSEMBLY, "Assembly %s[%p] added to ALC (%p), ref_count=%d", ass->aname.name, ass, (gpointer)alc, ass->ref_count); + + if (ass->image->references) { + for (i = 0; i < ass->image->nreferences; i++) { + // TODO: remove all this after we're confident this assert isn't hit + g_assertf (!ass->image->references [i], "Did not expect reference %d of %s to be resolved", i, ass->image->name); + } + } + + mono_alc_assemblies_unlock (alc); +} +#endif + static void mono_domain_fire_assembly_load (MonoAssembly *assembly, gpointer user_data) { @@ -1456,6 +1496,7 @@ mono_domain_fire_assembly_load (MonoAssembly *assembly, gpointer user_data) static MonoMethod *assembly_load_method; ERROR_DECL (error); MonoDomain *domain = mono_domain_get (); + MonoAssemblyLoadContext *alc = mono_domain_ambient_alc (domain); // FIXME: pass alc via mono_assembly_invoke_load_hook MonoClass *klass; MonoObjectHandle appdomain; @@ -1474,6 +1515,9 @@ mono_domain_fire_assembly_load (MonoAssembly *assembly, gpointer user_data) mono_domain_assemblies_lock (domain); add_assemblies_to_domain (domain, assembly, NULL); mono_domain_assemblies_unlock (domain); +#ifdef ENABLE_NETCORE + add_assembly_to_alc (alc, assembly); +#endif if (assembly_load_field == NULL) { assembly_load_field = mono_class_get_field_from_name_full (klass, "AssemblyLoad", NULL); @@ -2332,7 +2376,6 @@ mono_domain_assembly_search (MonoAssemblyLoadContext *alc, MonoAssembly *request MonoError *error) { g_assert (aname != NULL); - MonoDomain *domain = mono_domain_get (); GSList *tmp; MonoAssembly *ass; const gboolean strong_name = aname->public_key_token[0] != 0; @@ -2342,6 +2385,22 @@ mono_domain_assembly_search (MonoAssemblyLoadContext *alc, MonoAssembly *request const MonoAssemblyNameEqFlags eq_flags = (MonoAssemblyNameEqFlags)(strong_name ? MONO_ANAME_EQ_IGNORE_CASE : (MONO_ANAME_EQ_IGNORE_PUBKEY | MONO_ANAME_EQ_IGNORE_VERSION | MONO_ANAME_EQ_IGNORE_CASE)); +// TODO: this is currently broken due to the lack of proper ALC resolution logic and the load hook not using the correct ALC +#if 0 //def ENABLE_NETCORE + mono_alc_assemblies_lock (alc); + for (tmp = alc->loaded_assemblies; tmp; tmp = tmp->next) { + ass = (MonoAssembly *)tmp->data; + g_assert (ass != NULL); + // TODO: Can dynamic assemblies match here for netcore? Also, this ignores case while exact_sn_match does not. + if (assembly_is_dynamic (ass) || !mono_assembly_names_equal_flags (aname, &ass->aname, eq_flags)) + continue; + + mono_alc_assemblies_unlock (alc); + return ass; + } + mono_alc_assemblies_unlock (alc); +#else + MonoDomain *domain = mono_domain_get (); mono_domain_assemblies_lock (domain); for (tmp = domain->domain_assemblies; tmp; tmp = tmp->next) { ass = (MonoAssembly *)tmp->data; @@ -2355,6 +2414,7 @@ mono_domain_assembly_search (MonoAssemblyLoadContext *alc, MonoAssembly *request return ass; } mono_domain_assemblies_unlock (domain); +#endif return NULL; } diff --git a/src/mono/mono/metadata/appdomain.h b/src/mono/mono/metadata/appdomain.h index 65848aa..fc82e19 100644 --- a/src/mono/mono/metadata/appdomain.h +++ b/src/mono/mono/metadata/appdomain.h @@ -103,7 +103,7 @@ mono_domain_from_appdomain (MonoAppDomain *appdomain); MONO_API void mono_domain_foreach (MonoDomainFunc func, void* user_data); -MONO_API MonoAssembly * +MONO_API MONO_RT_EXTERNAL_ONLY MonoAssembly * mono_domain_assembly_open (MonoDomain *domain, const char *name); MONO_API mono_bool diff --git a/src/mono/mono/metadata/assembly-load-context.c b/src/mono/mono/metadata/assembly-load-context.c index 820e4f2..e33ba7f 100644 --- a/src/mono/mono/metadata/assembly-load-context.c +++ b/src/mono/mono/metadata/assembly-load-context.c @@ -22,14 +22,56 @@ mono_alc_init (MonoAssemblyLoadContext *alc, MonoDomain *domain) mono_loaded_images_init (li, alc); alc->domain = domain; alc->loaded_images = li; + alc->loaded_assemblies = NULL; + mono_coop_mutex_init (&alc->assemblies_lock); } void mono_alc_cleanup (MonoAssemblyLoadContext *alc) { + /* + * As it stands, ALC and domain cleanup is probably broken on netcore. Without ALC collectability, this should not + * be hit. I've documented roughly the things that still need to be accomplish, but the implementation is TODO and + * the ideal order and locking unclear. + * + * For now, this makes two important assumptions: + * 1. The minimum refcount on assemblies is 2: one for the domain and one for the ALC. The domain refcount might + * be less than optimal on netcore, but its removal is too likely to cause issues for now. + * 2. An ALC will have been removed from the domain before cleanup. + */ + GSList *tmp; + MonoDomain *domain = alc->domain; + + /* + * Missing steps: + * + * + Release GC roots for all assemblies in the ALC + * + Iterate over the domain_assemblies and remove ones that belong to the ALC, which will probably require + * converting domain_assemblies to a doubly-linked list, ideally GQueue + * + Close dynamic and then remaining assemblies, potentially nulling the data field depending on refcount + * + Second pass to call mono_assembly_close_finish on remaining assemblies + * + Free the loaded_assemblies list itself + */ + + alc->loaded_assemblies = NULL; + mono_coop_mutex_destroy (&alc->assemblies_lock); + mono_loaded_images_free (alc->loaded_images); + + g_assert_not_reached (); } +void +mono_alc_assemblies_lock (MonoAssemblyLoadContext *alc) +{ + mono_coop_mutex_lock (&alc->assemblies_lock); +} + +void +mono_alc_assemblies_unlock (MonoAssemblyLoadContext *alc) +{ + mono_coop_mutex_unlock (&alc->assemblies_lock); +} gpointer ves_icall_System_Runtime_Loader_AssemblyLoadContext_InternalInitializeNativeALC (gpointer this_gchandle_ptr, MonoBoolean is_default_alc, MonoBoolean collectible, MonoError *error) diff --git a/src/mono/mono/metadata/assembly.c b/src/mono/mono/metadata/assembly.c index c09dde6..0430195 100644 --- a/src/mono/mono/metadata/assembly.c +++ b/src/mono/mono/metadata/assembly.c @@ -764,6 +764,7 @@ assembly_names_compare_versions (MonoAssemblyName *l, MonoAssemblyName *r, int m void mono_assembly_request_prepare (MonoAssemblyLoadRequest *req, size_t req_size, MonoAssemblyContextKind asmctx) { + // TODO: Shouldn't this be setting an ALC? Seems almost none of the callers do. memset (req, 0, req_size); req->asmctx = asmctx; } @@ -2985,7 +2986,7 @@ mono_assembly_request_load_from (MonoImage *image, const char *fname, } } - /* We need to check for ReferenceAssmeblyAttribute before we + /* We need to check for ReferenceAssemblyAttribute before we * mark the assembly as loaded and before we fire the load * hook. Otherwise mono_domain_fire_assembly_load () in * appdomain.c will cache a mapping from the assembly name to diff --git a/src/mono/mono/metadata/domain-internals.h b/src/mono/mono/metadata/domain-internals.h index 9c1de42..d92a35a 100644 --- a/src/mono/mono/metadata/domain-internals.h +++ b/src/mono/mono/metadata/domain-internals.h @@ -630,6 +630,9 @@ mono_domain_parse_assembly_bindings (MonoDomain *domain, int amajor, int aminor, gboolean mono_assembly_name_parse (const char *name, MonoAssemblyName *aname); +MonoAssembly * +mono_domain_assembly_open_internal (MonoDomain *domain, MonoAssemblyLoadContext *alc, const char *name); + MonoImage *mono_assembly_open_from_bundle (MonoAssemblyLoadContext *alc, const char *filename, MonoImageOpenStatus *status, diff --git a/src/mono/mono/metadata/domain.c b/src/mono/mono/metadata/domain.c index 27447d4..72907b7 100644 --- a/src/mono/mono/metadata/domain.c +++ b/src/mono/mono/metadata/domain.c @@ -127,9 +127,6 @@ get_runtimes_from_exe (const char *exe_file, MonoImage **exe_image); static const MonoRuntimeInfo* get_runtime_by_version (const char *version); -MonoAssembly * -mono_domain_assembly_open_internal (MonoDomain *domain, const char *name); - static void mono_domain_alcs_destroy (MonoDomain *domain); @@ -1024,13 +1021,14 @@ mono_domain_assembly_open (MonoDomain *domain, const char *name) { MonoAssembly *result; MONO_ENTER_GC_UNSAFE; - result = mono_domain_assembly_open_internal (domain, name); + result = mono_domain_assembly_open_internal (domain, mono_domain_default_alc (domain), name); MONO_EXIT_GC_UNSAFE; return result; } +// Uses the domain on legacy mono and the ALC on current MonoAssembly * -mono_domain_assembly_open_internal (MonoDomain *domain, const char *name) +mono_domain_assembly_open_internal (MonoDomain *domain, MonoAssemblyLoadContext *alc, const char *name) { MonoDomain *current; MonoAssembly *ass; @@ -1038,6 +1036,17 @@ mono_domain_assembly_open_internal (MonoDomain *domain, const char *name) MONO_REQ_GC_UNSAFE_MODE; +#ifdef ENABLE_NETCORE + mono_alc_assemblies_lock (alc); + for (tmp = alc->loaded_assemblies; tmp; tmp = tmp->next) { + ass = (MonoAssembly *)tmp->data; + if (strcmp (name, ass->aname.name) == 0) { + mono_alc_assemblies_unlock (alc); + return ass; + } + } + mono_alc_assemblies_unlock (alc); +#else mono_domain_assemblies_lock (domain); for (tmp = domain->domain_assemblies; tmp; tmp = tmp->next) { ass = (MonoAssembly *)tmp->data; @@ -1047,10 +1056,11 @@ mono_domain_assembly_open_internal (MonoDomain *domain, const char *name) } } mono_domain_assemblies_unlock (domain); +#endif MonoAssemblyOpenRequest req; mono_assembly_request_prepare (&req.request, sizeof (req), MONO_ASMCTX_DEFAULT); - req.request.alc = mono_domain_default_alc (domain); + req.request.alc = alc; if (domain != mono_domain_get ()) { current = mono_domain_get (); @@ -1081,6 +1091,8 @@ unregister_vtable_reflection_type (MonoVTable *vtable) * This releases the resources associated with the specific domain. * This is a low-level function that is invoked by the AppDomain infrastructure * when necessary. + * + * In theory, this is dead code on netcore and thus does not need to be ALC-aware. */ void mono_domain_free (MonoDomain *domain, gboolean force) diff --git a/src/mono/mono/metadata/loader-internals.h b/src/mono/mono/metadata/loader-internals.h index 5d4533f..d240f6a 100644 --- a/src/mono/mono/metadata/loader-internals.h +++ b/src/mono/mono/metadata/loader-internals.h @@ -10,6 +10,7 @@ #include #include #include +#include typedef struct _MonoLoadedImages MonoLoadedImages; typedef struct _MonoAssemblyLoadContext MonoAssemblyLoadContext; @@ -19,10 +20,8 @@ typedef struct _MonoAssemblyLoadContext MonoAssemblyLoadContext; struct _MonoAssemblyLoadContext { MonoDomain *domain; MonoLoadedImages *loaded_images; -#if 0 GSList *loaded_assemblies; MonoCoopMutex assemblies_lock; -#endif /* Handle of the corresponding managed object. If the ALC is * collectible, the handle is weak, otherwise it's strong. */ @@ -43,6 +42,12 @@ mono_alc_init (MonoAssemblyLoadContext *alc, MonoDomain *domain); void mono_alc_cleanup (MonoAssemblyLoadContext *alc); +void +mono_alc_assemblies_lock (MonoAssemblyLoadContext *alc); + +void +mono_alc_assemblies_unlock (MonoAssemblyLoadContext *alc); + static inline MonoDomain * mono_alc_domain (MonoAssemblyLoadContext *alc) { diff --git a/src/mono/mono/metadata/sre.c b/src/mono/mono/metadata/sre.c index 2b48867..f0468ab 100644 --- a/src/mono/mono/metadata/sre.c +++ b/src/mono/mono/metadata/sre.c @@ -1309,6 +1309,7 @@ mono_reflection_dynimage_basic_init (MonoReflectionAssemblyBuilder *assemblyb, M MonoDynamicAssembly *assembly; MonoDynamicImage *image; MonoDomain *domain = mono_object_domain (assemblyb); + MonoAssemblyLoadContext *alc = mono_domain_default_alc (domain); if (assemblyb->dynamic_assembly) return; @@ -1373,6 +1374,12 @@ mono_reflection_dynimage_basic_init (MonoReflectionAssemblyBuilder *assemblyb, M mono_domain_assemblies_lock (domain); domain->domain_assemblies = g_slist_append (domain->domain_assemblies, assembly); +#ifdef ENABLE_NETCORE + // TODO: potentially relax the locking here? + mono_alc_assemblies_lock (alc); + alc->loaded_assemblies = g_slist_append (alc->loaded_assemblies, assembly); + mono_alc_assemblies_unlock (alc); +#endif mono_domain_assemblies_unlock (domain); register_assembly (mono_object_domain (assemblyb), &assemblyb->assembly, &assembly->assembly); diff --git a/src/mono/mono/metadata/w32process.c b/src/mono/mono/metadata/w32process.c index 442054f..b386cd6 100644 --- a/src/mono/mono/metadata/w32process.c +++ b/src/mono/mono/metadata/w32process.c @@ -423,6 +423,8 @@ get_domain_assemblies (MonoDomain *domain) for (tmp = domain->domain_assemblies; tmp; tmp = tmp->next) { MonoAssembly *ass = (MonoAssembly *)tmp->data; + // TODO: the reasoning behind this check being used is unclear to me. Maybe replace with mono_domain_get_assemblies()? + // Added in https://github.com/mono/mono/commit/46dc6758c67528fa7bc5590d1cfb4c119b69bc2b#diff-7017648b60461e8902a36d22782f4a14R451 if (m_image_is_fileio_used (ass->image)) continue; g_ptr_array_add (assemblies, ass); diff --git a/src/mono/mono/mini/driver.c b/src/mono/mono/mini/driver.c index 0e21980..850a34c 100644 --- a/src/mono/mono/mini/driver.c +++ b/src/mono/mono/mini/driver.c @@ -1353,7 +1353,7 @@ static void main_thread_handler (gpointer user_data) /* Treat the other arguments as assemblies to compile too */ for (i = 0; i < main_args->argc; ++i) { - assembly = mono_domain_assembly_open (main_args->domain, main_args->argv [i]); + assembly = mono_domain_assembly_open_internal (main_args->domain, mono_domain_default_alc (main_args->domain), main_args->argv [i]); if (!assembly) { if (mono_is_problematic_file (main_args->argv [i])) { fprintf (stderr, "Info: AOT of problematic assembly %s skipped. This is expected.\n", main_args->argv [i]); @@ -1388,7 +1388,7 @@ static void main_thread_handler (gpointer user_data) } } } else { - assembly = mono_domain_assembly_open (main_args->domain, main_args->file); + assembly = mono_domain_assembly_open_internal (main_args->domain, mono_domain_default_alc (main_args->domain), main_args->file); if (!assembly){ fprintf (stderr, "Can not open image %s\n", main_args->file); exit (1); -- 2.7.4