RFR: 8237637: Remove dubious type conversions from oop

[Kim Barrett]

> On Jan 22, 2020, at 8:33 AM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
> 
> Hi all,
> 
> Please review this patch to remove some dubious type conversions from oop.
> 
> https://bugs.openjdk.java.net/browse/JDK-8237637
> 
> When running with fastdebug and CHECK_UNHANDLED_OOPS oop is not a simple oopDesc* but a class. This class has a number of type conversions with the comment:
> 
>  // Explict user conversions
> 
> However, these are not only *explicit* conversions. They can be invoked implicitly as well.
> 
> I propose that we get rid of most of these and only leave these two:
> operator void* () const
> operator oopDesc* () const
> 
> so that we get better type safety in the code.
> 
> I've split this up into multiple webrevs to make it easier to review:
> 
> https://cr.openjdk.java.net/~stefank/8237637/webrev.01/00.promotedObject/
> https://cr.openjdk.java.net/~stefank/8237637/webrev.01/01.oopStar/
> https://cr.openjdk.java.net/~stefank/8237637/webrev.01/02.jobject/
> https://cr.openjdk.java.net/~stefank/8237637/webrev.01/03.address/
> https://cr.openjdk.java.net/~stefank/8237637/webrev.01/03.address/
> https://cr.openjdk.java.net/~stefank/8237637/webrev.01/05.voidStarVolatiles/
> https://cr.openjdk.java.net/~stefank/8237637/webrev.01/06.heapWord/
> 
> All changes combined:
> https://cr.openjdk.java.net/~stefank/8237637/webrev.01/all/
> 
> Testing: builds pass, tier1-3 running
> 
> Thanks,
> StefanK

I like this.  A few minor cleanups below.  I have some additional
suggestions, but I think they are more suited to followup RFEs that
I'm planning to write up.

------------------------------------------------------------------------------
src/hotspot/share/compiler/oopMap.cpp
 795     *derived_loc = (oop)(cast_from_oop<address>(base) + offset);

Seems like that should also be using cast_to_oop.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1FullGCOopClosures.cpp
  80         HeapRegion* to   = _g1h->heap_region_containing(cast_from_oop<HeapWord*>(obj));

src/hotspot/share/gc/g1/g1ParScanThreadState.hpp
 132     HeapRegion* const hr_obj = _g1h->heap_region_containing(cast_from_oop<HeapWord*>(o));

src/hotspot/share/gc/g1/heapRegion.cpp
 569           HeapRegion* to = _g1h->heap_region_containing(cast_from_oop<HeapWord*>(obj));

No casts were needed in these places, so remove them rather than
converting them to cast_from_oop<HeapWord*>.

Similarly, there are some calls to operations on mark bit maps that
don't need casts because there are overloads on oop, so should have
the casts removed rather than being converted to cast_from_oop<HeapWord*>.

Similarly for is_in_cset.  And there might be other functions like
this that I didn't notice.

------------------------------------------------------------------------------
src/hotspot/share/gc/parallel/psCardTable.cpp
  53         _unmarked_addr = (HeapWord*)(void*)p;

The addition of the cast to void* seems unnecessary.

------------------------------------------------------------------------------
src/hotspot/share/oops/oopsHierarchy.hpp

When removing this line (as was mentioned in the thread)
 113   operator void* () const             { return (void *)obj(); }

I suggest also changing cast_from_oop to use oopDesc* rather than void*:
 175   return (T)(CHECK_UNHANDLED_OOPS_ONLY((oopDesc*))o);

Note that there are a lot of implicit conversions (mostly to void* I
think) that prevent us from removing (or making explicit with C++11)
the oopDesc* conversion operator.

------------------------------------------------------------------------------