RFR: 8281213: Unsafe uses of long and size_t in MemReporterBase::diff_in_current_scale [v3]
Evgeny Astigeevich
eastigeevich at openjdk.org
Mon Dec 19 23:15:51 UTC 2022
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
More information about the hotspot-runtime-dev
mailing list