RFR: 8229258: Make markOopDesc AllStatic

[coleen.phillimore at oracle.com]

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.

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

This looks great!  Thank you!
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
>