RFR: 8237637: Remove dubious type conversions from oop

[Doerr, Martin]

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.

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