-
Notifications
You must be signed in to change notification settings - Fork 507
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
MSR: Create instances of NSObjects and INativeObjects without using reflection #18519
MSR: Create instances of NSObjects and INativeObjects without using reflection #18519
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
Nifty idea, well done. I have some comments and some performance nits.
} else { | ||
type.IsPublic = true; | ||
} else if (!type.IsPublic) { | ||
type.IsNotPublic = true; |
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 don't understand this. If it's not public, why are you marking it not public?
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.
We need the type to be at least internal. Unless it's already public, we need to change its visibility attributes. There might be a better way to set the visibility modifier but this worked for me so far.
This comment has been minimized.
This comment has been minimized.
src/ObjCRuntime/Runtime.cs
Outdated
@@ -1368,6 +1455,25 @@ static void MissingCtor (IntPtr ptr, IntPtr klass, Type type, MissingCtorResolut | |||
ctorArguments [1] = owns; | |||
|
|||
return (T?) ctor.Invoke (ctorArguments); | |||
|
|||
#if NET | |||
// This is a workaround for a compiler issue. |
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.
Are you going to leave me curious what this compiler issue is?
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 think the comment is outdated. I was running into an issue where the compiler would crash when T.ConstructINativeObject
or T.ConstructNSObject
was called in the parent method but it was probably caused by something else (or it was in combination with some invalid IL that I generated during earlier stages of development). I don't observe the same crash now that I rebuilt it with "direct" calls to those methods. I'll get rid of the comment and I think I'll also remove the local function.
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.
Update: I've been able to find the issue that led me to move the code into a separate function. Without T.ConstructINativeObject(...)
being called from different function than Runtime.ConstructINativeObject<T>
, for example calling CNContactFormatter.GetDescriptorForRequiredKeys (CNContactFormatterStyle.PhoneticFullName)
causes a crashes with:
2023-06-30 17:11:06.854 MySingleView[90723:11572593] error: * Assertion at /Users/runner/work/1/s/src/mono/mono/mini/mini-generic-sharing.c:2283, condition `m_class_get_vtable (info->klass)' not met
=================================================================
Native Crash Reporting
=================================================================
Got a SIGABRT while executing native code. This usually indicates
a fatal error in the mono runtime or one of the native libraries
used by your application.
=================================================================
=================================================================
Native stacktrace:
=================================================================
0x10362265c - ./xamarin-macios/tests/dotnet/MySingleView/bin/Release/net7.0-maccatalyst/maccatalyst-arm64/MySingleView.app/Contents/MonoBundle/libmonosgen-2.0.dylib : mono_dump_native_crash_info
0x1035e1578 - ./xamarin-macios/tests/dotnet/MySingleView/bin/Release/net7.0-maccatalyst/maccatalyst-arm64/MySingleView.app/Contents/MonoBundle/libmonosgen-2.0.dylib : mono_handle_native_crash
0x1038001a0 - ./xamarin-macios/tests/dotnet/MySingleView/bin/Release/net7.0-maccatalyst/maccatalyst-arm64/MySingleView.app/Contents/MonoBundle/libmonosgen-2.0.dylib : sigabrt_signal_handler.cold.1
0x103621f10 - ./xamarin-macios/tests/dotnet/MySingleView/bin/Release/net7.0-maccatalyst/maccatalyst-arm64/MySingleView.app/Contents/MonoBundle/libmonosgen-2.0.dylib : mono_runtime_setup_stat_profiler
0x18cd26a24 - /usr/lib/system/libsystem_platform.dylib : _sigtramp
0x18ccf7c28 - /usr/lib/system/libsystem_pthread.dylib : pthread_kill
0x18cc05ae8 - /usr/lib/system/libsystem_c.dylib : abort
0x102e39bf0 - ./xamarin-macios/tests/dotnet/MySingleView/bin/Release/net7.0-maccatalyst/maccatalyst-arm64/MySingleView.app/Contents/MonoBundle/libxamarin-dotnet.dylib : _ZL14print_callbackPKci
0x10365e4b8 - ./xamarin-macios/tests/dotnet/MySingleView/bin/Release/net7.0-maccatalyst/maccatalyst-arm64/MySingleView.app/Contents/MonoBundle/libmonosgen-2.0.dylib : monoeg_g_logv_nofree
0x10365e570 - ./xamarin-macios/tests/dotnet/MySingleView/bin/Release/net7.0-maccatalyst/maccatalyst-arm64/MySingleView.app/Contents/MonoBundle/libmonosgen-2.0.dylib : monoeg_assertion_message
0x10365e5b4 - ./xamarin-macios/tests/dotnet/MySingleView/bin/Release/net7.0-maccatalyst/maccatalyst-arm64/MySingleView.app/Contents/MonoBundle/libmonosgen-2.0.dylib : mono_assertion_message_unreachable
0x1035eaa8c - ./xamarin-macios/tests/dotnet/MySingleView/bin/Release/net7.0-maccatalyst/maccatalyst-arm64/MySingleView.app/Contents/MonoBundle/libmonosgen-2.0.dylib : instantiate_info
0x1035e9900 - ./xamarin-macios/tests/dotnet/MySingleView/bin/Release/net7.0-maccatalyst/maccatalyst-arm64/MySingleView.app/Contents/MonoBundle/libmonosgen-2.0.dylib : mini_instantiate_gshared_info
0x103595248 - ./xamarin-macios/tests/dotnet/MySingleView/bin/Release/net7.0-maccatalyst/maccatalyst-arm64/MySingleView.app/Contents/MonoBundle/libmonosgen-2.0.dylib : mini_init_method_rgctx
0x1026c2acc - ./xamarin-macios/tests/dotnet/MySingleView/bin/Release/net7.0-maccatalyst/maccatalyst-arm64/MySingleView.app/Contents/MacOS/MySingleView : ObjCRuntime_Runtime_ConstructINativeObject_T_REF_intptr_bool_System_Type_ObjCRuntime_Runtime_MissingCtorResolution_intptr_System_RuntimeMethodHandle
0x1026c5788 - ./xamarin-macios/tests/dotnet/MySingleView/bin/Release/net7.0-maccatalyst/maccatalyst-arm64/MySingleView.app/Contents/MacOS/MySingleView : ObjCRuntime_Runtime_GetINativeObject_T_REF_intptr_bool_System_Type_bool_intptr_System_RuntimeMethodHandle
0x1026c4860 - ./xamarin-macios/tests/dotnet/MySingleView/bin/Release/net7.0-maccatalyst/maccatalyst-arm64/MySingleView.app/Contents/MacOS/MySingleView : ObjCRuntime_Runtime_GetINativeObject_T_REF_intptr_bool_System_Type_bool
0x1026c47e0 - ./xamarin-macios/tests/dotnet/MySingleView/bin/Release/net7.0-maccatalyst/maccatalyst-arm64/MySingleView.app/Contents/MacOS/MySingleView : ObjCRuntime_Runtime_GetINativeObject_T_REF_intptr_bool_bool
0x1026c4768 - ./xamarin-macios/tests/dotnet/MySingleView/bin/Release/net7.0-maccatalyst/maccatalyst-arm64/MySingleView.app/Contents/MacOS/MySingleView : ObjCRuntime_Runtime_GetINativeObject_T_REF_intptr_bool
0x1026b6448 - ./xamarin-macios/tests/dotnet/MySingleView/bin/Release/net7.0-maccatalyst/maccatalyst-arm64/MySingleView.app/Contents/MacOS/MySingleView : Contacts_CNContactFormatter_GetDescriptorForRequiredKeys_Contacts_CNContactFormatterStyle
...
The comment is still outdated and I'll try to change the wording of the workaround description.
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.
Did you file an issue for this? Seems like an issue in the AOT compiler.
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 haven't done that yet but I will create a follow-up compiler issue.
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.
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
Looking good to me 👍
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
💻 [CI Build] Windows Integration Tests passed 💻✅ All Windows Integration Tests passed. Pipeline on Agent |
📚 [PR Build] Artifacts 📚Packages generatedView packagesPipeline on Agent |
✅ API diff for current PR / commitLegacy Xamarin (No breaking changes)
NET (empty diffs)
❗ API diff vs stable (Breaking changes)Legacy Xamarin (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
.NET (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
💻 [PR Build] Tests on macOS M1 - Mac Ventura (13.0) passed 💻✅ All tests on macOS M1 - Mac Ventura (13.0) passed. Pipeline on Agent |
💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) passed 💻✅ All tests on macOS M1 - Mac Big Sur (11.5) passed. Pipeline on Agent |
🚀 [CI Build] Test results 🚀Test results✅ All tests passed on VSTS: simulator tests. 🎉 All 235 tests passed 🎉 Tests counts✅ bcl: All 69 tests passed. Html Report (VSDrops) Download Pipeline on Agent |
Closes #18358
This PR adds lookup tables and factory methods for INativeObjects and NSObjects. These generated methods allow us to create instances of these objects without needing reflection.