8276227: ciReplay: SIGSEGV if classfile for replay compilation is not present after JDK-8275868 #6189

chhagedorn · 2021-11-01T13:25:03Z

The fix for JDK-8275868 does not handle the case when the classfile for the method to be replay compiled is not present. This will fail to load the klass. Afterwards, we are trying to access the protection domain of the failed to load klass (i.e. a null pointer) which results in a segmentation fault. The fix is straight forward to only set the new protection domain if the klass was loaded successfully. I additionally changed the code such that we are only trying to set the protection domain when reading the first instanceKlass entry. This avoids some potential problems with older replay files where we do not have this additional first entry set by JDK-8275868.

Thanks,
Christian

Progress

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

Issue

JDK-8276227: ciReplay: SIGSEGV if classfile for replay compilation is not present after JDK-8275868

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer) ⚠️ Review applies to 4fe9574
Tobias Hartmann (@TobiHartmann - Reviewer)
Dean Long (@dean-long - Reviewer) ⚠️ Review applies to 4fe9574

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6189

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

Using diff file

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

… present after JDK-8275868

bridgekeeper · 2021-11-01T13:25:22Z

👋 Welcome back chagedorn! 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 · 2021-11-01T13:27:13Z

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

hotspot-compiler

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.

mlbridge · 2021-11-01T13:32:19Z

Webrevs

01: Full - Incremental (1e608b85)
00: Full (4fe95743)

vnkozlov

Good.
Which test failed?

openjdk · 2021-11-01T18:32:49Z

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

8276227: ciReplay: SIGSEGV if classfile for replay compilation is not present after JDK-8275868

Reviewed-by: kvn, thartmann, dlong

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

6a04899: 8275840: Add test to java/nio/channels/Channels/TransferTo.java to test transfer sizes > 2GB
01105d6: 8276367: ProblemList vmTestbase/nsk/jvmti/RedefineClasses/StressRedefineWithoutBytecodeCorruption/TestDescription.java
8fc16f1: 8275729: Qualified method names in CodeHeap Analytics
fa4ce82: 8276260: (se) Remove java/nio/channels/Selector/Wakeup.java from ProblemList (win)
495c828: 8276188: Clarify "default charset" descriptions in String class
cd778f5: 8202667: java/awt/Debug/DumpOnKey/DumpOnKey.java times out on Windows
b889f2a: 8276175: codestrings.validate_vm gtest still broken on ppc64 after JDK-8276046
5b4e398: 8275766: (tz) Update Timezone Data to 2021e
8630f55: 8275406: Add copy-to-clipboard feature to snippet UI
9971a2c: 8275735: [linux] Remove deprecated Metrics api (kernel memory limit)
... and 15 more: https://git.openjdk.java.net/jdk/compare/5bb1992b8408a0d196b1afa308bc00d007458dbd...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.

chhagedorn · 2021-11-02T10:28:49Z

Thanks Vladimir for your review! Unfortunately, none of our tests caught this. I observed this when I did some additional testing and had not specified the correct class paths. I've added a test which covers this.

TobiHartmann

Looks good to me.

dean-long · 2021-11-01T19:49:22Z

src/hotspot/share/ci/ciReplay.cpp

+    // Only initialize the protection domain handle with the protection domain of the very first entry.
+    // This also ensures that older replay files work.
+    _protection_domain_initialized = true;
+


I don't see how this helps older replay files. In fact, it seems like it could make replay for older replay files fail, if the first entry has a different protection domain than the main class.

If we really want to preserve the old behavior of old replay files, then I think we need to add a version number or some other keyword so that we can tell if a replay file is old or not. However, in my opinion supporting old replay files should not be a goal.

I'm having second thoughts on not supporting old replay files. It's easy enough to add a version number, which allows us to introduce incompatible changes without breaking old replay files. I'll probably introduce a version number with my fix for 8276095.

You're right. A version number would solve this completely. This implementation is more robust than the previous one but not complete. Here we only try to set the protection domain once where in the previous implementation, we would have picked the first non-null protection domain found (which could happen after looking at many classes). Do you want to revisit this code with the introduction of version numbers in 8276095 and we proceed with this temporary fix?

