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
8288387: GetLocalXXX/SetLocalXXX spec should require suspending target thread #10586
Conversation
👋 Welcome back sspitsyn! A progress list of the required criteria for merging this PR into |
Webrevs
|
test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/GetSetLocalUnsuspended.java
Outdated
Show resolved
Hide resolved
PING: Could someone review the CSR, please? |
I reviewed it yesterday. |
Thank you, Alan! |
Mailing list message from Dmitry Samersoff on hotspot-dev: Hi Serguei, Looks good for me. Thank you! -Dmitry On 06/10/2022 20:31, Serguei Spitsyn wrote: -- |
Mailing list message from Serguei Spitsyn on hotspot-dev: Hi Dmitry, Do you have any plans to full review and approve this PR? From: hotspot-dev <hotspot-dev-retn at openjdk.org> on behalf of Dmitry Samersoff <dmitry.samersoff at bell-sw.com> Looks good for me. Thank you! -Dmitry On 06/10/2022 20:31, Serguei Spitsyn wrote: -- |
Leonid and Dmitry, thank you for review! |
test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/GetSetLocalUnsuspended.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/GetSetLocalUnsuspended.java
Outdated
Show resolved
Hide resolved
@sspitsyn This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 144 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
/integrate |
Going to push as commit 21a825e.
Your commit was automatically rebased without conflicts. |
The spec of JVM TI GetLocalXXX/SetLocalXXX functions is updated to require the target thread to be suspended. If not suspended then the JVMTI_ERROR_THREAD_NOT_SUSPENDED error code is returned by the implementation.
The CSR is: https://bugs.openjdk.org/browse/JDK-8294690
A few tests are impacted by this fix:
The following test has been removed as non-relevant any more:
test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/GetLocalWithoutSuspendTest.java
New negative test has been added instead:
test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/GetSetLocalUnsuspended.java
All JVM TI and JPDA tests were used locally for verification.
They were also run in Loom repository with
JTREG_MAIN_WRAPPER=Virtual
.Mach5 test runs on all platforms are TBD.
Progress
Issues
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10586/head:pull/10586
$ git checkout pull/10586
Update a local copy of the PR:
$ git checkout pull/10586
$ git pull https://git.openjdk.org/jdk pull/10586/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 10586
View PR using the GUI difftool:
$ git pr show -t 10586
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10586.diff