-
Notifications
You must be signed in to change notification settings - Fork 728
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
Fix sequence after frame pop query #18068
Conversation
@babsingh I think this fixes the frame pop issue |
@gacholio Can you please take a look. |
This seems incorrect to me. If a frame pop has been requested, we should not be continuing on with the rest of the code. What are you actually trying to fix here? |
I don't recall what the async check flags is for - will have to take a look at the code. |
The interrupt is actually not used - the only listener is an empty function. |
We return |
I think if we are not going to transition to async check we should resume normal operations at that point instead of trying to exit immediately |
I'll need to think about this - it's not clear to me how a hidden frame gets tagged for pop (maybe special handling because the visible caller frame is being popped?). |
The pop frames flag is not tagged on a frame (the notification event is) - there is a public flag for the pop frame request that results in the pop frame return value from the async check. I think the problem here is that the PopFrame/ForceEarlyReturn functions don't see the hidden frame (they both use the "visible only" flag) - they are expecting to operate on the visible top frame. More thought required for how to handle this - both APIs require that the target thread be suspended, which limits the places that require checking. |
Here's what I think is happening:
Special frame
So, the correct action would be to pop the special and hidden frames, and process the pop in the visible frame. Looking at the code in The correct change here is probably to always execute the async check (the hook call could be removed entirely as it serves no purpose other than as a flag to the JIT that the capabilities have been enabled). |
|
The existing code fails to remove the generic special frame if triggering the frame pop hook doesnt require us to run the async check. Signed-off-by: Tobi Ajila <[email protected]>
@babsingh I'd like to know if this resolves the test issue before we make this change. |
@gacholio Ran the test 250 times with the proposed fix, and no failures were seen. This PR resolves the test issue. |
I also ran some internal testing on it |
jenkins test sanity alinux64 jdk21 |
The existing code fails to remove the generic special frame if triggering the frame pop hook doesnt require us to run the async check.