Stop using __Internal in our p/invoke calls
#4757
Conversation
|
Our code will not directly address those p/invoke so the change should be fine for us. Unless there is a specific concern I'm not seeing? |
|
@garuma that's what I thought, but seeing so many Designer tests fail I thought the changes here broke them somehow, thanks! |
|
@garuma: any idea then why custom control use would be disabled? https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=3782292&view=ms.vss-test-web.build-test-results-tab&runId=13618984&resultId=100691&paneView=debug |
|
Looks like it's not resolving |
That's because because the XA runtime is named |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
grendello
changed the title
__Internal__Internal in our p/invoke calls
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Can't merge until designer-side changes are in place. TODO: link to designer-side changes? |
grendello
force-pushed
the
drop-internal
branch
from
740a4ed to
a42a479
Compare
grendello
force-pushed
the
drop-internal
branch
2 times, most recently
from
455fb4d to
e29a0e2
Compare
|
Something else that I've discovered as part of dotnet/java-interop#691 and #5031 is that we also need to migrate away from https://discordapp.com/channels/732297728826277939/732297965019988138/747507619542990938 |
grendello
force-pushed
the
drop-internal
branch
2 times, most recently
from
78fa65f to
0ec1c85
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
The macOS Designer (and Windows Designer) tests fail: |
grendello
force-pushed
the
drop-internal
branch
from
640e063 to
030442e
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Windows designer integration tests are failing: The Thus, the problem is that |
grendello
force-pushed
the
drop-internal
branch
8 times, most recently
from
486ce01 to
30ced74
Compare
| bool name_needs_free = false; | ||
| /* name is nullptr when we're P/Invoking __Internal, so remap to libxa-internal-api */ | ||
| if (name == nullptr) { | ||
| if (name == nullptr || strstr (name, "xa-internal-api") != nullptr) { |
There was a problem hiding this comment.
Should this instead be strstr (name, MONODROID_PATH_SEPARATOR "xa-internal-api") != nullptr, ensuring that we get e.g. /xa-internal-api?
There was a problem hiding this comment.
I'd rather not do that. We can't be sure we'll always be getting a path here. Right now we do get a path (which points to a location where there's no libraries we ship) but it may change and I'd rather be flexible.
src/monodroid/jni/monodroid-glue.cc
Outdated
| const char *last_sep = strrchr (the_path, MONODROID_PATH_SEPARATOR_CHAR); | ||
| if (last_sep != nullptr) { | ||
| tmp_name = utils.strdup_new (the_path, last_sep - the_path); | ||
| tmp_name = utils.string_concat (tmp_name, MONODROID_PATH_SEPARATOR, API_DSO_NAME); |
There was a problem hiding this comment.
This is a memory leak. The preceding line utils.strdup_new() does a new char[…], and utils.string_concat() also returns a new char[…] without a delete[] s1. Consequently, this sequence of statements "loses" the first new char[…] held by tmp_name, causing it to leak memory.
src/monodroid/jni/monodroid-glue.cc
Outdated
| name_needs_free = true; | ||
| } | ||
| } | ||
| #else |
There was a problem hiding this comment.
Should be #else // ndef WINDOWS
src/monodroid/jni/monodroid-glue.cc
Outdated
| // Next lets try the location of the XA runtime DLL, libxa-internal-api.dll should be next to it. | ||
| HMODULE hm; | ||
| const void *func = reinterpret_cast<const void*>(&Java_mono_android_Runtime_initInternal); | ||
| if (GetModuleHandleEx (GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS | GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT, static_cast<LPCSTR>(func), &hm) == 0) { |
There was a problem hiding this comment.
This looks like MonodroidRuntime::get_my_location()? Perhaps that should be used instead?
There was a problem hiding this comment.
heh, forgot that we had that one :)
grendello
force-pushed
the
drop-internal
branch
from
30ced74 to
caf5ff0
Compare
| // First try to see if it exist at the path pointed to by `name`. With p/invokes, currently (Sep 2020), we can't | ||
| // really trust the path since it consists of *some* directory path + p/invoke library name and it does not | ||
| // point to the location where the native library is. However, we still need to check the location first, should | ||
| // it point to the right place in the future. |
There was a problem hiding this comment.
TODO (fix after current PR build completes): to link to: mono/mono#20295 (comment)
src/monodroid/jni/monodroid-glue.cc
Outdated
| bool found = probe_dll_at (name); | ||
| if (!found) { | ||
| // Next lets try the location of the XA runtime DLL, libxa-internal-api.dll should be next to it. | ||
| const char *path = get_my_location (); |
There was a problem hiding this comment.
Silly question: why does get_my_location() return a const char* when we're supposed to free() it? Isn't convention to return a non-const type for values which should be freed?
There was a problem hiding this comment.
const char* doesn't mean it cannot be free'd, it means it {should,must} not be modified. Pointers can point to r/o (by convention) data after all..
| const char *path = get_my_location (); | ||
| found = probe_dll_at (path); | ||
| if (path != nullptr) { | ||
| free (reinterpret_cast<void*>(const_cast<char*>(path))); |
There was a problem hiding this comment.
…cause this is just bizarre.
There was a problem hiding this comment.
It is, but we have a no-warnings policy, so... :)
Context: dotnet#4914 Context: dotnet@d583b7c The .NET5 version of the Mono runtime "hijacked" `__Internal` for its own purposes and the change causes us trouble when trying to resolve p/invokes from our managed code to our runtime. Instead we now use `xa-internal-api` (added in d583b7c) to specify the p/invoke library name.
grendello
force-pushed
the
drop-internal
branch
from
caf5ff0 to
7e050a6
Compare
|
Summary: Content: Context: https://github.com/xamarin/xamarin-android/pull/4914
Context: d583b7c215fe9be2f732b4a52423be0aea7f0809
Context: https://github.com/mono/mono/issues/20295#issuecomment-679271621
Context? https://github.com/mono/mono/pull/17369
Context? https://github.com/mono/mono/commit/d5b6cf33b3d2a89189be4e6bddcf0207e9887ecd
As a runtime extension, Mono long supported `[DllImport("__Internal")]`
as a synonym for [`dlopen(NULL, 0)`][0]. Unfortunately, this
[did not work on Android][1] -- Bionic would SIGSEGV when the
filename parameter was `NULL` -- and as "fallout" of this change the
`loader_func` that Xamarin.Android registers with mono via
`mono_dl_fallback_register()` would be invoked *instead of*
`dlopen(NULL)` when loading `__Internal`…but *only* on Android.
Use of this feature remains widely used in Xamarin.Android, e.g.
[`java-interop` is mapped to `__Internal`][2], avoiding the need to
build and ship a separate `libjava-interop.so` library.
Unfortunately, as part of .NET 5, MonoVM will be overhauling the
semantics of `[DllImport("__Internal")]`, preventing `loader_func`
from being invoked on Android to resolve `__Internal`.
Consequently, we need to begin moving away from use of `__Internal`.
Begin this process, and remove all P/Invokes to `__Internal` within
`Mono.Android.dll` with P/Invokes to `xa-internal-api` (d583b7c2).
[0]: https://linux.die.net/man/3/dlopen
[1]: https://github.com/mono/mono/commit/9b73cd9c33507ba27829728ea82d10d98335e1d6
[2]: https://github.com/xamarin/xamarin-android/blob/faf1d16692ead1115d34fdf7b84c256c94a321da/src/monodroid/config.xml#L2 |
6 participants
Context: #4914
Context: d583b7c
The .NET5 version of the Mono runtime "hijacked"
__Internalfor itsown purposes and the change causes us trouble when trying to resolve
p/invokes from our managed code to our runtime. Instead we now use
xa-internal-api(added in d583b7c) to specify the p/invoke libraryname.