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

[Andrew Hughes]

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.

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.

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.

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