RFR: 8229258: Make markOopDesc AllStatic

[Stefan Karlsson]

On 2019-08-07 23:33, coleen.phillimore at oracle.com wrote:
> 
> I clicked through the cpu, oops, prims, runtime and sa directories. All 
> changes as expected.  Went through synchronizer.cpp too fast though, 
> somebody else should read that more carefully.
> 
> https://cr.openjdk.java.net/~stefank/8229258/webrev.01/src/hotspot/share/oops/oop.inline.hpp.udiff.html 
> 
> 
> -markOop oopDesc::mark() const {
> +markWord oopDesc::mark() const {
>     return HeapAccess<MO_VOLATILE>::load_at(as_oop(), 
> mark_offset_in_bytes());
>   }
> 
> -markOop oopDesc::mark_raw() const {
> +markWord oopDesc::mark_raw() const {
>     return _mark;
>   }
> 
> Nit, can you take out these blanks before 'const' while you're editing 
> these lines.  They don't line up with anything.

Done.

> 
> Removing the commented out SA code makes sense to me.  There are two 
> test directories for the SA.  One is here: open/test/jdk/sun/tools/jhsdb

Thanks. I tested both.

> 
> This looks great!  Thank you!

Thanks for reviewing!

StefanK

> Coleen
> 
> On 8/7/19 3:58 PM, Stefan Karlsson wrote:
>> 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
>>
>