RFR: 8313882: Fix -Wconversion warnings in runtime code [v2]

[Coleen Phillimore]

On Tue, 8 Aug 2023 21:09:30 GMT, Dean Long <dlong at openjdk.org> wrote:

>> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Make bytecode.hpp index() functions return u2 not long.
>
> src/hotspot/share/runtime/arguments.cpp line 1667:
> 
>> 1665: 
>> 1666:     if (InitialHeapSize == 0) {
>> 1667:       julong reasonable_initial = (julong)((phys_mem * (julong)InitialRAMPercentage) / 100);
> 
> This and the two changes above loses the fractional part.
> Suggestion:
> 
>       julong reasonable_initial = (julong)((double)phys_mem * InitialRAMPercentage / 100.0);

Thanks, yes. That's wrong.

> src/hotspot/share/runtime/arguments.cpp line 2738:
> 
>> 2736:       }
>> 2737: 
>> 2738:       if (FLAG_SET_CMDLINE(MaxTenuringThreshold, (uint)max_tenuring_thresh) != JVMFlag::SUCCESS) {
> 
> MaxTenuringThreshold was recently changed to uint, so it would make sense change uintx above to uint.

I couldn't change that one because there isn't a parse_uint() version of this:
      if (!parse_uintx(tail, &max_tenuring_thresh, 0)) {
Which calls parse_integer into a 64 bit value.

> src/hotspot/share/runtime/deoptimization.cpp line 255:
> 
>> 253:     result += frame_sizes()[index];
>> 254:   }
>> 255:   return checked_cast<int>(result);
> 
> Isn't _caller_adjustment already an int?

frame_sizes() returns intptr_t so I had to promote result, then demote it.  frame_sizes() is a more difficult change.

runtime/deoptimization.hpp:    intptr_t* _frame_sizes;               // Array of frame sizes, in bytes, for unrolling the stack

> src/hotspot/share/runtime/java.hpp line 108:
> 
>> 106: 
>> 107:   // Factory methods for convenience
>> 108:   static JDK_Version jdk(int m) {
> 
> Finally, support for JDK 256 and beyond!

Hooray!

> src/hotspot/share/runtime/objectMonitor.hpp line 238:
> 
>> 236:   //
>> 237:   #define OM_OFFSET_NO_MONITOR_VALUE_TAG(f) \
>> 238:     ((in_bytes(ObjectMonitor::f ## _offset())) - (unsigned)markWord::monitor_value)
> 
> This makes it int vs uint (-Wsign-conversion warning), and assumes the range of monitor_value.
> One option is to narrow the type of monitor_value.  Option 2, cast the lhs to uintptr_t.  Option 3, use offset_of instead of signed ByteSize.

I looked at option 1, but monitor_value is used for byte operations, and it seemed unsafe to make it smaller than uintptr_t, eg:

oops/markWord.hpp:    return markWord(tmp | monitor_value);

The -Wconversion warning is complaining about OM_OFFSET_NO_MONITOR_VALUE_TAG is an long unsigned int because the expression is promoted to the size of markWord::monitor_value without the cast.

Option 3 turns the address into a 64 bit int, but the Address constructor expects a 32 bit displacement.

   88 |   __ movptr(Address(mon, OM_OFFSET_NO_MONITOR_VALUE_TAG(owner)), r15_thread);

Should this convert to int instead of unsigned instead?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15177#discussion_r1287791980
PR Review Comment: https://git.openjdk.org/jdk/pull/15177#discussion_r1287793204
PR Review Comment: https://git.openjdk.org/jdk/pull/15177#discussion_r1287794097
PR Review Comment: https://git.openjdk.org/jdk/pull/15177#discussion_r1287794217
PR Review Comment: https://git.openjdk.org/jdk/pull/15177#discussion_r1287800508