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'

[Marc Chevalier]

On Thu, 24 Apr 2025 02:21:15 GMT, Dean Long <dlong 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.

There was still

count = (count > 0) ? count : 1;

so actually, values that became negative by overflow are actually pushed up to 1 (which seems to be a legitimate value to me): it was already not possible to distinguish overflows from good results.

But even if I return a (new) special value for overflows, there is the question of what to do with that from the caller perspective. In my understanding, the result is used for profiling purpose: to decide how often is a branch taken, and thus, whether it's worth [doing something]. If a branch is so much taken it causes an overflow here, I think the reasonable thing to do is to make it succeed all the comparisons to know if it's taken often enough. If they are all of the form `scale > threshold`, then returning `INT_MAX` will guarantee that we satisfy all such tests. Maybe another way would be to indeed return a special value that would be handled as "result was very high: between INT_MAX and infinity" and handle it as such a value would be, but I suspect it's how `INT_MAX` is handled.

Also, I don't think having an overflow here signifies a programming error: we have a product of multiple `int` and a `float`. It's easy to overflow an int: `46341 * 46341` is already more than `INT_MAX`.

Indeed, it can change the behavior in some overflow cases, but then I'd argue that the former behavior was basically random, so it's probably not worth trying to preserve.

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

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