8275610: C2: Object field load floats above its null check resulting in a segfault

[chhagedorn]

In the test case, a field load of an object floats above the object null check due to a CastPP that gets separated from its null check If node. C2 then schedules the field load before the object null check which results in a segfault.

The problem can be traced back to the elimination of identical back-to-back ifs (JDK-8143542). This is done in split-if where we detect identical back-to-back ifs in PhaseIdealLoop::identical_backtoback_ifs(). We then replace the boolean node of the dominated If by a phi to use the split-if optimization to nicely fold away the dominated If later in IGVN. This, however, does not update any data nodes that were dependent on the dominated If projections.

In the test case, we have the following graph right before splitting 313 If (NC 2) through 190 Region:

313 If is dominated by the identical (= both share 308 Bool) 309 If (NC 1). The bool input for 313 If is replaced by a phi and the split-if optimization is applied. However, the data nodes dependent on the out projections of the dominated 313 If (261 CastPP in this case) are not processed separately and just end up at the newly inserted regions in split-if. In the test case, we get the following graph where 261 CastPP ends up at the new 334 Region:

Loopopts are applied and can remove the 325 CountedLoop and we find that the 171 RangeCheck (RC 2) is applied on a constant array index 1.

Now at IGVN, the order in which the nodes are processed is important in order to trigger the segfault:

1. 334 Region with 332 If and 333 If are removed because of the special split-if setup we used to remove the identical 313 If. The control input of 261 CastPP is therefore updated to 172 IfTrue.
2. Applying RangeCheck::Ideal() for 171 RangeCheck finds that 305 RangeCheck (RC 1) already covers it and we remove 171 RangeCheck. In this process, we rewire 261 CastPP to the dominating 305 RangeCheck and we have the following graph:

261 CastPP - and also the field 263 LoadI - have now 306 IfFalse as early control. GCM is then scheduling 263 LoadI before the null check 309 If and we get a segfault.

An easy fix is not straight forward. What we actually would want to do is rewiring 261 CastPP from 334 Region to 311 IfTrue in the second graph after split-if to not separate it from the null check. But that's not possible because we would create a bad graph: The early control 311 IfTrue of 261 CastPP does not dominate its late control further down after 334 Region because of the not yet removed 334 Region. We would need to already clean the regions up and then do the rewiring. But then the question arises why to use the split-if optimization in the first place when we do not want to rely on IGVN to clean it up.

I therefore suggest to go with an easy bailout fix for JDK 18 where we do not apply this identical back-to-back if removal optimization if there are data dependencies and rework this in an RFE for JDK 19. Roland already has some ideas how to do that.

I ran some standard benchmarks and did not see any performance regressions with this fix.

Thanks,
Christian

---

Progress

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

Issue

• JDK-8275610: C2: Object field load floats above its null check resulting in a segfault

Reviewers

• Vladimir Kozlov (@vnkozlov - Reviewer)
• Roland Westrelin (@rwestrel - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6701

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

Using diff file

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

[bridgekeeper]

👋 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]

@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]

Webrevs

• 00: Full (df10ae8d)

[vnkozlov]

I agree with bailout for now and proper fix it later.

[openjdk]

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

8275610: C2: Object field load floats above its null check resulting in a segfault

Reviewed-by: kvn, roland

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

• f180a45: 8278016: Add compiler tests to tier{2,3}
• 104aa1f: 8268575: Annotations not visible on model elements before they are generated
• 839b606: 8278143: Remove unused "argc" from ConstantPool::copy_bootstrap_arguments_at_impl
• 267c024: 8265150: AsyncGetCallTrace crashes on ResourceMark
• 9642629: 8276779: (ch) InputStream returned by Channels.newInputStream should have fast path for SelectableChannels
• 02ee337: 8278175: Enable all doclint warnings for build of java.desktop
• 24e16ac: 8277617: Adjust AVX3Threshold for copy/fill stubs
• 2b87c2b: 8277793: Support vector F2I and D2L cast operations for X86
• e1cde19: 8278247: KeyStoreSpi::engineGetAttributes does not throws KeyStoreException
• a729a70: 8225181: KeyStore should have a getAttributes method
• ... and 269 more: https://git.openjdk.java.net/jdk/compare/b8d33a2a4e4ac1be322644102e8f09ce1435b4fb...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]

Thanks Vladimir for your review!

[rwestrel]

Looks good to me.

[chhagedorn]

Thanks Roland for your review!

[chhagedorn]

/integrate

[openjdk]

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

• a885aab: 8276125: RunThese24H.java SIGSEGV in JfrThreadGroup::thread_group_id
• 6994d80: 8278291: compiler/uncommontrap/TraceDeoptimizationNoRealloc.java fails with release VMs after JDK-8154011
• 286a26c: 8278277: G1: Simplify implementation of G1GCPhaseTimes::record_or_add_time_secs
• d14f06a: 8278031: MultiThreadedRefCounter should not use relaxed atomic decrement
• 8d190dd: 8277496: Remove duplication in c1 Block successor lists
• 194cdf4: 8277864: Compilation error thrown while doing a boxing conversion on selector expression
• f39fe5b: 8154011: Make TraceDeoptimization a diagnostic flag
• f180a45: 8278016: Add compiler tests to tier{2,3}
• 104aa1f: 8268575: Annotations not visible on model elements before they are generated
• 839b606: 8278143: Remove unused "argc" from ConstantPool::copy_bootstrap_arguments_at_impl
• ... and 276 more: https://git.openjdk.java.net/jdk/compare/b8d33a2a4e4ac1be322644102e8f09ce1435b4fb...master

Your commit was automatically rebased without conflicts.

[openjdk]

@chhagedorn Pushed as commit 7c6f57f.

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