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*) #2738

Closed
wants to merge 4 commits into from

Conversation

@mgronlun
Copy link

@mgronlun mgronlun commented Feb 26, 2021

Hi Denghui,

I did not manage to find a solution that would could reduce verbosity, but i did find one, although a bit involved, that lets us keep it local to and integrated in the core of JFR itself.

Can you do a test run with this suggestion to see if it works for you?

Thanks
Markus


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

Contributors

  • Denghui Dong <ddong@openjdk.org>

Download

To checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/2738/head:pull/2738
$ git checkout pull/2738

To update a local copy of the PR:
$ git checkout pull/2738
$ git pull https://git.openjdk.java.net/jdk pull/2738/head

@mgronlun
Copy link
Author

@mgronlun mgronlun commented Feb 26, 2021

/label hotspot-jfr

Loading

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 26, 2021

👋 Welcome back mgronlun! 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
Copy link

@openjdk openjdk bot commented Feb 26, 2021

@mgronlun
The hotspot-jfr label was successfully added.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 26, 2021

Webrevs

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 26, 2021

Mailing list message from Denghui Dong on hotspot-jfr-dev:

Of course, but I need some time to backport it to 8 and 11 to verify if our applications can work normally
since your changes involve jfrTypeSet whose logic has some difference between 8/11 and upstream.

Thanks,
denghui

------------------------------------------------------------------
From:Markus Gr?nlund <mgronlun at openjdk.java.net>
Send Time:2021?2?26?(???) 08:24
To:hotspot-jfr-dev <hotspot-jfr-dev at openjdk.java.net>
Subject:RFR: 8260589: Crash in JfrTraceIdLoadBarrier::load(_jclass*)

Hi Denghui,

I did not manage to find a solution that would could reduce verbosity, but i did find one, although a bit involved, that lets us keep it local to and integrated in the core of JFR itself.

Can you do a test run with this suggestion to see if it works for you?

Thanks
Markus

-------------

Commit messages:
- 8260589

Changes: https://git.openjdk.java.net/jdk/pull/2738/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2738&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8260589
Stats: 215 lines in 7 files changed: 204 ins; 3 del; 8 mod
Patch: https://git.openjdk.java.net/jdk/pull/2738.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2738/head:pull/2738

PR: https://git.openjdk.java.net/jdk/pull/2738

Loading

const oop mirror = JNIHandles::resolve(jc);
assert(mirror != NULL, "invariant");
const Klass* const k = java_lang_Class::as_Klass(mirror);
return k != NULL ? load(k) : load_primitive(mirror);
Copy link
Contributor

@D-D-H D-D-H Mar 1, 2021

Choose a reason for hiding this comment

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

I think the same modification should be made in JfrTraceId::load_raw.

Loading

Copy link
Author

@mgronlun mgronlun Mar 12, 2021

Choose a reason for hiding this comment

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

Thanks, Denghui, I will make the same change there. I will also put you down as a contributor to this change set.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 2, 2021

Mailing list message from Denghui Dong on hotspot-jfr-dev:

Hi Markus,

This patch is also worked in JDK 11 but needs some adjustments.
I think a proper review should be done for the backport after this PR is integrated.

Best Regards,
Denghui

------------------------------------------------------------------
From:Markus Gr?nlund <mgronlun at openjdk.java.net>
Send Time:2021?2?26?(???) 23:31
To:hotspot-jfr-dev <hotspot-jfr-dev at openjdk.java.net>
Subject:Re: RFR: 8260589: Crash in JfrTraceIdLoadBarrier::load(_jclass*) [v2]

Hi Denghui,

I did not manage to find a solution that would could reduce verbosity, but i did find one, although a bit involved, that lets us keep it local to and integrated in the core of JFR itself.

Can you do a test run with this suggestion to see if it works for you?

Thanks
Markus

Markus Gr?nlund has updated the pull request incrementally with two additional commits since the last revision:

