RFR: 8229258: Make markOopDesc AllStatic

[Stefan Karlsson]

On 2019-08-07 23:59, Roman Kennke wrote:
> Oh yeah! My IDE completed some operation on markOopDesc to something
> that is actually in oopDesc (and would map to something else in
> markOopDesc) more than once and left me wondering. E.g.
> oopDesc::is_forwarded() which would be markOopDesc::is_marked(). Calling
> mark->is_forwarded() seems the obvious thing, but is totally foobared.
> 
> So yeah, this is *very* welcome. I guess the changes are alright and
> mostly mechanical, but I'll take a deeper look tomorrow, when I'm not
> tired ;-)

:) Thanks for taking a look at this. I'll await your review.

Cheers,
StefanK

> 
> Thanks & cheers,
> Roman
> 
>> Hi all,
>>
>> Please review this patch to make markOopDesc AllStatic.
>>
>> https://cr.openjdk.java.net/~stefank/8229258/webrev.01/
>> https://bugs.openjdk.java.net/browse/JDK-8229258
>>
>>  From the RFE:
>>
>> I want to change/rename markOop and markOopDesc
>>
>> Today markOopDescs inherits from oopDesc even though they are not
>> oopDescs (Java objects):
>>   `class markOopDesc : public oopDesc {`
>>
>> This is very confusing to new-comers (and probably to old-timers as well).
>>
>> A simple fix would be to break the inheritance and do the following rename:
>> markOopDesc -> MarkWord
>> markOop -> markWord
>>
>> However, there are more dubious code in this area. markOopDescs are
>> created and used like this:
>>
>> ```
>> class oopDesc {
>> ...
>>    volatile markOop _mark;
>> ...
>> markOop  oopDesc::mark_raw()  const { return _mark; }
>> ...
>> // Usage
>> obj->mark_raw()->is_marked()
>> ...
>> // Implementation
>>    bool is_marked()   const {
>>      return (mask_bits(value(), lock_mask_in_place) == marked_value);
>>    }
>> ...
>> // Implementation
>>    bool is_being_inflated() const { return (value() == 0); }
>> ...
>> // Implementation of markOopDesc::value()
>>    uintptr_t value() const { return (uintptr_t) this; }
>> ```
>>
>> Remember, _mark is an arbitrary bit pattern describing the object. We
>> treat it as if it were a pointer to a markOopDesc object, but it's not
>> pointing to such an object. While using that (weird) pointer we call
>> value() and extract it's bit pattern to be used for further bit pattern
>> checking functions. AFAICT, this is also undefined behavior. At least
>> is_being_inflated() const is effectively 'return (uintptr_t) this ==
>> NULL'. This UB was recently discussed here:
>>   https://mail.openjdk.java.net/pipermail/hotspot-dev/2019-July/038704.html
>>   https://mail.openjdk.java.net/pipermail/hotspot-dev/2019-July/038712.html
>>
>> I propose that we change MarkWord (markOopDesc) to be an AllStatic class
>> instead and get rid of this (ab)use of the markWord (markOop) pointer.
>>
>> The patch is large but mostly straight forward. However, there is some
>> Java code that would need some extra scrutiny: SA and JVMCI.
>>
>> Tested with tier1-3.
>>
>> Coleen has asked for file rename from markOop.hpp to markWord.hpp. I'd
>> like to do that as a separate RFE when/if this patch gets accepted.
>>
>> Thanks,
>> StefanK
>>
>