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

Tue May 18 11:27:49 UTC 2021

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