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

[Thomas Stüfe]

Hi Dean,

Yes, that might be clearer. I probably still would have to fix up
LIR_Assembler::comp_op() for more than just x86, since most would not work
for comparisons with T_ADDRESS constants.

There are still a lot of tiny things I do not understand about the c1
coding, e.g. why addressConst() takes only a 32bit number. Or what
"single_cpu" and "double_cpu" means for LIR operands. I have to study this
code a bit more. So I withdraw this patch and will come up with a better
one later.

Thank you,

Thomas



On Fri, Oct 25, 2019 at 9:54 PM <dean.long at oracle.com> wrote:

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