RFR: 8230203: Replace markWord enums with uintptr_t constants

[Stefan Karlsson]

Hi Kim,

I think your comments makes sense. I have an updated webrev that changes 
some of the types:

  https://cr.openjdk.java.net/~stefank/8230203/webrev.02.delta/
  https://cr.openjdk.java.net/~stefank/8230203/webrev.02/

The new patch compiles on Linux x64 and not yet tested. I'll run it 
through testing if you think this is the right change.

Thanks,
StefanK

On 2019-08-27 20:04, Kim Barrett wrote:
>> On Aug 27, 2019, at 3:32 AM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
>>
>> Hi all,
>>
>> Please review this patch to convert the enum constants in markWord into static const unintptr_t constants.
>>
>> http://cr.openjdk.java.net/~stefank/8230203/webrev.01/
>> https://bugs.openjdk.java.net/browse/JDK-8230203
>>
>> This is a follow-up RFE from the review of JDK-8229258, where a number of unnecessary casts were pointed out.
>>
>> There was also a recent bug in this area, JDK-8221725, where Thomas points out the problem of relying on the bit width of the enum values.
>>
>> This patch:
>> 1) changes all constants to have the same type as the markWord::_value field.
>> 2) Removes unnecessary uintptr_t casts
>> 3) Removes uintx casts, because they are typedefed to be uintptr_t
>> 4) Adds explicit casts to (int) when values are negated.
>> 5) Try to only use uintptr_t inside markWord.
>> 6) Leaves the use of intptr_t for parameters and return types of the different hash functions.
>>
>> Tested with tier1-3
>>
>> Thanks,
>> StefanK
>
> ------------------------------------------------------------------------------
> src/hotspot/share/oops/markWord.hpp
>   134   static const uintptr_t age_bits                 = 4;
> ...
>
> and
>   144   static const uintptr_t lock_shift               = 0;
> ...
>
> The bit counts and shift counts are used as rhs in shift expressions
> and such.  I think they should be int or uint rather than uintptr_t.
>
> ------------------------------------------------------------------------------
> src/hotspot/share/oops/markWord.hpp
>   334     assert((uintptr_t)age <= max_age, "age too large");
>
> I think the cast here is unnecessary and not really helpful.
>
> Unfortunately, the cast in the immediately following line:
>   335     assert((uintptr_t)bias_epoch <= max_bias_epoch, "bias epoch too large");
>
> probably is needed to avoid -Wsign-compare warnings.  (Though
> JDK-8230118 suggests we might not be getting -Wsign-compare warnings
> when one might think we ought.)
>
> ------------------------------------------------------------------------------
> src/hotspot/share/oops/markWord.hpp
>   409     assert((size & ~size_mask) == 0, "shouldn't overflow size field");
>   410     return markWord((cms_free_prototype().value() & ~size_mask_in_place) |
>   411                     ((size & size_mask) << size_shift));
>
> I wonder why 411 is applying size_mask after the 409 assert?
>
> ------------------------------------------------------------------------------
> I think there were 3 casts to int added:
>
> src/hotspot/share/opto/macro.cpp
> 2229                                       (~(int)markWord::age_mask_in_place), 0);
>
> src/hotspot/cpu/ppc/macroAssembler_ppc.cpp
> 3003   addi(current_header, current_header, -(int)markWord::monitor_value); // monitor
>
> src/hotspot/cpu/aarch64/aarch64.ad
> 3619     __ add(tmp, tmp, -(int)markWord::monitor_value); // monitor
>
> Consider preceeding these with STATIC_ASSERTs that the value being
> cast is <= INT_MAX.
>
> ------------------------------------------------------------------------------
>