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

8264395: WB_EnqueueInitializerForCompilation fails with "method holder must be initialized" when called for uninitialized class #3757

Closed
wants to merge 3 commits into from

Conversation

@r-v-raghav
Copy link
Member

@r-v-raghav r-v-raghav commented Apr 28, 2021

https://bugs.openjdk.java.net/browse/JDK-8264395

Reported assert failure, during compilation triggered by Whitebox API enqueueInitializerForCompilation method,
is fixed by adding support to to bail out if a class not initialized yet.

Confirmed the new compiler/whitebox/TestEnqueueInitializerForCompilation.java test passes with the proposed whitebox.cpp changes and fails with reported assert without this fix.
No issues with tier tests.


Progress

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

Issue

  • JDK-8264395: WB_EnqueueInitializerForCompilation fails with "method holder must be initialized" when called for uninitialized class

Reviewers

Reviewing

Using git

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

Update a local copy of the PR:
$ git checkout pull/3757
$ git pull https://git.openjdk.java.net/jdk pull/3757/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3757

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3757.diff

… must be initialized when called for uninitialized class
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 28, 2021

👋 Welcome back rraghavan! 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 Apr 28, 2021

@r-v-raghav The following label will be automatically applied to this pull request:

  • hotspot

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.

Loading

@openjdk openjdk bot added the hotspot label Apr 28, 2021
@r-v-raghav r-v-raghav changed the title 8264395: WB_EnqueueInitializerForCompilation fails with method holder… 8264395: WB_EnqueueInitializerForCompilation fails with "method holder must be initialized" when called for uninitialized class Apr 28, 2021
@r-v-raghav
Copy link
Member Author

@r-v-raghav r-v-raghav commented Apr 28, 2021

/label remove hotspot
/label add hotspot-compiler

Loading

@openjdk openjdk bot removed the hotspot label Apr 28, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 28, 2021

@r-v-raghav
The hotspot label was successfully removed.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 28, 2021

@r-v-raghav
The hotspot-compiler label was successfully added.

Loading

…r must be initialized" when called for uninitialized class
@r-v-raghav r-v-raghav marked this pull request as ready for review Apr 28, 2021
@openjdk openjdk bot added the rfr label Apr 28, 2021
Copy link
Member

@chhagedorn chhagedorn left a comment

Looks good!

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 28, 2021

Webrevs

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 28, 2021

@r-v-raghav 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:

8264395: WB_EnqueueInitializerForCompilation fails with "method holder must be initialized" when called for uninitialized class

Reviewed-by: chagedorn, thartmann

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

  • 4d77171: 8249903: jdk/javadoc/doclet/testSerializedForm/TestSerializedForm.java needs to be updated after 8146022 got closed
  • 51b2188: 8266267: Remove unnecessary jumps in Intel Math Library StubRoutines
  • 2c381e0: 8262376: ReplaceCriticalClassesForSubgraphs.java fails if --with-build-jdk is used
  • 5ecef01: 8266217: ZGC: Improve the -Xlog:gc+init output for NUMA
  • 5d8c1cc: 8255410: Add ChaCha20 and Poly1305 support to SunPKCS11 provider
  • 46b4a14: 8266315: Problem list failing test java/awt/font/TextLayout/LigatureCaretTest.java
  • 42af7da: 8265933: Move Java monitor related fields from class Thread to JavaThread
  • 1afbab6: 8263998: Remove mentions of mc region in comments
  • 51b2fb5: 8266299: ProblemList runtime/stringtable/StringTableCleaningTest.java on linux-aarch64 with ZGC
  • 49d0458: 8266288: assert root method not found in witnessed_reabstraction_in_supers is too strong
  • ... and 76 more: https://git.openjdk.java.net/jdk/compare/dc323a933401e4e540cdc416de5260923f42ba7f...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 Apr 28, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 28, 2021

@r-v-raghav 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:

8264395: WB_EnqueueInitializerForCompilation fails with "method holder must be initialized" when called for uninitialized class

