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

[Evgeny Astigeevich]

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

>> ### Description
>> MemReporterBase::diff_in_current_scale is defined as follows:
>> 
>>   inline long diff_in_current_scale(size_t s1, size_t s2) const {
>>     long amount = (long)(s1 - s2);
>>     long scale = (long)_scale;
>>     amount = (amount > 0) ? (amount + scale / 2) : (amount - scale / 2);
>>     return amount / scale;
>>   }
>> 
>> Long and size_t can have different sizes: 4 bytes and 8 bytes (LLP64). The result of 's1 - s2' might not fit into long. It might not fit into int64_t. For example: s1 is SIZE_MAX and s2 is SIZE_MAX-MAX_INT64-1.
>> 
>> Size_t should be used instead of long. Assertions must be added to check:
>> s1 >= s2 and (amount - scale/2) >= 0 and (amount + scale/2) <= SIZE_MAX.
>> 
>> ### Patch
>> `long` is replaced by `size_t`. Comparison to 0 is implemented accordingly since size_t is always >= 0.
>> Since s1 can be less than s2 in some invocations of this method, no assert is written for `(s1 >= s2)` case.
>> 
>> ### Test
>> local: runtime/NMT/Jcmd*
>> mach5: tier1
>
> Afshin Zafari has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8281213: Unsafe uses of long and size_t in MemReporterBase::diff_in_current_scale

LGTM, just minor suggestions.

src/hotspot/share/services/memReporter.hpp line 80:

> 78:     assert(_scale != 0, "wrong scale");
> 79: 
> 80:     LP64_ONLY(assert(s1 < INT64_MAX && s2 < INT64_MAX, "size overflow");)

"size overflow" is not clear. I suggest "exceeded possible memory allocation limits".
I also suggest to separate the assert into two asserts: one for `s1`, another for `s2`. Otherwise it would be difficult to find out which of them has triggered the assert.

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

Changes requested by eastigeevich (Committer).

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