[netcore] Enable ALC loaded_assemblies (mono/mono#15850)
authorRyan Lucia <rylucia@microsoft.com>
Sat, 3 Aug 2019 18:47:38 +0000 (14:47 -0400)
committerMarek Safar <marek.safar@gmail.com>
Sat, 3 Aug 2019 18:47:38 +0000 (20:47 +0200)
* 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
src/mono/mono/metadata/appdomain.h
src/mono/mono/metadata/assembly-load-context.c
src/mono/mono/metadata/assembly.c
src/mono/mono/metadata/domain-internals.h
src/mono/mono/metadata/domain.c
src/mono/mono/metadata/loader-internals.h
src/mono/mono/metadata/sre.c
src/mono/mono/metadata/w32process.c
src/mono/mono/mini/driver.c

index faf4821..c41c1a4 100644 (file)
@@ -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;
 }
index 65848aa..fc82e19 100644 (file)
@@ -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
index 820e4f2..e33ba7f 100644 (file)
@@ -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)
index c09dde6..0430195 100644 (file)
@@ -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
index 9c1de42..d92a35a 100644 (file)
@@ -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,
index 27447d4..72907b7 100644 (file)
@@ -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)
index 5d4533f..d240f6a 100644 (file)
@@ -10,6 +10,7 @@
 #include <mono/metadata/object-forward.h>
 #include <mono/utils/mono-forward.h>
 #include <mono/utils/mono-error.h>
+#include <mono/utils/mono-coop-mutex.h>
 
 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)
 {
index 2b48867..f0468ab 100644 (file)
@@ -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);
index 442054f..b386cd6 100644 (file)
@@ -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);
index 0e21980..850a34c 100644 (file)
@@ -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);