PING: RE: RFR: 8233019: java.lang.Class.isPrimitive() (C1) returns wrong result if Klass* is aligned to 32bit

Wed Jul 7 08:19:56 UTC 2021

Hi,

Let me revive this.

Any comments please?

I have also prepared a 8u backport of 8150669:
    http://cr.openjdk.java.net/~fyang/8150669-8u/webrev.00/

Thanks,
Felix

> 
> Hi Andrew,
> 
> Thanks for the reply :-)
> 
> > -----Original Message-----
> > From: Andrew Hughes [mailto:gnu.andrew at redhat.com]
> > Sent: Tuesday, May 18, 2021 10:07 AM
> > To: Yangfei (Felix) <felix.yang at huawei.com>
> > Cc: Aleksey Shipilev <shade at redhat.com>; jdk8u-dev <jdk8u-
> > dev at openjdk.java.net>
> > Subject: Re: PING: RE: RFR: 8233019: java.lang.Class.isPrimitive()
> > (C1) returns wrong result if Klass* is aligned to 32bit
> >
> > On 01:15 Tue 18 May     , Yangfei (Felix) wrote:
> > > Gentle ping ...
> > >
> > > Any comments from our maintainers?
> > >
> > > Thanks,
> > > Felix
> > >
> > > > -----Original Message-----
> > > > From: Yangfei (Felix)
> > > > Sent: Tuesday, April 27, 2021 8:18 PM
> > > > To: 'Aleksey Shipilev' <shade at redhat.com>; jdk8u-dev <jdk8u-
> > > > dev at openjdk.java.net>
> > > > Subject: RE: RFR: 8233019: java.lang.Class.isPrimitive() (C1)
> > > > returns wrong result if Klass* is aligned to 32bit
> > > >
> > > > Hi,
> > > >
> > > > Thanks for the quick reply :-)
> > > >
> > > > > -----Original Message-----
> > > > > From: Aleksey Shipilev [mailto:shade at redhat.com]
> > > > > Sent: Monday, April 26, 2021 9:00 PM
> > > > > To: Yangfei (Felix) <felix.yang at huawei.com>; jdk8u-dev <jdk8u-
> > > > > dev at openjdk.java.net>
> > > > > Subject: Re: RFR: 8233019: java.lang.Class.isPrimitive() (C1)
> > > > > returns wrong result if Klass* is aligned to 32bit
> > > > >
> > > > > On 4/26/21 2:52 PM, Yangfei (Felix) wrote:
> > > > > > I find issue JDK-8239477 [1] is triggering for 16 jfr jtreg
> > > > > > tests with fastdebug
> > > > > build.
> > > > > > Fix for this issue depends on JDK-8233019 as it emits a
> > > > > > compare with
> > > > > metadataConst(0).
> > > > > > So need to backport JDK-8233019 first.
> > > > > >
> > > > > > Original bug:
> > > > > >      https://bugs.openjdk.java.net/browse/JDK-8233019
> > > > > >
> > > > > > https://hg.openjdk.java.net/jdk-updates/jdk11u/rev/e1b6631cbd2
> > > > > > f
> > > > > >
> > > > > > Original patch does not apply to 8u cleanly. Two adaptations
> > > > > > are made for
> > > > > 8u:
> > > > > > 1. Discarded changes in file c1_LIRGenerator.cpp as
> > > > > > JDK-8150669 [2] is not
> > > > > there in 8u.
> > > > > > 2. Added new test
> > > > > hotspot/test/compiler/intrinsics/class/TestClassIsPrimitive.java
> > > > > which was introduced by [2] and further modified by this issue.
> > > > >
> > > > > Honestly, I would consider backporting JDK-8150669 first. It
> > > > > seems quite
> > > > small:
> > > > >
> > > > > https://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/d15b795cdf21
> > > > >
> > > > > Otherwise, we are splicing the fix under the unrelated synopsis...
> > > > >
> > > > > Let 8u maintainers decide.
> > > >
> > > > Yes, I agree it will be more cleaner if we backport JDK-8150669 first.
> > > > And I am happy to propose another 8u webrev for JDK-8150669 if
> > > > it's OK for 8u maintainers.
> > > >
> > > > Felix
> >
> > Sorry, only just seen this.
> >
> > My understanding of these bugs is as follows:
> >
> > 1. JDK-8150669: "C1 intrinsic for Class.isPrimitive" is an enhancement
> > which improves the performance of Class.isPrimitive
> >
> > 2. JDK-8233019: "java.lang.Class.isPrimitive() (C1) returns wrong
> > result if
> > Klass* is aligned to 32bit" fixes a bug in #1.
> > In the process, it introduces handling of T_METADATA in C1.
> >
> > 3. JDK-8239477: "jdk/jfr/jcmd/TestJcmdStartStopDefault.java fails -
> > XX:+VerifyOops with "verify_oop: rsi: broken oop" is, AFAICS,
> > completely unrelated to Class.isPrimitive, but the fix relies on the
> > T_METADATA change that was bundled with #2.
> 
> Yes, that's correct.
> 
> > I'm confused as to why your original backport [0] added a test for the
> > Class.isPrimitive intrinsic, when the intrinsic is not been
> > backported. That makes no sense to me.
> 
> That's because [0] backports JDK-8233019 and JDK-8233019 adds extra
> checks to the test.
> So I simply added the whole test case for backport [0] for test coverage.
> 
> > I think you can go in one of two directions here; either backport all
> > three pretty much as is, or just backport JDK-8239477, and include the
> > T_METADATA additions. The latter is no different to what was done in
> > #2, bundling the additions in with the fix that needed to use them.
> 
> Personally, I would like to backport all three so that the three backports will
> be cleaner.
> But let's see what Aleksey would say about the risk of this way.
> 
> > My initial feeling is just to backport JDK-8239477 with the necessary
> > extra code. Adding an performance enhancement just so you can backport
> > a fix for said enhancement which brings in some other unrelated code
> > seems to be introducing unnecessary risk here.
> >
> > However, it seems Aleksey is the original author of JDK-8150669, so
> > I'd appreciate his input on how risky/worthwhile he feels it is to
> > backport.  If
> > JDK-8150669 has merit to being backported in isolation, then we can go
> > with all three.  Otherwise, I would just extend a
> > JDK-8239477 backport with the additional T_METADATA code it requires,
> > as
> > JDK-8150669 shouldn't be backported just for the sake of 8239477.
> >
> > [0] https://cr.openjdk.java.net/~fyang/8233019-
> > 8u/webrev.00/hotspot.changeset
> > --
> > Andrew :)
> >
> > Senior Free Java Software Engineer
> > OpenJDK Package Owner
> > Red Hat, Inc. (http://www.redhat.com)
> >
> > PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
> Fingerprint
> > = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222