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

[Thomas Stüfe]

Thank you, Dean.

On Fri 1. Nov 2019 at 23:10, <dean.long at oracle.com> wrote:

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