-
Notifications
You must be signed in to change notification settings - Fork 721
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
Ensure JIT/AOT code is not invalidated post-restore under -XX:+DebugOnRestore
#20047
Conversation
10291b2
to
967540d
Compare
@dsouzai would you mind enabling the following failure conditions that were disabled in #19754? I verified manually this PR passed the test with the failure conditions were reestablished. openj9/test/functional/cmdLineTests/criu/criu_jitPostRestore.xml Lines 75 to 77 in ebe8718
openj9/test/functional/cmdLineTests/criu/criu_jitPostRestore.xml Lines 94 to 96 in ebe8718
|
967540d
to
ff8812d
Compare
@ymanton could you please review/merge? |
@TobiAjila fyi this also needs to get into 0.48 |
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.
LGTM.
Jenkins test sanity.functional xlinux,plinux,zlinux jdk11,jdk17 |
* as well as whether method trace and FSD were enabled | ||
* pre-checkpoint. | ||
*/ | ||
cacheEventsStatus(); |
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 have an explicit return type (i.e. void
).
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.
Ah damn, thanks for pointing out. Surprising this ever built locally.
Cache the status of JVMTI events such as exception throw/catch as well as whether method trace and FSD were enabled pre-checkpoint. Signed-off-by: Irwin D'Souza <[email protected]>
Under -XX:+DebugOnRestore, the VM will enable some capabilities that will be reset on restore (if the user does not want debug on restore). However, if the JIT detects these capabilities via the J9HookDisable call, there is no way for the JIT to know whether the capability was set by the VM or a user pre-checkpoint. With the recent change to the isDebugOnRestoreEnabled to check if a uesr specified an JDWP agent pre-checkpoint, the JIT can simply return early from isFSDNeeded. Under -XX:+DebugOnRestore, the JIT already generates FSD code so there is on need to check the caps that were set by the VM pre-checkpoint. isFSDNeeded is called on restore, so at that point it is appropriate for the JIT to execute the rest of the method at that point. Signed-off-by: Irwin D'Souza <[email protected]>
Signed-off-by: Irwin D'Souza <[email protected]>
ff8812d
to
d3a7474
Compare
Jenkins test sanity.functional xlinux,plinux,zlinux jdk11,jdk17 |
The ppc64le test failure are all the
Because
rather than here
@ymanton I believe you've also seen this issue before right? |
Yeah that's a pre-existing issue. I'll merge shortly if there are no further comments and requested changes are confirmed. |
@ymanton could this be merged? |
#19754 adds VM Support for Debug On Restore. As the code stands, this will result in the JIT invalidating all code post-restore because it is unable to distinguish between FSD mode caused by the VM or the user.
This PR fixes this by adding the following changes:
-XX:+DebugOnRestore
Depends on #19754 (specifically the update to the
isDebugOnRestoreEnabled
API).