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

8260589: Crash in JfrTraceIdLoadBarrier::load(_jclass*) #47

Closed
wants to merge 2 commits into from

Conversation

@D-D-H
Copy link

@D-D-H D-D-H commented Jun 21, 2021

Could I have a review of this backport that fixes a crash problem?

Although there are many conflicts, I think it's necessary to backport it to 11u since the problem is very easy to reproduce.

And I hope JFR's folks could review it.

In addition to the different context of modified files, I think the following items we should review carefully:

  1. use MaxJfrEventId + 101 instead of LAST_TYPE_ID + 1 as the klass id of void.class
  2. jdk 11 doesn't support jfr streaming, so I removed the call JfrTraceIdEpoch::set_changed_tag_state() in load_primitive
  3. the Class in jdk 11 doesn't have the field 'hidden', so I removed writer->write<bool>(false); in write_primitive
  4. there are many differences in the API of JfrTraceId between 11u and tip

Thanks,
Denghui


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8260589: Crash in JfrTraceIdLoadBarrier::load(_jclass*)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk11u-dev pull/47/head:pull/47
$ git checkout pull/47

Update a local copy of the PR:
$ git checkout pull/47
$ git pull https://git.openjdk.java.net/jdk11u-dev pull/47/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 47

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk11u-dev/pull/47.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jun 21, 2021

👋 Welcome back ddong! 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.

Loading

@openjdk openjdk bot changed the title Backport a9d2267f8d306522522c999ff584ccaa34c46456 8260589: Crash in JfrTraceIdLoadBarrier::load(_jclass*) Jun 21, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 21, 2021

This backport pull request has now been updated with issue from the original commit.

Loading

@D-D-H
Copy link
Author

@D-D-H D-D-H commented Jun 21, 2021

testing:
release & slowdebug jtreg jdk/jfr

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 21, 2021

Webrevs

Loading

@D-D-H
Copy link
Author

@D-D-H D-D-H commented Jul 1, 2021

@mgronlun @egahlin
Sorry for messaging you directly.
Could you help review this backport when you have time?
This problem is easy to trigger.

Thanks,
Denghui

Loading

@RealCLanger
Copy link
Contributor

@RealCLanger RealCLanger commented Jul 2, 2021

Hi @D-D-H,
Unfortunately I can't comment so much on the actual changes in JFR coding. However, I ran your patch through our testing and couldn't spot any regressions. So I'm approving it. However, I think it would be good if Markus or Erik (or any other JFR expert) could throw an eye on it.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jul 2, 2021

@D-D-H This change now passes all automated pre-integration checks.

After integration, the commit message for the final commit will be:

8260589: Crash in JfrTraceIdLoadBarrier::load(_jclass*)

Reviewed-by: clanger, mgronlun

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

  • d424c0c: 8267396: Avoid recording "pc" in unhandled oops detector for better performance
  • d07f6d9: 8227766: CheckUnhandledOops is broken in MemAllocator
  • fdaa4f0: 8255810: Zero: build fails without JVMTI
  • 3f03476: 8224853: CDS address sanitizer errors
  • 65b215a: 8259338: Add expiry exception for identrustdstx3 alias to VerifyCACerts.java test
  • c61cfb5: 8255718: Zero: VM should know it runs in interpreter-only mode
  • 3e09f94: 8260923: Add more tests for SSLSocket input/output shutdown
  • c00a72f: 8253631: Remove unimplemented CompileBroker methods after JEP-165
  • 21345ef: 8269847: JDK-8269594 backport breaks 11u builds
  • 73e987c: 8269594: assert(_handle_mark_nesting > 1) failed: memory leak: allocating handle outside HandleMark
  • ... and 39 more: https://git.openjdk.java.net/jdk11u-dev/compare/0605504ec99448158590d0d7d7c54416ca95460f...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@RealCLanger, @mgronlun) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

Loading

@openjdk openjdk bot added the ready label Jul 2, 2021
@D-D-H
Copy link
Author

@D-D-H D-D-H commented Jul 2, 2021

Hi @D-D-H,
Unfortunately I can't comment so much on the actual changes in JFR coding. However, I ran your patch through our testing and couldn't spot any regressions. So I'm approving it. However, I think it would be good if Markus or Erik (or any other JFR expert) could throw an eye on it.

Thank you.
Yes, I also think this backport should be reviewed properly by JFR folks.

Loading

@egahlin
Copy link
Member

@egahlin egahlin commented Jul 2, 2021

Hi @D-D-H,
Unfortunately I can't comment so much on the actual changes in JFR coding. However, I ran your patch through our testing and couldn't spot any regressions. So I'm approving it. However, I think it would be good if Markus or Erik (or any other JFR expert) could throw an eye on it.

Thank you.
Yes, I also think this backport should be reviewed properly by JFR folks.

I think it will have to wait for a few weeks. People in Sweden like to take their vacation during the summer :-)

Loading

Loading
Loading
Loading
@D-D-H
Copy link
Author

@D-D-H D-D-H commented Jul 6, 2021

@mgronlun
Thank you, please help review the latest version.

Loading

Copy link
Contributor

@mgronlun mgronlun left a comment

Looks good.

Loading

@D-D-H
Copy link
Author

@D-D-H D-D-H commented Jul 6, 2021

/integrate

Loading

@openjdk openjdk bot added the sponsor label Jul 6, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 6, 2021

@D-D-H
Your change (at version b171d18) is now ready to be sponsored by a Committer.

Loading

@RealCLanger
Copy link
Contributor

@RealCLanger RealCLanger commented Jul 6, 2021

/integrate

@D-D-H you (still) need to flag the JBS item with jdk11u-fix-request and add a comment.

I wil run the updated version through our testing once more and then I'm ready to approve & sponsor after JBS bug was flagged. Thanks.

Loading

@D-D-H
Copy link
Author

@D-D-H D-D-H commented Jul 6, 2021

/integrate

@D-D-H you (still) need to flag the JBS item with jdk11u-fix-request and add a comment.

I wil run the updated version through our testing once more and then I'm ready to approve & sponsor after JBS bug was flagged. Thanks.

Thanks for reminding me.
Tagged.

Loading

@RealCLanger
Copy link
Contributor

@RealCLanger RealCLanger commented Jul 7, 2021

/sponsor

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jul 7, 2021

Going to push as commit 1d204c5.
Since your change was applied there have been 50 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Loading

@openjdk openjdk bot closed this Jul 7, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jul 7, 2021
@openjdk openjdk bot removed the sponsor label Jul 7, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 7, 2021

@RealCLanger @D-D-H Pushed as commit 1d204c5.

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

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants