RFR: 8237637: Remove dubious type conversions from oop

[Stefan Karlsson]

On 2020-01-23 08:06, Kim Barrett wrote:
>> 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.
> 

This patch is about fixing conversions *from* oop to other types. There 
are numerous places in the VM where we convert *to* oop with casts 
instead of cast_to_oop. I don't want to do those kind of changes in this 
RFE.

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

Changed.

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

Changed.

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

I can't find what you are referring to.

> 
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/parallel/psCardTable.cpp
>    53         _unmarked_addr = (HeapWord*)(void*)p;
> 
> The addition of the cast to void* seems unnecessary.
>

Reverted.

> ------------------------------------------------------------------------------
> src/hotspot/share/oops/oopsHierarchy.hpp
> 
> When removing this line (as was mentioned in the thread)
>   113   operator void* () const             { return (void *)obj(); }
> 

Done.

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

Done.

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

It's not obvious to me that we want to remove the implicit conversions 
to oopDesc*.

I'm going to run this (and David's suggestions) through tier1, and then 
I'll push the patch.

Thanks,
StefanK


> 
> ------------------------------------------------------------------------------
>