- cds hook
- skip return value

-------------

Changes:
- all: https://git.openjdk.java.net/jdk/pull/2738/files
- new: https://git.openjdk.java.net/jdk/pull/2738/files/2d08073d..ce1ed38

Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2738&range=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2738&range=00-01

Stats: 6 lines in 2 files changed: 4 ins; 1 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/2738.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2738/head:pull/2738

PR: https://git.openjdk.java.net/jdk/pull/2738

Loading

@mgronlun
Copy link
Author

@mgronlun mgronlun commented Mar 12, 2021

/contributor add @D-D-H

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Mar 12, 2021

@mgronlun
Contributor Denghui Dong <ddong@openjdk.org> successfully added.

Loading

@D-D-H
Copy link
Contributor

@D-D-H D-D-H commented Mar 19, 2021

LGTM, although I'm not a reviewer :)

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Mar 19, 2021

@jbachorik Unknown command review - for a list of valid commands use /help.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Mar 19, 2021

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

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

Co-authored-by: Denghui Dong <ddong@openjdk.org>
Reviewed-by: jbachorik, egahlin

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

  • b49c589: 8263658: Use the blessed modifier order in java.base
  • 1572f3c: 8263852: Unused method SoftRefPolicy::use_should_clear_all_soft_refs
  • 57497ab: 8263821: Remove unused MethodTypeForm canonicalization codes
  • 4d51a82: 8263818: Release JNI local references in get/set-InetXXAddress-member helper functions of net_util.c
  • 701fd9d: 8262476: Add filter to speed up CompileCommand lookup
  • 454af87: 8263185: Mallinfo deprecated in glibc 2.33
  • d24e4cf: 8263481: Specification of JComponent::setDefaultLocale doesn't mention that passing 'null' restores VM's default locale
  • 1a21f77: 8263482: Make access to the ICC color profiles data multithread-friendly
  • d185655: 8263832: Shenandoah: Fixing parallel thread iteration in final mark task
  • 434a399: 8260274: Cipher.init(int, key) does not use highest priority provider for random bytes
  • ... and 305 more: https://git.openjdk.java.net/jdk/compare/c54724da143bb96da9c3534539bc789847858464...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.

Loading

@openjdk openjdk bot added the ready label Mar 19, 2021
@mgronlun
Copy link
Author

@mgronlun mgronlun commented Mar 19, 2021

Thanks for reviewing!

Loading

@mgronlun
Copy link
Author

@mgronlun mgronlun commented Mar 22, 2021

/integrate

Loading

@openjdk openjdk bot closed this Mar 22, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Mar 22, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 22, 2021

@mgronlun Since your change was applied there have been 335 commits pushed to the master branch:

  • 42104e5: 8263488: Verify CWarningWindow works with metal rendering pipeline
  • 5a7f22a: 8263579: ZGC: Concurrent mark hangs with debug loglevel
  • 35cd945: 8263908: Build fails due to initialize_static_field_for_dump defined but not used after JDK-8263771
  • cd45538: 8263771: Refactor javaClasses initialization code to isolate dumping code
  • 118a49f: 8263846: Bad JNI lookup getFocusOwner in accessibility code on Mac OS X
  • cb742f9: 8255255: Update Apache Santuario (XML Signature) to version 2.2.1
  • d2c137d: 8263558: Possible NULL dereference in fast path arena free if ZapResourceArea is true
  • ab66d69: 8263138: Initialization of sun.font.SunFontManager.platformFontMap is not thread safe
  • 5b8233b: 8263871: On sem_destroy() failing we should assert
  • 96e5c3f: 8263890: Broken links to Unicode.org
  • ... and 325 more: https://git.openjdk.java.net/jdk/compare/c54724da143bb96da9c3534539bc789847858464...master

Your commit was automatically rebased without conflicts.

Pushed as commit a9d2267.

💡 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