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

[dean.long at oracle.com]

Hi Thomas.  Does it work if you compare against 
LIR_OprFact::addressConst(0)?

dl

On 10/25/19 9:32 AM, Thomas Stüfe wrote:
> Just occurred to me that my proposed fix could break non-x86. I have to
> think this through a bit more. I'd still be thankful for feedback.
>
> Cheers, Thomas
>
> On Fri, Oct 25, 2019, 16:57 Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
>
>> Hello,
>>
>> Could I please have opinions and reviews for the following fix.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8233019
>> webrev:
>> http://cr.openjdk.java.net/~stuefe/webrevs/8233019-c1-isprimitive/webrev.00/webrev/
>>
>>
>> While working with the prototype of the new metaspace allocator
>> (JDK-8221173), I saw weird errors which looked like
>> java.lang.Class.isPrimitive() returning sometimes incorrectly true for
>> boolean[].class, which is not a primitive.
>>
>> The bug seems to only happen in c1.
>>
>> boolean[].class is the very first Klass* ever to be created in the life of
>> a VM. For JDK-8221173 I removed metachunk headers from their payload. One
>> unintended side effect is that the first Klass* is now allocated directly
>> at the base of class space. If the class space base is aligned to 32bit,
>> the Klass* for boolean[].class is so too, and all lower 32bits are zero.
>>
>> That seems to trigger what I think is a bug in C1, in isPrimitive. There,
>> the Klass* pointer gets loaded from the mirror, and then compared with zero
>> (class objects for primitives have no associated Klass*, so isPrimitive()
>> returns true if that Klass* pointer is NULL).
>>
>> But to me it seems that this comparison is done with 32bit cmp:
>>
>> -------------
>>
>>   6512 Compiled method (c1)     822   31       1
>> java.lang.invoke.MethodTypeForm::canonicalize (233 bytes)
>> ..<snip>..
>>
>>   6544  ;;  block B2 [9, 13]
>>   6545   0x00007f2ca8e76f3f:   mov    0x50(%rsi),%rdi              ;
>> implicit exception: dispatches to 0x00007f2ca8e771ec
>>   6546   0x00007f2ca8e76f43:   cmp    $0x0,%edi      <<<< 32bit?
>>   6547   0x00007f2ca8e76f46:   mov    $0x0,%edi
>>   6548   0x00007f2ca8e76f4b:   jne    0x00007f2ca8e76f56
>>   6549   0x00007f2ca8e76f51:   mov    $0x1,%edi
>>   ;*invokevirtual isPrimitive {reexecute=0 rethrow=0 return_oop=0}
>>   6550                                                             ; -
>> java.lang.invoke.MethodTypeForm::canonicalize at 10 (line 263)
>>
>> -------------
>>
>> and therefore returns true for Klass pointers like 0x900000000.
>>
>> My fix makes the comparison 64bit.
>>
>> Would that be the correct way to fix it? Please note that I am new to the
>> compiler and this is my first fix in c1.
>>
>> At least on my system the fix seems to solve the error, and isPrimitive
>> now uses a 64bit compare, and the test errors are gone:
>>
>> 6544  ;;  block B2 [9, 13]
>>   6545   0x00007f7750e76f3f:   mov    0x50(%rsi),%rdi              ;
>> implicit exception: dispatches to 0x00007f7750e771ec
>>   6546   0x00007f7750e76f43:   cmp    $0x0,%rdi       <<<<<
>>   6547   0x00007f7750e76f47:   mov    $0x0,%edi
>>   6548   0x00007f7750e76f4c:   jne    0x00007f7750e76f57
>>   6549   0x00007f7750e76f52:   mov    $0x1,%edi
>>   ;*invokevirtual isPrimitive {reexecute=0 rethrow=0 return_oop=0}
>>   6550                                                             ; -
>> java.lang.invoke.MethodTypeForm::canonicalize at 10 (line 263)
>>
>>
>> Thanks, Thomas
>>
>>