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.

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

Merged
merged 1 commit into from
Sep 12, 2023
Merged

Conversation

tajila
Copy link
Contributor

@tajila tajila commented Sep 1, 2023

The existing code fails to remove the generic special frame if triggering the frame pop hook doesnt require us to run the async check.

@tajila
Copy link
Contributor Author

tajila commented Sep 1, 2023

@babsingh I think this fixes the frame pop issue

@tajila
Copy link
Contributor Author

tajila commented Sep 1, 2023

@gacholio Can you please take a look.

@tajila tajila requested a review from gacholio September 1, 2023 18:27
@gacholio
Copy link
Contributor

gacholio commented Sep 1, 2023

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?

@gacholio
Copy link
Contributor

gacholio commented Sep 1, 2023

I don't recall what the async check flags is for - will have to take a look at the code.

@gacholio
Copy link
Contributor

gacholio commented Sep 1, 2023

The interrupt is actually not used - the only listener is an empty function.

@tajila
Copy link
Contributor Author

tajila commented Sep 1, 2023

What are you actually trying to fix here?

We return rc = EXECUTE_BYTECODE without popping the special frame so the _pc is all messed up

@tajila
Copy link
Contributor Author

tajila commented Sep 1, 2023

I don't recall what the async check flags is for - will have to take a look at the code.

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

@gacholio
Copy link
Contributor

gacholio commented Sep 1, 2023

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?).

@gacholio
Copy link
Contributor

gacholio commented Sep 6, 2023

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.

@gacholio
Copy link
Contributor

gacholio commented Sep 8, 2023

Here's what I think is happening:

  • Target thread (T) his stack overflow and suspends due to a request from the popping thread (P), stack looks like:

Special frame
Hidden method frame (H)
Visible calling frame (V)

  • Thread P sees method V on top of stack and requests a pop and resumes thread T

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 handlePopFrames, it appears that the code can handle the hidden frames on top of stack, and it handles the case where method V is compiled.

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).

@gacholio gacholio marked this pull request as draft September 8, 2023 15:38
@gacholio
Copy link
Contributor

gacholio commented Sep 8, 2023

#if defined(DEBUG_VERSION)
			if (J9_CHECK_ASYNC_POP_FRAMES == action) {
				rc = GOTO_ASYNC_CHECK;
				goto done;
			}
#endif

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]>
@gacholio
Copy link
Contributor

@babsingh I'd like to know if this resolves the test issue before we make this change.

@babsingh
Copy link
Contributor

@gacholio Ran the test 250 times with the proposed fix, and no failures were seen. This PR resolves the test issue.

@tajila
Copy link
Contributor Author

tajila commented Sep 12, 2023

I also ran some internal testing on it

@tajila tajila marked this pull request as ready for review September 12, 2023 16:47
@tajila
Copy link
Contributor Author

tajila commented Sep 12, 2023

jenkins test sanity alinux64 jdk21

@gacholio gacholio merged commit 1f58a61 into eclipse-openj9:master Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants