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

[dean.long at oracle.com]

The shared changes look good to me, and I don't see any obvious problems 
with the cpu changes.

dl

On 10/31/19 12:01 AM, Thomas Stüfe wrote:
> 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
>>