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

[Doerr, Martin]

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;
+        }

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.

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

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.

Best regards,
Martin


From: Thomas Stüfe <thomas.stuefe at gmail.com>
Sent: Mittwoch, 30. Oktober 2019 11:48
To: hotspot compiler <hotspot-compiler-dev at openjdk.java.net>
Cc: Doerr, Martin <martin.doerr at sap.com>; Schmidt, Lutz <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