RFR: 8352422: [ubsan] Out-of-range reported in ciMethod.cpp:917:20: runtime error: 2.68435e+09 is outside the range of representable values of type 'int'

[Dean Long]

On Wed, 23 Apr 2025 10:58:54 GMT, Marc Chevalier <mchevalier at openjdk.org> wrote:

> The double `(double)count * prof_factor * method_life / counter_life + 0.5`
> can overflow a 32-bit int, causing UB on casting, but in practice computing
> a wrong scale, probably.
> 
> We just need to compare that the cast is not going to overflow. This is possible
> because `INT_MAX` is exactly representable in a `double`. It is also good to
> notice that the expression `(double)count * prof_factor * method_life / counter_life + 0.5`
> cannot overflow a `double`:
> - `count` is a int, max value = 2^31-1 < 2.2e9
> - `method_lie` is a int, max value < 2.2e9
> - `prof_factor` is a float, max value < 3.5e38
> - `counter_life` is a int, positive at this point, so min value = 1
> So, the whole expression is bounded by 16.94e56 + 0.5, which is much smaller than the
> max value of a double (about 1.8e308). We probably would have precision issues, but
> it probably doesn't matter a lot.
> 
> The semantic I picked here is basically `min(INT_MAX, count_d)`, so it'd always fit.
> 
> Thanks,
> Marc

src/hotspot/share/ci/ciMethod.cpp line 919:

> 917:       double count_d = (double)count * prof_factor * method_life / counter_life + 0.5;
> 918:       if (count_d >= static_cast<double>(INT_MAX)) {
> 919:         count = INT_MAX;

INT_MAX is probably the best choice, but could cause a change in behavior if the compiler previously returned a negative number here on overflow.  It's not clear to me if we want to preserve existing behavior or not.  In various places we use different limits for saturated counters and may use a negative number to represent overflow.  I don't know if this is by design or accident, but it can happen when, for example, the taken() or not_taken() value of a BranchData overflows and gets clamped to (uint)-1.  In Parse::dynamic_branch_prediction() we assign those uint values to int, resulting in a negative number, which will be rejected by counters_are_meaningful().
By clamping an overflow here to INT_MAX instead of a UB negative number, we could allow taken == INT_MAX, not_taken == 0 to sneak past, which might be harmless.  It's not clear to me the best way to handle saturated values, but you might find it useful looking at the history in JDK-8306331 and JDK-8306481.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24824#discussion_r2057258013