RFR: 8237637: Remove dubious type conversions from oop
Doerr, Martin
martin.doerr at sap.com
Thu Jan 23 14:40:23 UTC 2020
Hi Stefan,
thanks for adding my patch and for checking other platforms.
Best regards,
Martin
> -----Original Message-----
> From: Stefan Karlsson <stefan.karlsson at oracle.com>
> Sent: Donnerstag, 23. Januar 2020 14:33
> To: Doerr, Martin <martin.doerr at sap.com>; 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 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
> >>>>
More information about the hotspot-dev
mailing list