-
Notifications
You must be signed in to change notification settings - Fork 52
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
Remove support for using Java's WeakReference instead of JNI's weak handles #1257
base: main
Are you sure you want to change the base?
Conversation
d4d21b5
to
0087a52
Compare
@@ -22,7 +22,6 @@ unsafe public class JavaException : Exception, IJavaPeerable | |||
JniObjectReferenceType handle_type; | |||
#pragma warning disable 0169 | |||
// Used by JavaInteropGCBridge | |||
IntPtr weak_handle; |
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.
You shouldn't remove this field yet, as it's still used by dotnet/android, even if it isn't "used":
- https://github.com/dotnet/android/blob/852c38e0743e6e75981a491e7b430a5fe403027e/src/native/monodroid/monodroid-glue.cc#L766
- https://github.com/dotnet/android/blob/852c38e0743e6e75981a491e7b430a5fe403027e/src/native/monodroid/monodroid-glue.cc#L849-L853
- https://github.com/dotnet/android/blob/852c38e0743e6e75981a491e7b430a5fe403027e/src/native/monodroid/osbridge.hh#L83-L94
- https://github.com/dotnet/android/blob/852c38e0743e6e75981a491e7b430a5fe403027e/src/native/monodroid/osbridge.cc#L25-L33
Removing it now will result in app startup aborts until dotnet/android is updated to no longer mention weak_handle
.
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 didn't realize that all this code is duplicated in dotnet/android. I'll return back the field and try to see if we can remove it on dotnet/android side too. The lowest supported API level is 21 for .NET 9, so there doesn't seem to be a valid reason to keep this around.
@@ -24,7 +24,6 @@ unsafe public class JavaObject : IJavaPeerable | |||
JniObjectReferenceType handle_type; | |||
#pragma warning disable 0169 | |||
// Used by JavaInteropGCBridge | |||
IntPtr weak_handle; |
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.
Ditto.
static mono_bool | ||
take_global_ref_jni (JavaInteropGCBridge *bridge, JNIEnv *env, MonoObject *obj, const char *thread_name, int64_t thread_id) |
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 would prefer not changing these method names & related strings, as:
- The same names are used in dotnet/android, and consistency is "nice"
- The strings are present in
tools/logcat-parse
unit tests (tests/logcat-parse-Tests
), which implies (a) they've been around awhile, and (2) I'm not immediately sure what'll happen to logcat-parse we change them. (Hopefully nothing! But…)
@@ -393,7 +388,7 @@ partial class JreNativeMethods { | |||
internal static extern int java_interop_gc_bridge_set_current_once (IntPtr bridge); | |||
|
|||
[DllImport (JavaInteropLib, CallingConvention=CallingConvention.Cdecl)] | |||
internal static extern int java_interop_gc_bridge_register_hooks (IntPtr bridge, GCBridgeUseWeakReferenceKind weak_ref_kind); | |||
internal static extern int java_interop_gc_bridge_register_hooks (IntPtr bridge); |
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 change introduces a "mismatch" between the native side and the managed side. (Native side has an additional [[maybe_unused]] int weak_ref_kind
parameter.) This is nominally "fine" because it's C calling convention, but if we ever accidentally-on-purpose change the calling convention, the difference in number of parameters could cause stack corruption.
This should instead be:
internal static extern int java_interop_gc_bridge_register_hooks (IntPtr bridge, int must_be_one);
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.
That was unintentional. I initially kept the parameter as unused and then changed my mind to remove it…
@@ -1284,23 +1203,6 @@ java_interop_gc_bridge_register_hooks (JavaInteropGCBridge *bridge, int weak_ref | |||
MonoGCBridgeCallbacks bridge_cbs; | |||
memset (&bridge_cbs, 0, sizeof (bridge_cbs)); | |||
|
|||
switch (weak_ref_kind) { |
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 should assert that weak_ref_kind
is one, and bail otherwise:
if (weak_ref_kind != 1) {
return -1;
}
@@ -1272,7 +1191,7 @@ gc_cross_references (int num_sccs, MonoGCBridgeSCC **sccs, int num_xrefs, MonoGC | |||
} | |||
|
|||
int | |||
java_interop_gc_bridge_register_hooks (JavaInteropGCBridge *bridge, int weak_ref_kind) | |||
java_interop_gc_bridge_register_hooks (JavaInteropGCBridge *bridge, [[maybe_unused]] int weak_ref_kind) |
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 tried building this, but I'm kinda shocked at the idea that this would build and/or run, considering that you updated the header file to remove the second parameter.
So of course I need to play around…
extern "C" {
void foo();
}
void foo([[maybe_unused]] int x) {
}
int main() {
foo(0);
}
It compiles without error:
% clang++ mu.cc -std=c++17
…but the symbols, they're mangled!
% nm a.out
0000000100003f80 T __Z3fooi
0000000100000000 T __mh_execute_header
0000000100003f90 T _main
There is no foo
symbol, but there is __Z3fooi
, the C++ mangled version of foo(int)
.
My guess -- again, without compiling this PR -- is that src/java-interop
will compile, but the resulting native library will not contain a C callable java_interop_gc_bridge_register_hooks
, but instead a C++ mangled __Z37java_interop_gc_bridge_register_hooks…
symbol, and things will blow up at runtime because the P/Invoke for java_interop_gc_bridge_register_hooks()
won't find that symbol.
dotnet/android@e98ae9c removes support for Development of #1257 can resume, if desired. |
Contributes to #1255