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

Correctly initialize walkState values when calling walkStackFrames #18967

Merged
merged 2 commits into from
Feb 20, 2024

Conversation

fengxue-IS
Copy link
Contributor

After a thread's stackWalkState is used, we don't reset the fields to 0 but expects future code uses to set the parameters to the expected value before calling walkStackFrames.
This change fixes walkWrapperImpl where userData1 is not properly set.

Fixes: #18696
Fixes: #18961

After a thread's stackWalkState is used, we don't reset the fields to 0
but expects future code uses to set the parameters to the expected value
before calling walkStackFrames.
This change fixes walkWrapperImpl where userData1 is not properly set.

Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>
@fengxue-IS
Copy link
Contributor Author

Personal build, build failures due to network issue, sanity.functional test failure is unrelated.

@keithc-ca can you please take a look at this.

@fengxue-IS fengxue-IS marked this pull request as ready for review February 16, 2024 16:22
@@ -146,6 +147,8 @@ Java_java_lang_StackWalker_walkWrapperImpl(JNIEnv *env, jclass clazz, jint flags
if (NULL == walkerMethodChars) { /* native out of memory exception pending */
return NULL;
}
/* Ensure userData1/2 used by stackFrameFilter function is set properly. */
walkState->userData1 = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please explain why this is appropriate. Should line 156 not be here instead?

	walkState->userData1 = (void *)(UDATA)flags;

Same question for continuations (lines 216-220).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, I placed the NULL to avoid conflict with getCallerClass call, but given #18868 is already merged, it should be fine to move the assignment earlier.

I will update the PR and run local testing before asking for another round of review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

local testing passed, @keithc-ca can you please take another look

@fengxue-IS fengxue-IS changed the title Correctly initialize walkState values when calling walkStackFrames [WIP] Correctly initialize walkState values when calling walkStackFrames Feb 16, 2024
Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>
@fengxue-IS fengxue-IS changed the title [WIP] Correctly initialize walkState values when calling walkStackFrames Correctly initialize walkState values when calling walkStackFrames Feb 16, 2024
@keithc-ca
Copy link
Contributor

Jenkins test sanity alinux jdk17,jdk22

@keithc-ca
Copy link
Contributor

Exclude test (java/lang/StackWalker/CallerSensitiveMethod/Main.java) passed in https://openj9-jenkins.osuosl.org/job/Grinder/3319.

@keithc-ca
Copy link
Contributor

The most recent personal build is here.

@keithc-ca
Copy link
Contributor

There was one CRIU test failure which seems unrelated to this change.

@keithc-ca keithc-ca merged commit 7a3f413 into eclipse-openj9:master Feb 20, 2024
5 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants