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