Reviewed-by: chagedorn

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

  • 343a4a7: 8185127: Add tests to cover hashCode() method for java supported crypto key types
  • e325a75: 8264593: debug.cpp utilities should be available in product builds.
  • e879f8c: 8265587: IGV: track nodes across matching
  • 164454f: 8265867: thread.hpp defines some enums but no reference
  • 75a2354: 8266028: C2 computes -0.0 for Math.pow(-0.0, 0.5)
  • ca37be1: 8197800: Test java/awt/Focus/NonFocusableWindowTest/NoEventsTest.java fails on Windows
  • cf92693: 8198619: java/awt/Focus/FocusTraversalPolicy/ButtonGroupLayoutTraversal/ButtonGroupLayoutTraversalTest.java fails on mac
  • 2201e11: 8266055: ZGC: ZHeap::print_extended_on() doesn't disable deferred delete
  • ce48f04: 8198617: java/awt/Focus/6382144/EndlessLoopTest.java fails on mac
  • 0438cea: 8136517: [macosx]Test java/awt/Focus/8073453/AWTFocusTransitionTest.java fails on MacOSX
  • ... and 22 more: https://git.openjdk.java.net/jdk/compare/dc323a933401e4e540cdc416de5260923f42ba7f...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

Loading
Loading
Loading
Loading
public class TestEnqueueInitializerForCompilation {

public static void main(String[] args) {
WhiteBox.getWhiteBox().enqueueInitializerForCompilation(LongWrapper.class, 4);
Copy link
Member

@TobiHartmann TobiHartmann Apr 28, 2021

Choose a reason for hiding this comment

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

What if compilation level 4 (= C2) is not available?

Loading

Copy link
Member Author

@r-v-raghav r-v-raghav Apr 29, 2021

Choose a reason for hiding this comment

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

Found unavailable/bad comp levels handled in WhiteBox::compile_method() or later in CompileBroker::compile_method.

Loading

Copy link
Member

@TobiHartmann TobiHartmann Apr 29, 2021

Choose a reason for hiding this comment

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

Right but wouldn't we hit an assert there if C2 is not available?

Loading

Copy link
Member Author

@r-v-raghav r-v-raghav Apr 30, 2021

Choose a reason for hiding this comment

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

For the case scenario mentioned found that the test simply displays a WB whitebox error that is ignored.
So seems we can go ahead with the current test case itself. Thanks

Loading

Copy link
Member

@TobiHartmann TobiHartmann Apr 30, 2021

Choose a reason for hiding this comment

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

Yes, as we've discussed off-thread, that's fine. Thanks for investigating.

Loading

…r must be initialized" when called for uninitialized class
@r-v-raghav
Copy link
Member Author

@r-v-raghav r-v-raghav commented Apr 29, 2021

Thank you @TobiHartmann for the comment suggestions. Please note now tried revising the fix for further review.

Loading

@r-v-raghav
Copy link
Member Author

@r-v-raghav r-v-raghav commented Apr 30, 2021

/integrate

Loading

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

@openjdk openjdk bot commented Apr 30, 2021

@r-v-raghav Since your change was applied there have been 86 commits pushed to the master branch:

  • 4d77171: 8249903: jdk/javadoc/doclet/testSerializedForm/TestSerializedForm.java needs to be updated after 8146022 got closed
  • 51b2188: 8266267: Remove unnecessary jumps in Intel Math Library StubRoutines
  • 2c381e0: 8262376: ReplaceCriticalClassesForSubgraphs.java fails if --with-build-jdk is used
  • 5ecef01: 8266217: ZGC: Improve the -Xlog:gc+init output for NUMA
  • 5d8c1cc: 8255410: Add ChaCha20 and Poly1305 support to SunPKCS11 provider
  • 46b4a14: 8266315: Problem list failing test java/awt/font/TextLayout/LigatureCaretTest.java
  • 42af7da: 8265933: Move Java monitor related fields from class Thread to JavaThread
  • 1afbab6: 8263998: Remove mentions of mc region in comments
  • 51b2fb5: 8266299: ProblemList runtime/stringtable/StringTableCleaningTest.java on linux-aarch64 with ZGC
  • 49d0458: 8266288: assert root method not found in witnessed_reabstraction_in_supers is too strong
  • ... and 76 more: https://git.openjdk.java.net/jdk/compare/dc323a933401e4e540cdc416de5260923f42ba7f...master

Your commit was automatically rebased without conflicts.

Pushed as commit 3554dc2.

💡 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
3 participants