RFR: 8237637: Remove dubious type conversions from oop

[Stefan Karlsson]

Hi Martin,

On 2020-01-23 12:57, Doerr, Martin wrote:
> Hi Stefan,
> 
> thanks a lot for cleaning this up.
> Could you add the following s390 part, please?
> Otherwise it would break the build. PPC64 seems to be fine.

Thanks for testing the patch. I compiled against aarch64 and Shenandoah 
and found more places that needed to be fixed. I've rolled up all 
changes into a new webrev:

https://cr.openjdk.java.net/~stefank/8237637/webrev.02.delta/
https://cr.openjdk.java.net/~stefank/8237637/webrev.02/

Thanks,
StefanK

> 
> Best regards,
> Martin
> 
> 
> diff -r 55fa1be40f39 src/hotspot/cpu/s390/assembler_s390.hpp
> --- a/src/hotspot/cpu/s390/assembler_s390.hpp   Thu Jan 23 12:44:02 2020 +0100
> +++ b/src/hotspot/cpu/s390/assembler_s390.hpp   Thu Jan 23 12:54:47 2020 +0100
> @@ -351,14 +351,6 @@
>       : _address((address) addr),
>         _rspec(rspec_from_rtype(rtype, (address) addr)) {}
> 
> -  AddressLiteral(oop addr, relocInfo::relocType rtype = relocInfo::none)
> -    : _address((address) addr),
> -      _rspec(rspec_from_rtype(rtype, (address) addr)) {}
> -
> -  AddressLiteral(oop* addr, relocInfo::relocType rtype = relocInfo::none)
> -    : _address((address) addr),
> -      _rspec(rspec_from_rtype(rtype, (address) addr)) {}
> -
>     AddressLiteral(float* addr, relocInfo::relocType rtype = relocInfo::none)
>       : _address((address) addr),
>         _rspec(rspec_from_rtype(rtype, (address) addr)) {}
> @@ -390,7 +382,6 @@
> 
>    public:
>     ExternalAddress(address target) : AddressLiteral(target, reloc_for_target(          target)) {}
> -  ExternalAddress(oop*    target) : AddressLiteral(target, reloc_for_target((address) target)) {}
>   };
> 
>   // Argument is an abstraction used to represent an outgoing actual
> 
> 
> 
>> -----Original Message-----
>> From: hotspot-dev <hotspot-dev-bounces at openjdk.java.net> On Behalf Of
>> Stefan Karlsson
>> Sent: Donnerstag, 23. Januar 2020 10:28
>> To: David Holmes <david.holmes at oracle.com>; hotspot-dev <hotspot-
>> dev at openjdk.java.net>
>> Subject: Re: RFR: 8237637: Remove dubious type conversions from oop
>>
>> Hi David,
>>
>> Thanks for looking at this. I've incorporated your suggestions.
>>
>> StefanK
>>
>> On 2020-01-23 00:56, David Holmes wrote:
>>> Hi Stefan,
>>>
>>> I don't have any concerns with this, but if cast_to_oop is for
>>> "numerical conversions" then can't we get rid of the secondary casts to
>>> jlong and u8 where used i.e can
>>>
>>>
>>> ---
>>>
>> old/src/hotspot/share/jfr/leakprofiler/checkpoint/objectSampleWriter.cpp
>>> 2020-01-22 14:24:21.194074776 +0100
>>> +++
>>>
>> new/src/hotspot/share/jfr/leakprofiler/checkpoint/objectSampleWriter.cpp
>>> 2020-01-22 14:24:20.714066939 +0100
>>> @@ -206,7 +206,7 @@
>>>      oop object = oosi->_data._object;
>>>      assert(object != NULL, "invariant");
>>>      writer->write(oosi->_id);
>>> -  writer->write((u8)(const HeapWord*)object);
>>> +  writer->write((u8)cast_from_oop<HeapWord*>(object));
>>>
>>> not use:
>>>
>>> writer->write(cast_from_oop<u8>(object));
>>>
>>> and:
>>>
>>> --- old/src/hotspot/share/jvmci/jvmciCompilerToVM.cpp    2020-01-22
>>> 14:24:22.674098943 +0100
>>> +++ new/src/hotspot/share/jvmci/jvmciCompilerToVM.cpp    2020-01-22
>>> 14:24:22.142090256 +0100
>>> @@ -431,7 +431,7 @@
>>>          } else if (JVMCIENV->isa_HotSpotObjectConstantImpl(base_object)) {
>>>            Handle base_oop = JVMCIENV->asConstant(base_object,
>>> JVMCI_CHECK_NULL);
>>>            if (base_oop->is_a(SystemDictionary::Class_klass())) {
>>> -          base_address = (jlong) (address) base_oop();
>>> +          base_address = (jlong) cast_from_oop<address>(base_oop());
>>>
>>> not use
>>>
>>>             base_address = cast_from_oop<jlong>(base_oop());
>>>
>>> ?
>>>
>>> Thanks,
>>> David
>>>
>>> On 22/01/2020 11:33 pm, Stefan Karlsson 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.promotedObje
>> ct/
>>>> 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.voidStarVolatil
>> es/
>>>>
>>>> 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
>>>>