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] Introduce designated direct pinvokes to mono aot compiler #79721

Merged
merged 38 commits into from Feb 2, 2023

Conversation

mdh1418
Copy link
Member

@mdh1418 mdh1418 commented Dec 15, 2022

Contributes to #79377

On Mono, the Managed/native Interop story does not facilitate a granular specification of which native libraries or entry points for generating direct pinvoke calls. Instead, there was only a blanket boolean flag to handle direct pinvoke logic. On the other hand, Native AOT allows specification of which unmanaged libraries, entrypoints, or even files containing a list of such unmanaged libraries and entrypoints to generate direct pinvoke calls. To expand the capabilities on mono, similar logic can be introduced to the mono aot compiler.

This PR does the following:

  • Introduce direct-pinvokes and direct-pinvoke-list parameters to the mono aot compiler, hooked in by the MonoAOTCompiler Task
  • Processes the passed in direct_pinvokes and direct_pinvoke_list values to generate a Hash Table mapping of modules:symbols-list to determine direct pinvoke calls
  • Replaces the blanket direct-pinvoke flag logic with mono_aot_direct_pinvoke_enabled_for_method that compares the method being aot compiled with the direct-pinvoke Hash Table mapping

Copy link
Member

@lateralusX lateralusX left a comment

Choose a reason for hiding this comment

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

Did a first pass, but didn't complete it full since there is already a bunch of things to look over.

A couple of things I had in mind around the organization of data in hash tables:

We always add a * to the hash table, I guess the intent should be all pinvokes, like the bool we had in the pass. I guess it shouldn't be added unless a user uses a pinvoke entry with *? Maybe we should optimize that use case a little since its the old use case that we still need to support, direct-pinvoke or we break command line combability, so I suggest that you keep the bool and it can be set with the direct-pinvoke flag as it used to do and you have that as part of the check to speed up old scenario where you enable direct-pinvoke for all dllimports. The same would apply if you find a direct-pinvokes=* or a line in the file with *, that would toggle on the old bool flag, if you combine those you should probably get a warning that all dllimports will be direct-pinvoke, since * will override any specific module or module!entrypoint records.

