-
-
Notifications
You must be signed in to change notification settings - Fork 245
Fix jit crash caused by hooking intrinsic methods in bootclasspath on Android 15 and above #148
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
base: master
Are you sure you want to change the base?
Conversation
… Android 15 and above
@@ -28,6 +28,9 @@ class ArtMethod { | |||
inline static auto GetMethodShorty_ = | |||
"_ZN3art15GetMethodShortyEP7_JNIEnvP10_jmethodID"_sym.as<const char *(JNIEnv *env, jmethodID mid)>; | |||
|
|||
inline static auto SetNotIntrinsic_ = |
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.
Why do we need another symbol here? SetNonCompilable just use SetAccessFlags for example.
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.
Because the value of kAccIntrinsic
may be modified in subsequent updates of ART, I call this function first.
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.
We should use SetNotIntrinsic
symbol since switching between intrinsic and non-intrinsic may affect many flags (kAccIntrinsicBits).
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.
We should use
SetNotIntrinsic
symbol since switching between intrinsic and non-intrinsic may affect many flags (kAccIntrinsicBits).
Already resolved 25 minutes ago
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 mean just unsetting the kAccIntrinsic flag is not enough, because intrinsic methods use some other flag bits to store their type.
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.
Copilot wasn't able to review any files in this pull request.
Files not reviewed (2)
- lsplant/src/main/jni/art/runtime/art_method.cxx: Language not supported
- lsplant/src/main/jni/lsplant.cc: Language not supported
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.
Pull Request Overview
This PR fixes a JIT crash on Android 15 and above that occurs when hooking intrinsic methods in the bootclasspath.
- In lsplant.cc, a call to target->SetNonIntrinsic() is added to remove the intrinsic flag from the target method.
- In art_method.cxx, a new SetNonIntrinsic() method is introduced and the intrinsic flag handling is adjusted based on Android API levels.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
lsplant/src/main/jni/lsplant.cc | Added target->SetNonIntrinsic() in DoHook to disable intrinsic optimizations. |
lsplant/src/main/jni/art/runtime/art_method.cxx | New SetNonIntrinsic() method and intrinsic flag handling for different SDK versions. |
@@ -539,6 +539,8 @@ bool DoHook(ArtMethod *target, ArtMethod *hook, ArtMethod *backup) { | |||
} else { | |||
LOGV("Generated trampoline %p", entrypoint); | |||
|
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.
[nitpick] Consider adding a comment here to explain that removing the intrinsic flag is necessary to prevent a JIT crash on Android 15 and above.
// Removing the intrinsic flag is necessary to prevent a JIT crash on Android 15 and above. |
Copilot uses AI. Check for mistakes.
dummy->SetAccessFlags(kAccIntrinsic); | ||
SetNotIntrinsic_(dummy.get()); | ||
if (dummy->GetAccessFlags() == kAccIntrinsic) [[unlikely]] { | ||
for (auto shift = 16U; 32U > shift; ++shift) { |
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.
[nitpick] Consider documenting the rationale behind the magic numbers (16U through 32U) used in the bit-shifting logic or replacing them with named constants to clarify their purpose.
for (auto shift = 16U; 32U > shift; ++shift) { | |
for (auto shift = kBitShiftStart; kBitShiftEnd > shift; ++shift) { |
Copilot uses AI. Check for mistakes.
This only happens on Android 15 and above, you can see intrinsics_list.h and instruction_builder.cc