8367967: C2: "fatal error: Not monotonic" with Mod nodes #27408

SirYwell · 2025-09-21T06:11:11Z

Generally, we shouldn't return a wider type (ZERO) if there is a later case that would return a more narrow type (TOP) for the same input types. If the inputs are widened and the first case doesn't match anymore but the later one still does, the result is not monotonic with the previous result.

Please review :)

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8367967: C2: "fatal error: Not monotonic" with Mod nodes (Bug - P3)

Reviewers

Christian Hagedorn (@chhagedorn - Reviewer) Review applies to 9ba78d4e
Aleksey Shipilev (@shipilev - Reviewer) Review applies to 9ba78d4e
Benoît Maillard (@benoitmaillard - Author)
Vladimir Ivanov (@iwanowww - Reviewer)

Contributors

Christian Hagedorn <[email protected]>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27408/head:pull/27408
$ git checkout pull/27408

Update a local copy of the PR:
$ git checkout pull/27408
$ git pull https://git.openjdk.org/jdk.git pull/27408/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27408

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27408.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-09-21T06:12:09Z

👋 Welcome back hgreule! 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 · 2025-09-21T06:12:29Z

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

8367967: C2: "fatal error: Not monotonic" with Mod nodes

Co-authored-by: Christian Hagedorn <[email protected]>
Reviewed-by: bmaillard, vlivanov, chagedorn, shade

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

a663812: 8368124: Show useful thread names in ASAN reports
ca03080: 8368030: Make package bundlers stateless
648582a: 8368714: [BACKOUT] JDK-8368468 Split out everything but configure results from spec.gmk
... and 127 more: https://git.openjdk.org/jdk/compare/94a301a70e19be284f406ebb6d8b94b6f96e1a24...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.

SirYwell · 2025-09-21T06:13:13Z

/contributor add @chhagedorn

Thanks for the test case!

openjdk · 2025-09-21T06:13:22Z

@SirYwell 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.

openjdk · 2025-09-21T06:13:58Z

@SirYwell
Contributor Christian Hagedorn <[email protected]> successfully added.

mlbridge · 2025-09-21T06:19:56Z

Webrevs

MBaesken · 2025-09-22T06:48:45Z

I'll test this in our CI to see if this fixes the linux aarch64 issues (observed when running Test java/foreign/TestUpcallStress.java ) .

MBaesken · 2025-09-22T06:49:49Z

Btw. why do we get always zero size replay files when running into the issue ?

# Internal Error (/priv/jenkins/client-home/workspace/openjdk-jdk-dev-linux_aarch64-dbg/jdk/src/hotspot/share/opto/phaseX.cpp:2763), pid=1089937, tid=1089972
# fatal error: Not monotonic

Is it another bug of the replay file generation or a known limitation ?

chhagedorn

The fix looks good to me, thanks for the fix and the credit for the test!

I'll give it a spin in our testing as well.

chhagedorn · 2025-09-22T07:06:44Z

Btw. why do we get always zero size replay files when running into the issue ?
# Internal Error (/priv/jenkins/client-home/workspace/openjdk-jdk-dev-linux_aarch64-dbg/jdk/src/hotspot/share/opto/phaseX.cpp:2763), pid=1089937, tid=1089972
# fatal error: Not monotonic
Is it another bug of the replay file generation or a known limitation ?

We've encountered empty replay files before which could be traced back to a timeout in error reporting due to threads being stuck. We filed JDK-8297588 for it but it's not fixed, yet. I did a closer investigation back there (see summary). You might be hitting the same issue.

shipilev

This makes sense. I was about to ask what would be the result of 0 mod 0 then, but I see it is also covered: we return TOP on any X mod 0 early on.

benoitmaillard

Looks good to me, I only have one minor comment.

benoitmaillard · 2025-09-22T13:23:36Z

test/hotspot/jtreg/compiler/c2/TestModValueMonotonic.java

+ * @test
+ * @bug 8367967
+ * @summary Ensure ModI/LNode::Value is monotonic with potential divison by 0
+ * @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:CompileOnly=compiler.c2.TestModValueMonotonic::test*


You could probably add another @run main ... without flags to potentially catch other things in the future

SirYwell · 2025-09-22T21:37:03Z

I added the suggestion from @benoitmaillard and fixed a typo in the test summary. Please let me know when the test results are in :)

chhagedorn · 2025-09-23T07:46:49Z

test/hotspot/jtreg/compiler/ccp/TestModValueMonotonic.java

