RFR: 8281213: Unsafe uses of long and size_t in MemReporterBase::diff_in_current_scale [v3]

[Evgeny Astigeevich]

On Mon, 19 Dec 2022 18:37:35 GMT, Afshin Zafari <duke at openjdk.org> wrote:

>> src/hotspot/share/services/memReporter.hpp line 82:
>> 
>>> 80:     assert(amount <= SIZE_MAX - _scale / 2, "size_t overflow");
>>> 81:     amount = (amount + _scale / 2) / _scale;
>>> 82:     // We assume the valid range for deltas [INT64_MIN, INT64_MAX] to simplify the code.
>> 
>> In the assert the comment should be:
>> 
>> We assume the valid range for deltas [-INT64_MAX, INT64_MAX] to simplify the code.
>> 
>> 
>> I excluded the `INT64_MIN` case to simplify the code.
>> The `INT64_MIN` case is when `sizeof(size_t) == sizeof(int64_t) && amount == INT64_MAX + 1 && is_negative == true`.
>> For this case `-(int64_t)amount` is UB which needs proper handling.
>> The code instead of
>> 
>> return (is_negative) ? -(int64_t)amount : (int64_t)amount;
>> 
>> would be more complex:
>> 
>> if (!is_negative) {
>>   return (int64_t)amount;
>> } else {
>>   return (amount != INT64_MAX+1) ? -(int64_t)amount : INT64_MIN;
>> }
>> 
>> 
>> This is why the assert part is `sizeof(size_t) == sizeof(int64_t) && amount <= INT64_MAX)`
>> 
>> Maybe the better comment is
>> 
>> // We assume the valid range for deltas [-INT64_MAX, INT64_MAX].
>> // We excluded the `INT64_MIN` case to simplify the code.
>
> I couldn't follow your idea.
> The `INT64_MAX + 1` results in compiler overflow error.  Isn't it equal to `INT64_MIN`?

I did not know there was a defect report [#1313](https://cplusplus.github.io/CWG/issues/1313.html) fixed in C++11.
Now the standard prohibits to use constant expressions like `INT64_MAX + 1` in code.

> I couldn't follow your idea.

The idea is to show that if you want to handle -(2^63), you need more code. Your comment is not correct. The code does not handle INT64_MIN.

If we limit cases to [-(2^63 - 1), 2^63 - 1] the code would be simpler. IMHO one byte does not worth to increase the code complexity. In most cases -(2^63) would be a result of some wrong calculations.

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

PR: https://git.openjdk.org/jdk/pull/11514