RFR: 8323688: C2: Fix UB of jlong overflow in PhaseIdealLoop::is_counted_loop()

[Tobias Hartmann]

On Tue, 3 Sep 2024 07:35:10 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> The computation of `final_correction` in `PhaseIdealLoop::is_counted_loop()` could overflow which is UB:
> 
> https://github.com/openjdk/jdk/blob/dc4fd896289db1d2f6f7bbf5795fec533448a48c/src/hotspot/share/opto/loopnode.cpp#L1958-L1967
> 
> `canonicalized_correction` equals `max_int - 1` if stride is `max_int`. `limit_correction` is at most `max_int - 1` in that case. Adding both together will overflow. I don't think that any compiler would wrongly optimize this and we have not observed any issues with that. But we should still fix this UB.
> 
> The fix I propose is to simply bail out with very large positive and negative strides such that we avoid an over- or underflow with the existing logic (see added comments for how the upper bound for the stride is determined). These large strides should be very uncommon in practice and even if we encounter these, the loop would only run for a few iterations. So, a bailout seems fine. This bailout has the additional benefit that we avoid other possibly unknown issues or issues in the future with counted loops having large edge-case strides like `min_int`.
> 
> Thanks,
> Christian

Looks good to me otherwise.

src/hotspot/share/opto/loopnode.cpp line 1974:

> 1972: 
> 1973:   // Check (vi) and bail out if the stride is too big.
> 1974:   if (stride_con == min_signed_integer(iv_bt) || ABS(stride_con) > max_signed_integer(iv_bt) / 2) {

Suggestion:

  if (stride_con == min_signed_integer(iv_bt) || (ABS(stride_con) > max_signed_integer(iv_bt) / 2)) {

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

Marked as reviewed by thartmann (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20828#pullrequestreview-2276828546
PR Review Comment: https://git.openjdk.org/jdk/pull/20828#discussion_r1741693684