Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mono][aot] Load AOT module of a container assembly using assembly name #83511

Merged
merged 8 commits into from
Mar 20, 2023

Conversation

kotlarmilos
Copy link
Member

This PR should fix loading a container assembly on Mac Catalyst. It loads container AOT module using its name, without an actual assembly in the bundle.

This should simplify the build process as the container assembly might not be required.

@kotlarmilos
Copy link
Member Author

@vargaz do you see any limitations to the proposed approach?

@vargaz
Copy link
Contributor

vargaz commented Mar 16, 2023

Looks good, the MonoAssembly/MonoImage might need more initialization, will see.

Comment on lines 2448 to 2452
MonoAssembly *assm = g_new0 (MonoAssembly, 1);
assm->image = g_new0 (MonoImage, 1);
assm->image->dynamic = 0;
assm->image->alc = alc;
assm->aname.name = container_assm_name;
Copy link
Member

@lambdageek lambdageek Mar 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to increment the refcount of the MonoImage.

In general, this should be like a fake version of

MonoAssembly *
mono_assembly_request_load_from (MonoImage *image, const char *fname,
				 const MonoAssemblyLoadRequest *req,
				   MonoImageOpenStatus *status)

and of

static MonoImage *
do_mono_image_open (MonoAssemblyLoadContext *alc, const char *fname, MonoImageOpenStatus *status, const MonoImageOpenOptions *options)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can mono_assembly_addref be used to increment reference count?

Do you think that something else should be added from mono_assembly_request_load_from and do_mono_image_open?

@kotlarmilos
Copy link
Member Author

It seems that more initialization is needed. Unfortunately, error logs can't be found on the CI as it fails during the initialization. I will try to reproduce it locally.

@vargaz
Copy link
Contributor

vargaz commented Mar 16, 2023

mono_dynamic_image_create () contains some initialization code. In particular, mono_image_init () should be called.

@@ -2443,13 +2443,16 @@ load_container_amodule (MonoAssemblyLoadContext *alc)
if (!container_assm_name || container_amodule)
return;

char *local_ref = container_assm_name;
container_assm_name = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without locking, there is still a race here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would mono_aot_lock help?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should use mono_loader_lock (), thats our global lock.


mono_loader_lock ();
// There might be several threads that passed the first check
// Adding another check to ensure single load of a container assembly due to race condition
if (!container_amodule) {
char *local_ref = container_assm_name;
Copy link
Contributor

@vargaz vargaz Mar 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be needed any more.
The locking should be:

if (container_amodule)
   return;
mono_loader_lock ();
if (!container_amodule) {
   <rest>
  mono_memory_barrier ();
  container_amodule = assm;
}
mono_loader_unlock ();

Copy link
Member Author

@kotlarmilos kotlarmilos Mar 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like threads are not in sync without container_assm_name check.

if (!container_assm_name || container_amodule)
		return;

The library tests fail at g_assert (assm->image->aot_module); as it is NULL.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the following is used to prevent recursive calls within the same thread.

char *local_ref = container_assm_name;
container_assm_name = NULL;

When load_container_amodule is invoked initially, load_aot_module is invoked with the fake container assembly and image. load_aot_module invokes load_image that indirectly invokes mono_aot_get_method for OnLoadAssembly method. Then, load_container_amodule is invoked again within the same thread, and assumption is that it can enter the lock.

As OnLoadAssembly method is not emitted into the container AOT module, it is not needed to get the method.

Copy link
Member Author

@kotlarmilos kotlarmilos Mar 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vargaz According to the findings above, is the mono_loader_lock still needed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, its still needed, load_aot_module () is not thread safe since it assumes its being called from the assembly loader code which does all kinds of locking.

@kotlarmilos
Copy link
Member Author

Failing tests shouldn't be related.

@kotlarmilos kotlarmilos merged commit 1e601c5 into dotnet:main Mar 20, 2023
@kotlarmilos kotlarmilos deleted the bugfix/load_container_amodule branch March 20, 2023 16:22
@SamMonoRT
Copy link
Member

@lewing @vargaz - With dedup enabled by default on WASM, do you think the change is risky enough to merge just a couple hours prior to P3 snap ? If you believe so, we should revert the merge and re-merge tomorrow am.

@vargaz
Copy link
Contributor

vargaz commented Mar 20, 2023

I guess we should revert just to be safe.

@kotlarmilos
Copy link
Member Author

I guess we should revert just to be safe.

@vargaz can we merge it without tests as it might take up to 2h?

#83686

vargaz pushed a commit that referenced this pull request Mar 20, 2023
@kotlarmilos kotlarmilos restored the bugfix/load_container_amodule branch March 21, 2023 07:39
@kotlarmilos kotlarmilos deleted the bugfix/load_container_amodule branch March 21, 2023 15:50
@ghost ghost locked as resolved and limited conversation to collaborators Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants