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.

Sign up for GitHub

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

Jump to bottom

[Mono.Android] treat AllocObjectSupported as always true #9146

Merged
merged 2 commits into from
Sep 13, 2024
Merged

[Mono.Android] treat AllocObjectSupported as always true #9146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6e32c64
[Mono.Android] treat `AllocObjectSupported` as always true
jonathanpeppers Jul 25, 2024
fee9aa7
Comment for `JnienvInitializeArgs`
jonathanpeppers Sep 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions src/Mono.Android/Android.Runtime/AndroidRuntime.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,11 @@ class AndroidRuntime : JniRuntime {

internal AndroidRuntime (IntPtr jnienv,
IntPtr vm,
bool allocNewObjectSupported,
IntPtr classLoader,
IntPtr classLoader_loadClass,
bool jniAddNativeMethodRegistrationAttributePresent)
: base (new AndroidRuntimeOptions (jnienv,
vm,
allocNewObjectSupported,
classLoader,
classLoader_loadClass,
jniAddNativeMethodRegistrationAttributePresent))
Expand Down Expand Up @@ -87,7 +85,6 @@ public override void RaisePendingException (Exception pendingException)
class AndroidRuntimeOptions : JniRuntime.CreationOptions {
public AndroidRuntimeOptions (IntPtr jnienv,
IntPtr vm,
bool allocNewObjectSupported,
IntPtr classLoader,
IntPtr classLoader_loadClass,
bool jniAddNativeMethodRegistrationAttributePresent)
Expand All @@ -96,7 +93,6 @@ public AndroidRuntimeOptions (IntPtr jnienv,
ClassLoader = new JniObjectReference (classLoader, JniObjectReferenceType.Global);
ClassLoader_LoadClass_id= classLoader_loadClass;
InvocationPointer = vm;
NewObjectRequired = !allocNewObjectSupported;
ObjectReferenceManager = new AndroidObjectReferenceManager ();
TypeManager = new AndroidTypeManager (jniAddNativeMethodRegistrationAttributePresent);
ValueManager = new AndroidValueManager ();
Expand Down
18 changes: 3 additions & 15 deletions src/Mono.Android/Android.Runtime/JNIEnv.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,10 +172,7 @@ public static IntPtr AllocObject (Type type)

public static unsafe IntPtr StartCreateInstance (IntPtr jclass, IntPtr constructorId, JValue* constructorParameters)
{
if (JNIEnvInit.AllocObjectSupported) {
return AllocObject (jclass);
}
return NewObject (jclass, constructorId, constructorParameters);
return AllocObject (jclass);
}

public static unsafe IntPtr StartCreateInstance (IntPtr jclass, IntPtr constructorId, params JValue[] constructorParameters)
Expand All @@ -186,8 +183,6 @@ public static unsafe IntPtr StartCreateInstance (IntPtr jclass, IntPtr construct

public static unsafe void FinishCreateInstance (IntPtr instance, IntPtr jclass, IntPtr constructorId, JValue* constructorParameters)
{
if (!JNIEnvInit.AllocObjectSupported)
return;
CallNonvirtualVoidMethod (instance, jclass, constructorId, constructorParameters);
}

Expand All @@ -199,10 +194,7 @@ public static unsafe void FinishCreateInstance (IntPtr instance, IntPtr jclass,

public static unsafe IntPtr StartCreateInstance (Type type, string jniCtorSignature, JValue* constructorParameters)
{
if (JNIEnvInit.AllocObjectSupported) {
return AllocObject (type);
}
return CreateInstance (type, jniCtorSignature, constructorParameters);
return AllocObject (type);
}

public static unsafe IntPtr StartCreateInstance (Type type, string jniCtorSignature, params JValue[] constructorParameters)
Expand All @@ -213,9 +205,7 @@ public static unsafe IntPtr StartCreateInstance (Type type, string jniCtorSignat

public static unsafe IntPtr StartCreateInstance (string jniClassName, string jniCtorSignature, JValue* constructorParameters)
{
if (JNIEnvInit.AllocObjectSupported)
return AllocObject (jniClassName);
return CreateInstance (jniClassName, jniCtorSignature, constructorParameters);
return AllocObject (jniClassName);
}

public static unsafe IntPtr StartCreateInstance (string jniClassName, string jniCtorSignature, params JValue[] constructorParameters)
Expand All @@ -226,8 +216,6 @@ public static unsafe IntPtr StartCreateInstance (string jniClassName, string jni

public static unsafe void FinishCreateInstance (IntPtr instance, string jniCtorSignature, JValue* constructorParameters)
{
if (!JNIEnvInit.AllocObjectSupported)
return;
InvokeConstructor (instance, jniCtorSignature, constructorParameters);
}

Expand Down
7 changes: 2 additions & 5 deletions src/Mono.Android/Android.Runtime/JNIEnvInit.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ namespace Android.Runtime
static internal class JNIEnvInit
{
#pragma warning disable 0649
// NOTE: Keep this in sync with the native side in src/native/monodroid/monodroid-glue-internal.hh
internal struct JnienvInitializeArgs {
public IntPtr javaVm;
public IntPtr env;
Expand All @@ -22,7 +23,6 @@ internal struct JnienvInitializeArgs {
public IntPtr Class_forName;
public uint logCategories;
public int version; // TODO: remove, not needed anymore
public int androidSdkVersion;
Copy link
Member

Choose a reason for hiding this comment

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

As per your comment on Discord:

Ok, I found maybe the size of JnienvInitializeArgs in C++ / C# didn't match
I removed from one and not the other

this means we should also add a public ulong struct_size field to the beginning of the struct, and assert that the size matches:

internal partial struct JnienvInitializeArgs {
    public static along ExpectedStructSize = IntPtr.Size == 8 ? XXX : YYY;
    public ulong struct_size;
    // …
}

Update the C++ side to be consistent:

In C++ land, set the new struct_size field in init_android_runtime():

struct JnienvInitializeArgs init = {};
init.struct_size = sizeof(JnienvInitializeArgs);

And finally in Initialize(), assert that things match:

if (args->struct_size != JnienvInitializeArgs.ExpectedStructSize) {
    FailFast(…);
}

While doing this, we may also want to reorder the struct members so that pointers are stored together, to minimize internal alignment padding.

Copy link
Contributor

Choose a reason for hiding this comment

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

sizeof() on the native size won't work as expected in this case. It will include padding of the structure which may differ depending on the target platform (even if bitness is the same). We'd have to calculate structure size as a sum of individual field sizes (a compile-time calculation), but then the managed size would have to compute it at run time, a bit of time loss.

However, perhaps we can just add a "serial number" field to the structure and bump it every time the structure changes, with comments to update both the managed and native sides at the same time?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a comment to both structs. I think if I had seen that, I would have caught it earlier.

public int grefGcThreshold;
public IntPtr grefIGCUserPeer;
public int isRunningOnDesktop;
Expand All @@ -36,7 +36,6 @@ internal struct JnienvInitializeArgs {
#pragma warning restore 0649

internal static AndroidValueManager? AndroidValueManager;
internal static bool AllocObjectSupported;
internal static bool IsRunningOnDesktop;
internal static bool jniRemappingInUse;
internal static bool LogAssemblyCategory;
Expand Down Expand Up @@ -94,12 +93,10 @@ internal static unsafe void Initialize (JnienvInitializeArgs* args)

mid_Class_forName = new JniMethodInfo (args->Class_forName, isStatic: true);

bool androidNewerThan10 = args->androidSdkVersion > 10;
BoundExceptionType = (BoundExceptionType)args->ioExceptionType;
androidRuntime = new AndroidRuntime (args->env, args->javaVm, androidNewerThan10, args->grefLoader, args->Loader_loadClass, args->jniAddNativeMethodRegistrationAttributePresent != 0);
androidRuntime = new AndroidRuntime (args->env, args->javaVm, args->grefLoader, args->Loader_loadClass, args->jniAddNativeMethodRegistrationAttributePresent != 0);
AndroidValueManager = (AndroidValueManager) androidRuntime.ValueManager;

AllocObjectSupported = androidNewerThan10;
IsRunningOnDesktop = args->isRunningOnDesktop == 1;

grefIGCUserPeer_class = args->grefIGCUserPeer;
Expand Down
1 change: 0 additions & 1 deletion src/java-runtime/java/mono/android/MonoPackageManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,6 @@ public static void LoadApplication (Context context, ApplicationInfo runtimePack
localDateTimeOffset,
loader,
MonoPackageManager_Resources.Assemblies,
Build.VERSION.SDK_INT,
isEmulator (),
haveSplitApks
);
Expand Down
1 change: 0 additions & 1 deletion src/java-runtime/java/mono/android/Runtime.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ public static native void initInternal (
int localDateTimeOffset,
ClassLoader loader,
String[] assemblies,
int apiLevel,
boolean isEmulator,
boolean haveSplitApks
);
Expand Down
6 changes: 0 additions & 6 deletions src/native/monodroid/internal-pinvokes.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,12 +132,6 @@ _monodroid_gc_wait_for_bridge_processing ()
mono_gc_wait_for_bridge_processing ();
}

int
_monodroid_get_android_api_level ()
{
return monodroidRuntime.get_android_api_level ();
}

void
monodroid_clear_gdb_wait ()
{
Expand Down
2 changes: 1 addition & 1 deletion src/native/monodroid/mono_android_Runtime.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 2 additions & 8 deletions src/native/monodroid/monodroid-glue-internal.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ namespace xamarin::android::internal
Java = 0x01,
};

// NOTE: Keep this in sync with managed side in src/Mono.Android/Android.Runtime/JNIEnvInit.cs
struct JnienvInitializeArgs {
JavaVM *javaVm;
JNIEnv *env;
Expand All @@ -91,7 +92,6 @@ namespace xamarin::android::internal
jmethodID Class_forName;
unsigned int logCategories;
int version;
int androidSdkVersion;
int grefGcThreshold;
jobject grefIGCUserPeer;
int isRunningOnDesktop;
Expand All @@ -118,16 +118,11 @@ namespace xamarin::android::internal
void Java_mono_android_Runtime_register (JNIEnv *env, jstring managedType, jclass nativeClass, jstring methods);
void Java_mono_android_Runtime_initInternal (JNIEnv *env, jclass klass, jstring lang, jobjectArray runtimeApksJava,
jstring runtimeNativeLibDir, jobjectArray appDirs, jint localDateTimeOffset,
jobject loader, jobjectArray assembliesJava, jint apiLevel, jboolean isEmulator,
jobject loader, jobjectArray assembliesJava, jboolean isEmulator,
jboolean haveSplitApks);

jint Java_JNI_OnLoad (JavaVM *vm, void *reserved);

int get_android_api_level () const
{
return android_api_level;
}

jclass get_java_class_System () const
{
return java_System;
Expand Down Expand Up @@ -252,7 +247,6 @@ namespace xamarin::android::internal
#endif
private:
MonoMethod *registerType = nullptr;
int android_api_level = 0;
volatile bool monodroid_gdb_wait = true;
jclass java_System;
jmethodID java_System_identityHashCode;
Expand Down
8 changes: 2 additions & 6 deletions src/native/monodroid/monodroid-glue.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -819,7 +819,6 @@ MonodroidRuntime::init_android_runtime (JNIEnv *env, jclass runtimeClass, jobjec
init.env = env;
init.logCategories = log_categories;
init.version = env->GetVersion ();
init.androidSdkVersion = android_api_level;
init.isRunningOnDesktop = is_running_on_desktop ? 1 : 0;
init.brokenExceptionTransitions = application_config.broken_exception_transitions ? 1 : 0;
init.packageNamingPolicy = static_cast<int>(application_config.package_naming_policy);
Expand Down Expand Up @@ -1353,7 +1352,7 @@ MonodroidRuntime::install_logging_handlers ()
inline void
MonodroidRuntime::Java_mono_android_Runtime_initInternal (JNIEnv *env, jclass klass, jstring lang, jobjectArray runtimeApksJava,
jstring runtimeNativeLibDir, jobjectArray appDirs, jint localDateTimeOffset,
jobject loader, jobjectArray assembliesJava, jint apiLevel, jboolean isEmulator,
jobject loader, jobjectArray assembliesJava, jboolean isEmulator,
jboolean haveSplitApks)
{
char *mono_log_mask_raw = nullptr;
Expand Down Expand Up @@ -1394,7 +1393,6 @@ MonodroidRuntime::Java_mono_android_Runtime_initInternal (JNIEnv *env, jclass kl
);
}

android_api_level = apiLevel;
AndroidSystem::detect_embedded_dso_mode (applicationDirs);
AndroidSystem::set_running_in_emulator (isEmulator);

Expand Down Expand Up @@ -1548,7 +1546,6 @@ Java_mono_android_Runtime_init (JNIEnv *env, jclass klass, jstring lang, jobject
0,
loader,
assembliesJava,
apiLevel,
/* isEmulator */ JNI_FALSE,
/* haveSplitApks */ JNI_FALSE
);
Expand All @@ -1557,7 +1554,7 @@ Java_mono_android_Runtime_init (JNIEnv *env, jclass klass, jstring lang, jobject
JNIEXPORT void JNICALL
Java_mono_android_Runtime_initInternal (JNIEnv *env, jclass klass, jstring lang, jobjectArray runtimeApksJava,
jstring runtimeNativeLibDir, jobjectArray appDirs, jint localDateTimeOffset, jobject loader,
jobjectArray assembliesJava, jint apiLevel, jboolean isEmulator,
jobjectArray assembliesJava, jboolean isEmulator,
jboolean haveSplitApks)
{
monodroidRuntime.Java_mono_android_Runtime_initInternal (
Expand All @@ -1570,7 +1567,6 @@ Java_mono_android_Runtime_initInternal (JNIEnv *env, jclass klass, jstring lang,
localDateTimeOffset,
loader,
assembliesJava,
apiLevel,
isEmulator,
application_config.ignore_split_configs ? false : haveSplitApks
);
Expand Down
1 change: 0 additions & 1 deletion src/native/pinvoke-override/generate-pinvoke-tables.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ const std::vector<std::string> internal_pinvoke_names = {
"monodroid_free",
"_monodroid_freeifaddrs",
"_monodroid_gc_wait_for_bridge_processing",
"_monodroid_get_android_api_level",
"_monodroid_get_dns_servers",
"monodroid_get_dylib",
"_monodroid_get_identity_hash_code",
Expand Down
8 changes: 3 additions & 5 deletions src/native/pinvoke-override/pinvoke-tables.include
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
namespace {
#if INTPTR_MAX == INT64_MAX
//64-bit internal p/invoke table
std::array<PinvokeEntry, 49> internal_pinvokes {{
std::array<PinvokeEntry, 48> internal_pinvokes {{
{0x452e23128e42f0a, "monodroid_get_log_categories", reinterpret_cast<void*>(&monodroid_get_log_categories)},
{0xa50ce5de13bf8b5, "_monodroid_timezone_get_default_id", reinterpret_cast<void*>(&_monodroid_timezone_get_default_id)},
{0x19055d65edfd668e, "_monodroid_get_network_interface_up_state", reinterpret_cast<void*>(&_monodroid_get_network_interface_up_state)},
Expand Down Expand Up @@ -43,7 +43,6 @@ namespace {
{0xc2a21d3f6c8ccc24, "_monodroid_lookup_replacement_method_info", reinterpret_cast<void*>(&_monodroid_lookup_replacement_method_info)},
{0xc5b4690e13898fa3, "monodroid_timing_start", reinterpret_cast<void*>(&monodroid_timing_start)},
{0xcc873ea8493d1dd5, "monodroid_embedded_assemblies_set_assemblies_prefix", reinterpret_cast<void*>(&monodroid_embedded_assemblies_set_assemblies_prefix)},
{0xce439cfbe29dec11, "_monodroid_get_android_api_level", reinterpret_cast<void*>(&_monodroid_get_android_api_level)},
{0xd1e121b94ea63f2e, "_monodroid_gref_get", reinterpret_cast<void*>(&_monodroid_gref_get)},
{0xd5151b00eb33d85e, "monodroid_TypeManager_get_java_class_name", reinterpret_cast<void*>(&monodroid_TypeManager_get_java_class_name)},
{0xda517ef392b6a888, "java_interop_free", reinterpret_cast<void*>(&java_interop_free)},
Expand Down Expand Up @@ -553,14 +552,13 @@ constexpr hash_t system_security_cryptography_native_android_library_hash = 0x18
constexpr hash_t system_globalization_native_library_hash = 0x28b5c8fca080abd5;
#else
//32-bit internal p/invoke table
std::array<PinvokeEntry, 49> internal_pinvokes {{
std::array<PinvokeEntry, 48> internal_pinvokes {{
{0xb7a486a, "monodroid_TypeManager_get_java_class_name", reinterpret_cast<void*>(&monodroid_TypeManager_get_java_class_name)},
{0xf562bd9, "monodroid_embedded_assemblies_set_assemblies_prefix", reinterpret_cast<void*>(&monodroid_embedded_assemblies_set_assemblies_prefix)},
{0x1a8eab17, "_monodroid_get_identity_hash_code", reinterpret_cast<void*>(&_monodroid_get_identity_hash_code)},
{0x227a2636, "monodroid_get_namespaced_system_property", reinterpret_cast<void*>(&monodroid_get_namespaced_system_property)},
{0x2a0e1744, "java_interop_strdup", reinterpret_cast<void*>(&java_interop_strdup)},
{0x2aea7c33, "_monodroid_max_gref_get", reinterpret_cast<void*>(&_monodroid_max_gref_get)},
{0x2f7d0f53, "_monodroid_get_android_api_level", reinterpret_cast<void*>(&_monodroid_get_android_api_level)},
{0x30b9487b, "_monodroid_get_dns_servers", reinterpret_cast<void*>(&_monodroid_get_dns_servers)},
{0x3227d81a, "monodroid_timing_start", reinterpret_cast<void*>(&monodroid_timing_start)},
{0x333d4835, "_monodroid_lookup_replacement_method_info", reinterpret_cast<void*>(&_monodroid_lookup_replacement_method_info)},
Expand Down Expand Up @@ -1095,6 +1093,6 @@ constexpr hash_t system_security_cryptography_native_android_library_hash = 0x93
constexpr hash_t system_globalization_native_library_hash = 0xa66f1e5a;
#endif

constexpr size_t internal_pinvokes_count = 49;
constexpr size_t internal_pinvokes_count = 48;
constexpr size_t dotnet_pinvokes_count = 477;
} // end of anonymous namespace
1 change: 0 additions & 1 deletion src/native/runtime-base/internal-pinvokes.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ void _monodroid_weak_gref_delete (jobject handle, char type, const char *threadN
void _monodroid_lref_log_new (int lrefc, jobject handle, char type, const char *threadName, int threadId, const char *from, int from_writable);
void _monodroid_lref_log_delete (int lrefc, jobject handle, char type, const char *threadName, int threadId, const char *from, int from_writable);
void _monodroid_gc_wait_for_bridge_processing ();
int _monodroid_get_android_api_level ();
void monodroid_clear_gdb_wait ();
void* _monodroid_get_identity_hash_code (JNIEnv *env, void *v);
void* _monodroid_timezone_get_default_id ();
Expand Down