- Notifications
You must be signed in to change notification settings - Fork564
[native] Better p/invoke handling when loading foreign libraries#10165
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
d80648e to61b8ee2CompareCo-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com>
61b8ee2 to2ef0038CompareThere 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.
Pull Request Overview
This PR improves the handling of foreign libraries loaded via p/invoke by ensuring that library names match the expected system naming conventions. Key changes include:
- Adding an ends_with() utility function to check string suffixes in the runtime-base strings utilities.
- Simplifying the library path resolution logic in AndroidSystem and updating DSO loading via a templated monodroid_dlopen.
- Introducing a new constant DSO_PREFIX and a utility function to detect directory components in paths.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/native/common/include/runtime-base/strings.hh | Added an ends_with() helper for string suffix checking. |
| src/native/clr/runtime-base/android-system.cc | Refactored library path resolution and suffix handling. |
| src/native/clr/include/runtime-base/util.hh | Added a utility to check for directory separators in paths. |
| src/native/clr/include/runtime-base/monodroid-dl.hh | Updated monodroid_dlopen to a templated version using a flag. |
| src/native/clr/include/host/pinvoke-override.hh | Adjusted p/invoke override to use the new templated API. |
| src/native/clr/include/constants.hh | Introduced DSO_PREFIX for consistent library name handling. |
Comments suppressed due to low confidence (1)
src/native/clr/include/runtime-base/monodroid-dl.hh:109
- [nitpick] Consider adding a clarifying comment explaining the purpose and expected usage of the template parameter PREFER_AOT_CACHE to aid future maintainers in understanding this API change.
template<bool PREFER_AOT_CACHE> [[gnu::flatten]]Uh oh!
There was an error while loading.Please reload this page.
| returnfalse; | ||
| } | ||
| returnmemcmp (buffer.get () + (length () - s.length ()), s.data (), s.length ()) ==0; |
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.
Just for me to understand,length() is always inlined, so no problem with calling it multiple times in an expression?
android/src/native/common/include/runtime-base/strings.hh
Lines 452 to 453 in2ef0038
| [[gnu::always_inline]] | |
| autolength ()constnoexcept -> size_t |
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.
Yeah, it's fully inlined each time it's called (which can also the be further optimized by common sub-expression elimination and other techniques, depending on context)
9f8e3d4 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Improve looking for "foreign" libraries when resolving a p/invoke symbol.
Foreign libraries are those which either aren't contained in the application APK
or which reside on the device in a system location. In such cases, we must attempt
to load the library using its name passed from the managed code (which uses the
DllImportattribute to name the library).Since
DllImportdoesn't usually use full name of the target library (e.g. insteadof
liblog.soit will use eitherliblogorlog), we must modify the libraryname so that the
dlopen(3)call can find it on the filesystem. We must make surethat the library name follows the
libNAME.soconvention before attempting to loadthe library from filesystem..
This is an interim fix for us not finding e.g. the
liblog.solibrary when the[DllImport("liblog")]attribute is used in the managed code. The library resolutionand reloading system must be redesigned, which will happen in a separate PR as it is
currently not the highest priority task.