Maybe we should add an optimization for all entry points in a module as well since I believe that will be common use case as well, so if you have just module, that should make sure you override any specific entry points and you should probably issue a warning if you have a combination of module and module!entrypoints. That could be done by holding a struct in the hash table with a bool and a pointer to the hash table for entry points, if you just use module meaning all entrypoints, then add a struct with bool set to true (and don't allocate hash table) and later validate records and warn if you have bool set to true and a hash table allocated as well. That way you can quickly find out if a all entrypoints in a module should be direct-pinvoke or if you need to make another lookup in the modules entrypoint hash table.

src/mono/mono/mini/aot-compiler.c Outdated Show resolved Hide resolved
src/mono/mono/mini/aot-compiler.c Outdated Show resolved Hide resolved
src/mono/mono/mini/aot-compiler.c Outdated Show resolved Hide resolved
src/mono/mono/mini/aot-compiler.c Outdated Show resolved Hide resolved
src/mono/mono/mini/aot-compiler.c Outdated Show resolved Hide resolved
src/mono/mono/mini/aot-compiler.c Outdated Show resolved Hide resolved
src/mono/mono/mini/aot-compiler.c Outdated Show resolved Hide resolved
src/mono/mono/mini/aot-compiler.c Outdated Show resolved Hide resolved
src/mono/mono/mini/aot-compiler.c Show resolved Hide resolved
src/mono/mono/mini/aot-compiler.c Outdated Show resolved Hide resolved
@lateralusX
Copy link
Member

@mdh1418 Fix up the build warnings and get all CI lanes pass. Did you validate everything with a end to end test? Once that is done I believe this is ready to be merged.

@mdh1418
Copy link
Member Author

mdh1418 commented Jan 24, 2023

@lateralusX The end-to-end testing I've done so far was only on iOS device to see whether the unmanaged code's symbol appears in the shared library.
When adding an unmanaged code file with a native function void mitch (void) { print ("Value: %i\n", value);, adding to the managed side

[DllImport("autoinit")]
public static extern void mitch ();

And setting an ItemGroup DirectPInvokes Include="autoinit" to the csproj,

0000000001493590 t _mitch appears from nm artifacts/bin/iOS.Device.Aot-Llvm.Test/Debug/net8.0/ios-arm64/AppBundle/iOS.Device.Aot-Llvm.Test/Debug-iphoneos/lib-iOS.Device.Aot-Llvm.Test.framework/lib-iOS.Device.Aot-Llvm.Test where lib-iOS.Device.Aot-Llvm.Test is the final shared library produced by the library builder.
There was also 00000000013e57b0 t wrapper_managed_to_native_Program_mitch

@mdh1418
Copy link
Member Author

mdh1418 commented Feb 1, 2023

For more testing, proceeded to build a static library based on a source file (hdm.c) containing something like

int
hdm (void)
{
    return 1418;
}

and linked the resulting libhdm.a with its full path hardcoded into

for iOS and for Android


With a test by linking the static library (without defining the appropriate native function, i.e. changing to hdm2) adding HelloiOS to the DirectPInvokes itemgroup, and adding a call to hdm() on the managed side in addition to [DllImport("HelloiOS")]\nprivate static extern int hdm (); I get a linker error as we had discussed, I'm guessing this is the expected error?

/Users/mihw/runtime/mixed_reality_direct_pinvoke/mixed_reality_direct_pinvoke_ios/src/mono/sample/iOS/Program.csproj(88,5): error MSB4018: Undefined symbols for architecture arm64:
/Users/mihw/runtime/mixed_reality_direct_pinvoke/mixed_reality_direct_pinvoke_ios/src/mono/sample/iOS/Program.csproj(88,5): error MSB4018:   "_hdm", referenced from:
/Users/mihw/runtime/mixed_reality_direct_pinvoke/mixed_reality_direct_pinvoke_ios/src/mono/sample/iOS/Program.csproj(88,5): error MSB4018:       wrapper_managed_to_native_Program_hdm in Program.dll.o
/Users/mihw/runtime/mixed_reality_direct_pinvoke/mixed_reality_direct_pinvoke_ios/src/mono/sample/iOS/Program.csproj(88,5): error MSB4018:      (maybe you meant: _p_24_plt_Program_Program_hdm_llvm)
/Users/mihw/runtime/mixed_reality_direct_pinvoke/mixed_reality_direct_pinvoke_ios/src/mono/sample/iOS/Program.csproj(88,5): error MSB4018: ld: symbol(s) not found for architecture arm64
/Users/mihw/runtime/mixed_reality_direct_pinvoke/mixed_reality_direct_pinvoke_ios/src/mono/sample/iOS/Program.csproj(88,5): error MSB4018: clang: error: linker command failed with exit code 1 (use -v to see invocation)

iOS

When properly adding the function declaration to the source file that is built into a static library that is linked into the final shared lib/executable, I was able to run the app with the expected results on iOS.

Android

Though having the same steps as iOS, Android seems to hit other errors and has an app crash.
(mono-rt critical) [ERROR] FATAL UNHANDLED EXCEPTION: System.InvalidProgramException: Invalid IL code in (wrapper managed-to-native) Program:hdm (): IL_004c: ret https://gist.github.com/mdh1418/bed70895d5c6c2bb2ffad057a1fde8ef

@steveisok
Copy link
Member

Android

Though having the same steps as iOS, Android seems to hit other errors and has an app crash. (mono-rt critical) [ERROR] FATAL UNHANDLED EXCEPTION: System.InvalidProgramException: Invalid IL code in (wrapper managed-to-native) Program:hdm (): IL_004c: ret https://gist.github.com/mdh1418/bed70895d5c6c2bb2ffad057a1fde8ef

From my testing, I believe this has to do with the fallout from the runtime trying to dlopen regardless.

@lateralusX
Copy link
Member

lateralusX commented Feb 1, 2023

Sounds like a different issue, the fact that you get an IL error at runtime indicate that the wrapper gets JIT:ed and not picked up from the AOT module and looking through your log it clearly says that it can't use the static linked AOT module since its full AOT, meaning that the embedding has not set the AOT mode correctly for the runtime in order to run full AOT:

: assembly_preload_hook: System.Private.CoreLib (null) /data/user/0/net.dot.HelloAndroid/files
01-31 08:27:15.404 1381 1419 D DOTNET : (Mono debug) Assembly Loader probing location: '/data/user/0/net.dot.HelloAndroid/files/System.Private.CoreLib.dll'.
01-31 08:27:15.404 1381 1419 D DOTNET : (Mono debug) Image addref System.Private.CoreLib[0x788f76e670] (default ALC) -> /data/user/0/net.dot.HelloAndroid/files/System.Private.CoreLib.dll[0x792f73a250]: 2
01-31 08:27:15.405 1381 1419 D DOTNET : (Mono debug) Prepared to set up assembly 'System.Private.CoreLib' (/data/user/0/net.dot.HelloAndroid/files/System.Private.CoreLib.dll)
01-31 08:27:15.405 1381 1419 D DOTNET : (Mono debug) Found statically linked AOT module 'System.Private.CoreLib'.
01-31 08:27:15.405 1381 1419 D DOTNET : Looking for aot data for assembly 'System.Private.CoreLib'.
01-31 08:27:15.405 1381 1419 D DOTNET : Loaded aot data for System.Private.CoreLib.
01-31 08:27:15.405 1381 1419 D DOTNET : (Mono debug) AOT: module System.Private.CoreLib is unusable: compiled with --aot=full.

Looking at the AndroidAppBuilder it looks like it needs both ForceAOT and ForceFullAOT msbuild properties set in order to end up calling:

mono_jit_set_aot_mode(MONO_AOT_MODE_FULL);

Have you made sure you can run you app with the pinvoke on Android using regular JIT?

@mdh1418
Copy link
Member Author

mdh1418 commented Feb 1, 2023

After setting ForceFullAOT on Android, the runtime exception encountered earlier is no longer hit, and the sample app runs to completion as anticipated. For each native function DllImported into the managed side, there are mono debug logs indicating that the wrapper managed-to-native was found. Moreover, in the assembly .dll.s, there are wrapper_managed_to_native_Program_(hdm/mdh) symbols. It seems like the DirectPinvoked module!entrypoint libautoinit!mdh has references in the .dll.s similar to the non DirectPInvoked function hdm that was declared in monodroid.c, I'm guessing theres some mechanism behind how __Internal works? Moreover, when not directPInvoking the method, it then has 20 references in the .dll.s, so there is a difference when adding the DirectPInvoke reference.

@mdh1418
Copy link
Member Author

mdh1418 commented Feb 1, 2023

Unless there are other things to ensure, I think that completes the end-to-end testing for Android and iOS.

@lateralusX lateralusX merged commit 9e8d0a8 into dotnet:main Feb 2, 2023
@mdh1418 mdh1418 deleted the mixed_reality_direct_pinvoke branch February 2, 2023 21:51
@ghost ghost locked as resolved and limited conversation to collaborators Mar 5, 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.

None yet

6 participants