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.

Sign up for GitHub

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

Jump to bottom

Fix continuation stacks when breakpointing #18413

Merged
merged 1 commit into from Nov 14, 2023
Merged

Fix continuation stacks when breakpointing #18413

merged 1 commit into from Nov 14, 2023

Conversation

gacholio
Copy link
Contributor

@gacholio gacholio commented Nov 7, 2023 •
edited

The breakpoint handling code was only fixing stacks actively running on
a thread. The stacks for continuations also need to be fixed.

Fixes: #18088

Signed-off-by: Graham Chapman graham_chapman@ca.ibm.com

@gacholio gacholio changed the title Fix continutation stacks when breakpointing Fix continuation stacks when breakpointing Nov 7, 2023
@gacholio
Copy link
Contributor Author

gacholio commented Nov 9, 2023

@babsingh I'm looking into the JVMTI test failures. Please do an initial review of my use of the continuations.

babsingh
babsingh reviewed Nov 9, 2023
View reviewed changes
Copy link
Contributor

@babsingh babsingh left a comment •
edited

Choose a reason for hiding this comment

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

  • Is exclusive VM access acquired for all j9mm_iterate_all_continuation_objects calls?
  • Should j9gc_flush_nonAllocationCaches_for_walk and j9mm_iterate_all_continuation_objects be invoked together?
  • Need to pass the correct threadObject to walkContinuationStackFrames.

runtime/jvmti/jvmtiHelpers.c Outdated Show resolved Hide resolved
runtime/codert_vm/decomp.cpp Show resolved Hide resolved
@babsingh
Copy link
Contributor

babsingh commented Nov 9, 2023

++ @fengxue-IS for a second set of eyes.

fengxue-IS
fengxue-IS reviewed Nov 9, 2023
View reviewed changes
Copy link
Contributor

@fengxue-IS fengxue-IS left a comment

Choose a reason for hiding this comment

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

code lgtm, few minor format picks

runtime/jvmti/jvmtiHelpers.c Outdated Show resolved Hide resolved
runtime/codert_vm/decomp.cpp Outdated Show resolved Hide resolved
runtime/codert_vm/decomp.cpp Outdated Show resolved Hide resolved
runtime/codert_vm/decomp.cpp Outdated Show resolved Hide resolved
@gacholio
Copy link
Contributor Author

gacholio commented Nov 9, 2023

I found more places that need the flush, so I've helperized acquiring exclusive in JVMTI (some of the callers don't strictly need the flush, but it's a tiny amount of time compared to acquiring exclusive).

@gacholio
Copy link
Contributor Author

On reflection, I don't like the exclusive/flush solution. I think @babsingh initial idea is best - do the flush before the iterate. I'll update tomorrow.

@gacholio
Copy link
Contributor Author

@dmitripivkine Please confirm it's OK to call j9gc_flush_nonAllocationCaches_for_walk multiple times in the same exclusive VM access.

@dmitripivkine
Copy link
Contributor

@dmitripivkine Please confirm it's OK to call j9gc_flush_nonAllocationCaches_for_walk multiple times in the same exclusive VM access.

Yes, I believe so. However it might be not optimum because once threads local buffers are flushed nothing should be added under the same exclusive.

babsingh
babsingh previously requested changes Nov 11, 2023
View reviewed changes
Copy link
Contributor

@babsingh babsingh left a comment

Choose a reason for hiding this comment

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

accidentally approved while juggling through multiple PRs.

@gacholio gacholio marked this pull request as ready for review November 13, 2023 11:44
@gacholio
Copy link
Contributor Author

@babsingh Ready for final review.

babsingh
babsingh reviewed Nov 13, 2023
View reviewed changes
Copy link
Contributor

@babsingh babsingh left a comment

Choose a reason for hiding this comment

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

minor formatting nits; otherwise, lgtm.

runtime/codert_vm/decomp.cpp Outdated Show resolved Hide resolved
runtime/codert_vm/decomp.cpp Outdated Show resolved Hide resolved
runtime/codert_vm/decomp.cpp Outdated Show resolved Hide resolved
runtime/codert_vm/decomp.cpp Outdated Show resolved Hide resolved
@gacholio
Fix continuation stacks when breakpointing
a4ac0f5 
The breakpoint handling code was only fixing stacks actively running on
a thread. The stacks for continuations also need to be fixed.

Fixes: eclipse-openj9#18088

Signed-off-by: Graham Chapman <graham_chapman@ca.ibm.com>
@babsingh
Copy link
Contributor

jenkins test sanity zlinux jdk8,jdk21

@babsingh
Copy link
Contributor

jenkins compile win jdk8,jdk21

babsingh
babsingh approved these changes Nov 14, 2023
View reviewed changes
Copy link
Contributor

@babsingh babsingh left a comment •
edited

Choose a reason for hiding this comment

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

https://openj9-jenkins.osuosl.org/job/Test_openjdk21_j9_sanity.functional_s390x_linux_Personal_testList_1/57/consoleFull

@babsingh babsingh merged commit 5fde7f7 into eclipse-openj9:master Nov 14, 2023
7 of 9 checks passed
@babsingh babsingh mentioned this pull request Nov 14, 2023
Closed
@gacholio gacholio deleted the break branch November 14, 2023 10:24
@babsingh babsingh mentioned this pull request Nov 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fengxue-IS fengxue-IS fengxue-IS left review comments

@babsingh babsingh babsingh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@gacholio @babsingh @dmitripivkine @fengxue-IS