RFR: 8230203: Replace markWord enums with uintptr_t constants

[David Holmes]

Hi Stefan,

On 28/08/2019 6:06 am, Stefan Karlsson wrote:
> 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/

I agree that is much clearer now. "bits" and "shifts" are just numbers. 
masks and comparison-constants should be the same type as the 
variable(s) they will be applied to.

In markWord.hpp:

334     assert(age <= max_age, "age too large");

This assert is a tautology as age and max_age are the same uint type. (I 
expect some compilers may point this out.)

Thanks,
David
-----

> 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.
>>
>> ------------------------------------------------------------------------------ 
>>
>>
>