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

[Doerr, Martin]

Hi Thomas,

looks good.

Thanks,
Martin


From: Thomas Stüfe <thomas.stuefe at gmail.com>
Sent: Donnerstag, 31. Oktober 2019 08:02
To: Doerr, Martin <martin.doerr at sap.com>
Cc: hotspot compiler <hotspot-compiler-dev at openjdk.java.net>; Schmidt, Lutz <lutz.schmidt at sap.com>
Subject: Re: RFR(xs): 8233019: java.lang.Class.isPrimitive() (C1) returns wrong result if Klass* is aligned to 32bit

Hi Martin,

thanks for the review!

New version: http://cr.openjdk.java.net/~stuefe/webrevs/8233019--c1-intrinsic-for-java.lang.class.isprimitive()-does-32bit-compare/webrev.01/webrev/

pls find remarks inline.

On Wed, Oct 30, 2019 at 1:09 PM Doerr, Martin <martin.doerr at sap.com<mailto:martin.doerr at sap.com>> wrote:
Hi Thomas,

thank you for finding and fixing this issue.

Shared code part and regression test look good to me.
But I have a few requests and questions related to the platform code.

Arm32:
Breaks build. The local variable p needs a scope. Can be fixed by:
-        case T_METADATA:
+        case T_METADATA: {
           // We only need, for now, comparison with NULL for metadata.
…
           break;
+        }

Ouch. Fixed. Actually, I discarded my coding completely and took over the code from T_OBJECT to keep in line with the rest of the coding here.


S390:
Using z_cgfi is correct, but there are equivalent instructions with shorter opcode.
For comparing to 0, z_ltgr(reg1, reg1) or z_cghi(reg1, 0) may be preferred. But that’s not a big deal.


Okay I switched to cghi. ltgr sounds cool but would be difficult to integrate into the shared part since that one does first a move, then a compare.

I wonder why you have added includes to some platform files. Isn’t that redundant?
"utilities/debug.hpp" comes via shared assembler.hpp.

Did this because I added asserts and the rule is that every file should include what it needs and not rely on other includes including it (save for umbrella includes like globalDefinitions.hpp). But okay, I removed the added includes again to keep the patch small.


I’d probably choose Unimplemented() instead of ShouldNotReachHere() for non-null cases because it’s not bad in general, it’s just currently not used and therefore not yet implemented.
But you can keep that as it is. I’m ok with that, too.

I rather keep it to keep in line with the rest of the code (see e.g. the default: branches).


Best regards,
Martin


Thanks Martin!

..Thomas


From: Thomas Stüfe <thomas.stuefe at gmail.com<mailto:thomas.stuefe at gmail.com>>
Sent: Mittwoch, 30. Oktober 2019 11:48
To: hotspot compiler <hotspot-compiler-dev at openjdk.java.net<mailto:hotspot-compiler-dev at openjdk.java.net>>
Cc: Doerr, Martin <martin.doerr at sap.com<mailto:martin.doerr at sap.com>>; Schmidt, Lutz <lutz.schmidt at sap.com<mailto:lutz.schmidt at sap.com>>
Subject: RFR(xs): 8233019: java.lang.Class.isPrimitive() (C1) returns wrong result if Klass* is aligned to 32bit

Hi all,

second attempt at a fix (please find first review thread here: https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2019-October/035608.html)

Issue: https://bugs.openjdk.java.net/browse/JDK-8233019
webrev: http://cr.openjdk.java.net/~stuefe/webrevs/8233019--c1-intrinsic-for-java.lang.class.isprimitive()-does-32bit-compare/webrev.00/webrev/

In short, C1 intrinsic for jlC::isPrimitive does a compare with the Klass* pointer for the class to find out if its NULL and hence a primitive type. That compare is done using 32bit cmp and so it gives wrong results when the Klass* pointer is aligned to 32bit.

In the generator I changed the comparison constant type from intConst(0) to metadataConst(0) and implemented the missing code paths for all CPUs. Since on most architectures we do not seem to have a comparison with a 64bit immediate (at least I could not find one) I kept the change simple and only implemented comparison with NULL for now.

I tested the fix in our nightlies (jtreg tier1, jck and others) as well as manually testing it.

I did not test on aarch64 and arm though and would be thankful if someone knowledgeable to these platforms could take a look.

Thanks to Martin and Lutz for eyeballing the ppc and s390 parts.

Thanks, Thomas