Yes, go ahead with your fix.

Great, thanks Dean for your review!

chhagedorn · 2021-11-04T08:52:26Z

/integrate

openjdk · 2021-11-04T08:53:51Z

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

9eadcbb: 8276217: Harmonize StrictMath intrinsics handling
fb0be81: 8276096: Simplify Unsafe.{load|store}Fence fallbacks by delegating to fullFence
558ee40: 8276615: Update CR number of some tests in ProblemList-zgc.txt
603bba2: 8271420: Extend CDS custom loader support to Windows platform
ce8c767: 8276220: Reduce excessive allocations in DateTimeFormatter
0ab910d: 8276066: Reset LoopPercentProfileLimit for x86 due to suboptimal performance
f3320d2: 8276588: Change "ccc" to "CSR" in HotSpot sources
32895ac: 8275650: Problemlist java/io/File/createTempFile/SpecialTempFile.java for Windows 11
c7f070f: 8276208: vmTestbase/nsk/jdb/repeat/repeat001/repeat001.java fails with "AssertionError: Unexpected output"
684edbb: 8273922: (fs) UserDefinedFileAttributeView doesn't handle file names that are just under the MAX_PATH limit (win)
... and 38 more: https://git.openjdk.java.net/jdk/compare/5bb1992b8408a0d196b1afa308bc00d007458dbd...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-11-04T08:54:09Z

@chhagedorn Pushed as commit a1f4c42.

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

8276227: ciReplay: SIGSEGV if classfile for replay compilation is not…

4fe9574

… present after JDK-8275868

openjdk bot added the rfr Pull request is ready for review label Nov 1, 2021

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Nov 1, 2021

vnkozlov approved these changes Nov 1, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 1, 2021

add test

1e608b8

TobiHartmann approved these changes Nov 2, 2021

View reviewed changes

dean-long approved these changes Nov 2, 2021

View reviewed changes

openjdk bot closed this Nov 4, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8276227: ciReplay: SIGSEGV if classfile for replay compilation is not present after JDK-8275868 #6189

8276227: ciReplay: SIGSEGV if classfile for replay compilation is not present after JDK-8275868 #6189

chhagedorn commented Nov 1, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 1, 2021

openjdk bot commented Nov 1, 2021

mlbridge bot commented Nov 1, 2021 •

edited

Loading

vnkozlov left a comment

openjdk bot commented Nov 1, 2021 •

edited

Loading

chhagedorn commented Nov 2, 2021

TobiHartmann left a comment

dean-long Nov 1, 2021

dean-long Nov 3, 2021 •

edited

Loading

chhagedorn Nov 3, 2021

dean-long Nov 3, 2021

chhagedorn Nov 4, 2021

chhagedorn commented Nov 4, 2021

openjdk bot commented Nov 4, 2021

openjdk bot commented Nov 4, 2021

8276227: ciReplay: SIGSEGV if classfile for replay compilation is not present after JDK-8275868 #6189

8276227: ciReplay: SIGSEGV if classfile for replay compilation is not present after JDK-8275868 #6189

Conversation

chhagedorn commented Nov 1, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 1, 2021

openjdk bot commented Nov 1, 2021

mlbridge bot commented Nov 1, 2021 • edited Loading

Webrevs

vnkozlov left a comment

Choose a reason for hiding this comment

openjdk bot commented Nov 1, 2021 • edited Loading

chhagedorn commented Nov 2, 2021

TobiHartmann left a comment

Choose a reason for hiding this comment

dean-long Nov 1, 2021

Choose a reason for hiding this comment

dean-long Nov 3, 2021 • edited Loading

Choose a reason for hiding this comment

chhagedorn Nov 3, 2021

Choose a reason for hiding this comment

dean-long Nov 3, 2021

Choose a reason for hiding this comment

chhagedorn Nov 4, 2021

Choose a reason for hiding this comment

chhagedorn commented Nov 4, 2021

openjdk bot commented Nov 4, 2021

openjdk bot commented Nov 4, 2021

chhagedorn commented Nov 1, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 1, 2021 •

edited

Loading

openjdk bot commented Nov 1, 2021 •

edited

Loading

dean-long Nov 3, 2021 •

edited

Loading