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

[Thomas Stüfe]

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> 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>
> *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
>