RFR: 8338194: ubsan: mulnode.cpp:862:59: runtime error: shift exponent 64 is too large for 64-bit type 'long unsigned int'

Marc Chevalier mchevalier at openjdk.org
Thu Apr 24 08:25:56 UTC 2025


We have a UB when the shift is equal to or bigger than the number of bits in
the type. Our expression is

(julong)CONST64(1) << (julong)(BitsPerJavaLong - shift)

so we have a UB when the RHS is `>= 64`, that is when `shift` is `<= 0`. Since shift is masked to
be in `[0, BitPerJavaLong - 1]`, we actually have a UB when `shift == 0`. The
code doesn't forbid it, indeed, and it doesn't seem to be enforced by more global
invariants.

This UB doesn't reproduce anymore with the provided cases.
I've replaced the UB with an explicit assert to try to find another failing
case. No hit when run with tier1, tier2, tier3, hs-precheckin-comp and hs-comp-stress.

Nevertheless, the assert indeed hit on the master of when the issue was filed.
More precisely, I've bisect for the two tests `java/foreign/StdLibTest.java`
and `java/lang/invoke/PermuteArgsTest.java` and the assert hits until
[8339196: Optimize BufWriterImpl#writeU1/U2/Int/Long](https://bugs.openjdk.org/browse/JDK-8339196).

It is not clear to me why the issue stopped reproducing after this commit, but given
the lack of reproducer, I went with a semi-blind fix: it fixes the issue back then,
and still removes a chance of UB. It simply makes sure the RHS of this shift cannot be
64 by making sure `shift` cannot be 0.

If `shift` is indeed 0, since it is the RHS of a `RShiftLNode`, `RShiftLNode::Identity`
should simply returns the LHS of the shift, and entirely eliminate the RShiftLNode.

The implementation of `AndINode::Ideal` is, on the other hand, safe. Indeed, it uses
`right_n_bits(BitsPerJavaInteger - shift)` instead of doing manually
`(1 << (BitsPerJavaInteger - shift)) - 1`. This macro is safe as it would return `-1` if
the shift is too large rather than causing a UB. Yet, I didn't use this way since it would
cause the replacement of `AndI(X, RShiftI(Y, 0))` by `AndI(X, URShiftI(Y, 0))` before
simplifying the `URShiftI` into `Y`. In between, it also implies that all users of the
And node will be enqueued for IGVN for a not-very-interesting change. Simply skipping
the replacement of RShiftL into URShiftL allows to directly come to `AndL(X, Y)` without
useless steps.

Thanks,
Marc

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

Commit messages:
 - Avoiding the UB

Changes: https://git.openjdk.org/jdk/pull/24841/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=24841&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8338194
  Stats: 9 lines in 1 file changed: 2 ins; 0 del; 7 mod
  Patch: https://git.openjdk.org/jdk/pull/24841.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/24841/head:pull/24841

PR: https://git.openjdk.org/jdk/pull/24841


More information about the hotspot-compiler-dev mailing list