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

8293143: Workaround for JDK-8292217 when doing "step over" of bytecode with unresolved cp reference #10091

Closed
wants to merge 1 commit into from

Conversation

plummercj
Copy link
Contributor

@plummercj plummercj commented Aug 31, 2022

There is a workaround we can do for JDK-8292217 for the use case where a step over or step out is being done. This workaround can't be made to also work for the step into case. From JDK-8292217

"There is a workaround that fixes this issue when doing a STEP_OVER or STEP_OUT. Rather than the debug agent checking if it has enabled JVMTI single stepping, instead it checks some fields in the ThreadNode that say if single stepping is pending, and it is for a STEP_INTO. If it is not STEP_INTO, then it can assume that no StepEvent will occur at the same location and therefor the MethodEntryEvent should not be deferred. So this limits the bug to only happening when doing a STEP_INTO. "


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8293143: Workaround for JDK-8292217 when doing "step over" of bytecode with unresolved cp reference

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10091/head:pull/10091
$ git checkout pull/10091

Update a local copy of the PR:
$ git checkout pull/10091
$ git pull https://git.openjdk.org/jdk pull/10091/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10091

View PR using the GUI difftool:
$ git pr show -t 10091

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10091.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 31, 2022

👋 Welcome back cjplummer! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot changed the title 8293143 8293143: Workaround for JDK-8292217 when doing "step over" of bytecode with unresolved cp referernce Aug 31, 2022
@openjdk openjdk bot added the rfr Pull request is ready for review label Aug 31, 2022
@openjdk
Copy link

openjdk bot commented Aug 31, 2022

@plummercj The following label will be automatically applied to this pull request:

  • serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the serviceability serviceability-dev@openjdk.org label Aug 31, 2022
@mlbridge
Copy link

mlbridge bot commented Aug 31, 2022

Webrevs

@plummercj
Copy link
Contributor Author

Ping!

1 similar comment
@plummercj
Copy link
Contributor Author

Ping!

}
System.out.println("TESTCASE #" + testcase + " FAILED" +
(testcase == 2 ? "(ignoring)" : "") +
": too many events in EventSet: " + set.size());
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment says there is a workaround in place allowing the 1st test case to pass.
However, the 1st test case is marked as failed at the line 222 and the produced message says it is ignoring the 2nd test case. I'm confused. Do I miss anything?

Copy link
Contributor Author

@plummercj plummercj Sep 20, 2022

Choose a reason for hiding this comment

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

testcase #2 is expected to fail, so that is why we do not set "testFailed = true" at line 222. However, if testcase #1 fails, we do want the overall test to fail, so in that case we set "testFailed = true" at line 222.

Copy link
Contributor

@sspitsyn sspitsyn left a comment

Choose a reason for hiding this comment

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

The workaround looks good.
Thanks,
Serguei

@plummercj
Copy link
Contributor Author

I could still use one more review for this PR. Thanks!

@plummercj
Copy link
Contributor Author

Thanks Alex and Serguei!

/integrate

@openjdk
Copy link

openjdk bot commented Sep 27, 2022

@plummercj This pull request has not yet been marked as ready for integration.

@plummercj plummercj changed the title 8293143: Workaround for JDK-8292217 when doing "step over" of bytecode with unresolved cp referernce 8293143: Workaround for JDK-8292217 when doing "step over" of bytecode with unresolved cp reference Sep 27, 2022
@openjdk
Copy link

openjdk bot commented Sep 27, 2022

@plummercj 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:

8293143: Workaround for JDK-8292217 when doing "step over" of bytecode with unresolved cp reference

Reviewed-by: sspitsyn, amenkov

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 330 new commits pushed to the master branch:

  • 763d4bf: 8293592: Remove JVM_StopThread, stillborn, and related cleanup
  • 739fdec: 8289162: runtime/NMT/ThreadedMallocTestType.java should print out memory allocations to help debug
  • a11477c: 8289797: tools/launcher/I18NArgTest.java fails on Japanese Windows environment
  • 7151128: 8294317: Insufficient build rules for tzdb.dat
  • fb4979c: 8290401: Support dump all phases and print nodes in ascending order of index
  • 112ca2b: 8293964: Unused check_for_duplicates parameter in ClassLoaderExt::process_jar_manifest
  • 99017b0: 8293064: Remove unused NET_xxx functions
  • 3419363: 8294361: Cleanup usages of StringBuffer in SQLOutputImpl
  • 1abf971: 8249627: Degrade Thread.suspend and Thread.resume
  • bc12e95: 8292969: Bad Thread Utilization in ForkJoinPool
  • ... and 320 more: https://git.openjdk.org/jdk/compare/1cf245d77c0948b6f03025e80faa4517a6f79f3f...master

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 master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Sep 27, 2022
@plummercj
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Sep 27, 2022

Going to push as commit 6ad151d.
Since your change was applied there have been 331 commits pushed to the master branch:

  • 22b59b6: 8294471: SpecTaglet is inconsistent with SpecTree for inline property
  • 763d4bf: 8293592: Remove JVM_StopThread, stillborn, and related cleanup
  • 739fdec: 8289162: runtime/NMT/ThreadedMallocTestType.java should print out memory allocations to help debug
  • a11477c: 8289797: tools/launcher/I18NArgTest.java fails on Japanese Windows environment
  • 7151128: 8294317: Insufficient build rules for tzdb.dat
  • fb4979c: 8290401: Support dump all phases and print nodes in ascending order of index
  • 112ca2b: 8293964: Unused check_for_duplicates parameter in ClassLoaderExt::process_jar_manifest
  • 99017b0: 8293064: Remove unused NET_xxx functions
  • 3419363: 8294361: Cleanup usages of StringBuffer in SQLOutputImpl
  • 1abf971: 8249627: Degrade Thread.suspend and Thread.resume
  • ... and 321 more: https://git.openjdk.org/jdk/compare/1cf245d77c0948b6f03025e80faa4517a6f79f3f...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Sep 27, 2022
@openjdk openjdk bot closed this Sep 27, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 27, 2022
@openjdk
Copy link

openjdk bot commented Sep 27, 2022

@plummercj Pushed as commit 6ad151d.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@plummercj plummercj deleted the 8293143_cle_workaround branch October 11, 2022 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated serviceability serviceability-dev@openjdk.org
3 participants