-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Fixed mono from gc handle #62071
Fixed mono from gc handle #62071
Conversation
…XIT_GC_UNSAFE; mono_runtime_delegate_invoke wrapper with MONO_ENTER_GC_UNSAFE/MONO_EXIT_GC_UNSAFE;
@@ -3714,24 +3714,25 @@ mono_get_delegate_end_invoke_checked (MonoClass *klass, MonoError *error) | |||
MonoObject* | |||
mono_runtime_delegate_invoke (MonoObject *delegate, void **params, MonoObject **exc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove all changes of mono_runtime_delegate_invoke from this PR, this is duplicate change of #62003.
@@ -12,6 +12,7 @@ | |||
|
|||
namespace System.Runtime.Loader | |||
{ | |||
[StructLayout(LayoutKind.Sequential)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this construction is a little problematic and you see the symptoms of it. Just adding LayoutKind.Sequential might fix it, but depending on sequential field order in a partial class that is in this case not even sealed, so don't know what guarantees that will give you of complete class layout. You can also see that CoreCLR depends on a different order, that is not inline with how the fields are declared, but how they are layed out by loader. I believe a better way to fix it is to get rid of MonoManagedAssemblyLoadContext
and fix mono_alc_from_gchandle
to use a cached offset to the nativeAssemblyLoadContext field and then access it based on the offset instead of depending on field order in the class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, thank you, i will try later
fix: #62052