@@ -0,0 +1,66 @@
+/*


Another thing: You could move this test to compiler/ccp which fits better than the generic c2 folder.

iwanowww · 2025-09-23T14:12:40Z

src/hotspot/share/opto/divnode.cpp

  if (t1 == Type::TOP) { return Type::TOP; }
  if (t2 == Type::TOP) { return Type::TOP; }

+  // Mod by zero?  Throw exception at runtime!


The comment is a bit confusing. It's not the node itself which produces the exception, but a dominating zero check (inserted during parsing). So, if a divisor becomes 0, it means the node is effectively dead and can go away.

Also, the node should go away anyway as part of CFG pruning of dead branches when corresponding guard goes away.

BTW if there are cases when control is not eliminated, it may irrevocably break the IR causing crashes down the road (take a look at JDK-8154831 as an example). So, maybe it's safer to just rely on dead control pruning to eliminate effectively dead ModI/ModL nodes and assert that there are no effectively dead ModI/ModL nodes present after GVN pass is over.

The comment comes from the original code before my change in #25254, where that path also returned POS but that wasn't monotonic with my changes anymore.

So, if a divisor becomes 0, it means the node is effectively dead and can go away.

I think this check mostly comes down to CCP. We need to return something for a zero divisor, and that something has to be monotonic with subsequent wider inputs.

If you agree with that observation, I can change the comment to better reflect what's going on, e.g., Mod by zero can be observed in PhaseCCP, return TOP to ensure monotonic results (I'm open for other suggestions).

Thanks for the clarifications. I thought about it for some time, but as things work now, I don't see a better alternative except just ignoring 0 divisor case. So, please, proceed with the fix as it is now.

Alternatively, to improve robustness, a dead ModI/ModL can kill dependent control akin to what Roland did for Type nodes with JDK-8349479.

I don't see a better alternative except just ignoring 0 divisor case

That probably also works. It seems that for DivI/L, we already ignore this case as well.

The question is: What is better when the zero check is not folded but we observe zero for the divisor: Having top to possibly corrupt the graph or just possibly risking miscompilation/div by zero crashes at runtime when the zero check is really off - but not folding the zero check does not necessarily mean it's wrong at runtime. The former is probably easy to catch when it happens while the latter seems more robost but when the zero check is off, it's probably harder to detect/trace back.

Alternatively, to improve robustness, a dead ModI/ModL can kill dependent control akin to what Roland did for Type nodes with JDK-8349479.

Could be an option. We then should probably also extend it to Div nodes. Might be worth to investigate separately.

MBaesken · 2025-09-24T09:21:32Z

I'll test this in our CI to see if this fixes the linux aarch64 issues (observed when running Test java/foreign/TestUpcallStress.java ) .

Unfortunately we still see an assert in the test java/foreign/TestUpcallStress on Linux aarch64 .
But this time it is not the 'old' one but

# assert(oopDesc::is_oop(obj)) failed: not an oop: 0x0000000000000001

Maybe it is unrelated, not sure .

iwanowww · 2025-09-25T22:15:47Z

src/hotspot/share/opto/divnode.cpp

  if (t1 == Type::TOP) { return Type::TOP; }
  if (t2 == Type::TOP) { return Type::TOP; }

+  // Mod by zero?  Throw exception at runtime!


Thanks for the clarifications. I thought about it for some time, but as things work now, I don't see a better alternative except just ignoring 0 divisor case. So, please, proceed with the fix as it is now.

Alternatively, to improve robustness, a dead ModI/ModL can kill dependent control akin to what Roland did for Type nodes with JDK-8349479.

SirYwell · 2025-09-26T08:36:09Z

Unfortunately we still see an assert in the test java/foreign/TestUpcallStress on Linux aarch64 . But this time it is not the 'old' one but

# assert(oopDesc::is_oop(obj)) failed: not an oop: 0x0000000000000001

Maybe it is unrelated, not sure .

@MBaesken this looks rather unrelated, but hard to tell without more output.

@chhagedorn did your tests came back green?

chhagedorn · 2025-09-26T08:46:53Z

Testing looks good!

I also left a comment about ignoring the zero divisor case. It's an interesting thought to just ignore/remove it. Anyway, the current patch just fixes the current situation and does not make it worse. So, I agree with it but if you want to switch to the ignoring case, I'm also fine. In the latter case, I won't be able to review it anymore since I will be on vacation next week (assuming we also wait for Vladimir's additional input about it). But you would have my implicit approval :-)

MBaesken · 2025-09-29T07:18:36Z

@MBaesken this looks rather unrelated, but hard to tell without more output.

Should I open a new JBS issue for it? Probably it is something else and we cannot address it in this PR .

SirYwell · 2025-09-29T08:38:43Z

@MBaesken this looks rather unrelated, but hard to tell without more output.

Should I open a new JBS issue for it? Probably it is something else and we cannot address it in this PR .

Yes, please. I'll integrate this change later today if there is no objection.

MBaesken · 2025-09-29T10:25:41Z

Yes, please. I'll integrate this change later today if there is no objection.

There is already https://bugs.openjdk.org/browse/JDK-8360595 ; I added the info about our assert there .
(so far this existing JBS issue is about ShenandoahGC but we see it also with G1GC).

SirYwell · 2025-09-29T18:39:17Z

Thanks everyone for the reviews :)

/integrate

openjdk · 2025-09-29T18:40:43Z

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

6c8e384: 8356022: Migrate descriptor parsing from generics to BytecodeDescriptor
3d97e17: 8367318: Test vmTestbase/nsk/jdi/MethodEntryRequest/addClassFilter_rt/filter_rt001/TestDescription.java timed out after passing
aabf699: 8355339: Test java/io/File/GetCanonicalPath.java failed: The specified network name is no longer available
... and 160 more: https://git.openjdk.org/jdk/compare/94a301a70e19be284f406ebb6d8b94b6f96e1a24...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-09-29T18:40:51Z

@SirYwell Pushed as commit 59e76af.

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

SirYwell added 2 commits September 19, 2025 11:34

test

d143d8b

move up div by zero check

9ba78d4

openjdk bot changed the title ~~8367967~~ 8367967: C2: "fatal error: Not monotonic" with Mod nodes Sep 21, 2025

openjdk bot added the hotspot-compiler [email protected] label Sep 21, 2025

SirYwell marked this pull request as ready for review September 21, 2025 06:14

openjdk bot added the rfr Pull request is ready for review label Sep 21, 2025

chhagedorn approved these changes Sep 22, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 22, 2025

shipilev approved these changes Sep 22, 2025

View reviewed changes

benoitmaillard reviewed Sep 22, 2025

View reviewed changes

add a second @run

ade824e

openjdk bot removed the ready Pull request is ready to be integrated label Sep 22, 2025

chhagedorn reviewed Sep 23, 2025

View reviewed changes

move test

0193749

iwanowww reviewed Sep 23, 2025

View reviewed changes

iwanowww approved these changes Sep 25, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 25, 2025

benoitmaillard approved these changes Sep 26, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Sep 29, 2025

openjdk bot closed this Sep 29, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 29, 2025

SirYwell deleted the fix/mod-not-monotonic branch September 29, 2025 18:58

8367967: C2: "fatal error: Not monotonic" with Mod nodes #27408

8367967: C2: "fatal error: Not monotonic" with Mod nodes #27408

Uh oh!

Conversation

SirYwell commented Sep 21, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Contributors

Reviewing

Uh oh!

bridgekeeper bot commented Sep 21, 2025

Uh oh!

openjdk bot commented Sep 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SirYwell commented Sep 21, 2025

Uh oh!

openjdk bot commented Sep 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Sep 21, 2025

Uh oh!

mlbridge bot commented Sep 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

MBaesken commented Sep 22, 2025

Uh oh!

MBaesken commented Sep 22, 2025

Uh oh!

chhagedorn left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chhagedorn commented Sep 22, 2025

Uh oh!

shipilev left a comment

Choose a reason for hiding this comment

Uh oh!

benoitmaillard left a comment

Choose a reason for hiding this comment

Uh oh!

benoitmaillard Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

SirYwell commented Sep 22, 2025

Uh oh!

chhagedorn Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

SirYwell Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

iwanowww Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SirYwell Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

iwanowww Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

chhagedorn Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

MBaesken commented Sep 24, 2025

Uh oh!

iwanowww Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

SirYwell commented Sep 26, 2025

Uh oh!

chhagedorn commented Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MBaesken commented Sep 29, 2025

Uh oh!

SirYwell commented Sep 21, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Sep 21, 2025 •

edited

Loading

openjdk bot commented Sep 21, 2025 •

edited

Loading

mlbridge bot commented Sep 21, 2025 •

edited

Loading

chhagedorn left a comment •

edited

Loading

iwanowww Sep 23, 2025 •

edited

Loading

chhagedorn commented Sep 26, 2025 •

edited

Loading