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

[Thomas Stuefe]

On Fri, 13 Jan 2023 08:15:14 GMT, Afshin Zafari <duke at openjdk.org> wrote:

>> src/hotspot/share/services/memReporter.hpp line 74:
>> 
>>> 72: 
>>> 73:   // Convert diff amount in bytes to current reporting scale
>>> 74:   int64_t diff_in_current_scale(size_t s1, size_t s2) const {
>> 
>> - Can you add a little comment please about why you use int64_t? E.g. something like:
>> 
>> // We use int64_t instead of ssize_t because on 32-bit it allows us to express deltas larger than 2 gb. On 64-bit we
>> // never expect memory sizes larger than INT64_MAX".
>> 
>> - I would assert the input values before doing the subtraction, then don't need the is_negative part. You could shrink the function to something like this:
>>  ```
>>   int64_t diff_in_current_scale(size_t s1, size_t s2) const {
>>     LP64_ONLY(assert(s1 < INT64_MAX && s2 < INT64_MAX, "size overflow");)
>>     return ((int64_t)s1 - (int64_t)s2)/scale;
>>  }
>> 
>> The LP64_ONLY could be needed because on 32-bit the compiler may complain about the compare being always true.
>> 
>> Am I overlooking something?
>
> The difference should be round up/down to an exact number, i.e.  +/- scale/2.
> s1 and s2 can be both very large, even larger than INT64_MAX, but the difference and round off is our concern here in this function. As an 8bit example: `s1 = 200` and `s2 = 220`  both larger than 127 = INT8_MAX.

They cannot be larger than INT64_MAX. These are counters for memory allocated, and no hardware today allows for >8 exabytes to be allocated. So, seeing a large value like that is likely a bug, e.g. a negative overflow. It would be good to assert that. If both values are large, the delta is small, and the error would be hidden.

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

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