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

[Marc Chevalier]

On Thu, 24 Apr 2025 06:55:54 GMT, Marc Chevalier <mchevalier at openjdk.org> wrote:

> 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

Thanks @dean-long and @dafedafe for your reviews!

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

PR Comment: https://git.openjdk.org/jdk/pull/24841#issuecomment-2835208391