8219582: PPC: Crash after C1 checkcast patched and GC

[Lindenmaier, Goetz]

Hi Martin, 

Thanks for addressing this issue!

Well, this shuffling of registers is quite complex and error prone.
Forcing the register allocation to have 5 different registers seems
to be the safer fix. But I checked, about 10% of the cases in jvm98
only use 4 registers here, and I would consider this a significant
amount to justify the complexity.

For me, all the renaming and assignment of conditions makes
the code harder to read,  (e.g., keep_object_alive encodes
checkcast && reg_conflict, I.e., it rather means "must_move_obj" ...),
as well as the names of registers (k_RInfo, klass_RInfo ... what's the idea 
behind these names?) 
But I understand you want to keep this similar to the code on 
other platforms, which is also desireable. 

So consider this reviewed.

Best regards,
  Goetz.

> -----Original Message-----
> From: hotspot-compiler-dev <hotspot-compiler-dev-
> bounces at openjdk.java.net> On Behalf Of Doerr, Martin
> Sent: Tuesday, February 26, 2019 6:51 PM
> To: Anton Kozlov <akozlov at azul.com>; 'hotspot-compiler-
> dev at openjdk.java.net' <hotspot-compiler-dev at openjdk.java.net>
> Subject: [CAUTION] RE: 8219582: PPC: Crash after C1 checkcast patched and
> GC
> 
> Hi Anton,
> 
> I noticed that my fix had missed that the Runtime1::slow_subtype_check_id
> stub needs fixed registers. So I need to undo the register changes before
> and after that call.
> 
> Quite messy. Now I remember why I didn't want to do it this way originally ��
> 
> I just added the shuffling in place:
> http://cr.openjdk.java.net/~mdoerr/8219582_ppc64_c1_fix/webrev.01/
> 
> I'll run tests and try to find somebody for a 2nd review.
> 
> Best regards,
> Martin
> 
> 
> -----Original Message-----
> From: Anton Kozlov <akozlov at azul.com>
> Sent: Montag, 25. Februar 2019 16:25
> To: Doerr, Martin <martin.doerr at sap.com>; 'hotspot-compiler-
> dev at openjdk.java.net' <hotspot-compiler-dev at openjdk.java.net>
> Subject: Re: 8219582: PPC: Crash after C1 checkcast patched and GC
> 
> Hi, Martin,
> 
> my bad, the null check looked like optimization (why to resolve if the klass
> doesn't matter), but it's actually specified
> 
>     If objectref is null , then the operand stack is unchanged.
>     Otherwise, the named class, array, or interface type is resolved ...
> 
> Your patch looks correct; reproducer does not fail anymore.
> 
> A very minor note:
> >    } else if (code == lir_checkcast) {
> >      Label success, failure;
> > -    emit_typecheck_helper(op, &success, /*fallthru*/&failure, &success);
> // Moves obj to dst.
> > +    emit_typecheck_helper(op, &success, /*fallthru*/&failure, &success);
> >      __ b(*op->stub()->entry());
> >      __ align(32, 12);
> >      __ bind(success);
> > +    __ mr(op->result_opr()->as_register(), op->object()->as_register());
> shouldn't __mr_if_needed be here? As it was before, obj and dst can be
> same register:
> 
> >    __ cmpdi(CCR0, obj, 0);
> > -  if (move_obj_to_dst || reg_conflict) {
> > -    __ mr_if_needed(dst, obj);
> ^^^ here
> > -    if (reg_conflict) { obj = dst; }
> > -  }
> >
> 
> Thanks for fixing the patch!
> -- Anton
> 
> On 25.02.2019 17:20, Doerr, Martin wrote:
> > Hi Anton,
> >
> > your proposal fixes the issue, but introduces another one:
> > We must not use the patching stub before the null check (resolving is not
> allowed at this place).
> > JCK tests exist which verify this:
> > vm/instr/checkcast
> > vm/instr/instanceof
> >
> > So I suggest to fix it this way:
> > http://cr.openjdk.java.net/~mdoerr/8219582_ppc64_c1_fix/webrev.01/
> > It is closer to the x86 implementation.
> >
> > Can you verify this proposal, please?
> >
> > Thanks again for your helpful analysis of the problem.
> >
> > Best regards,
> > Martin
> >
> >
> > -----Original Message-----
> > From: hotspot-compiler-dev <hotspot-compiler-dev-
> bounces at openjdk.java.net> On Behalf Of Doerr, Martin
> > Sent: Freitag, 22. Februar 2019 17:12
> > To: 'hotspot-compiler-dev at openjdk.java.net' <hotspot-compiler-
> dev at openjdk.java.net>; Anton Kozlov <akozlov at azul.com>
> > Subject: [CAUTION] RFR: 8219582: PPC: Crash after C1 checkcast patched
> and GC
> >
> > Hi Anton,
> >
> > reposting on hotspot-compiler-dev.
> > Thanks for analyzing the issue and for providing a fix. I'll take a closer look
> next week.
> >
> > Best regards,
> > Martin
> >
> >
> > -----Original Message-----
> > From: hotspot-runtime-dev <hotspot-runtime-dev-
> bounces at openjdk.java.net> On Behalf Of Anton Kozlov
> > Sent: Freitag, 22. Februar 2019 16:05
> > To: hotspot-runtime-dev at openjdk.java.net
> > Subject: RFR: 8219582: PPC: Crash after C1 checkcast patched and GC
> >
> > Hi,
> >
> > bug: https://bugs.openjdk.java.net/browse/JDK-8219582
> > webrev: http://cr.openjdk.java.net/~akozlov/8219582/webrev.00/
> >
> > PPC C1 checkcast implementation overcomes possible object-to-check and
> temp registers conflict by using destination register as temp to store the
> object. It usually works, but after object moved to dst and before checkcast
> completed, safepoint may occur because of implicit runtime call from
> klass2reg_with_patching. During the call, oop in dst register is not visible to
> GC, so it will not be updated after GC moved objects.
> >
> > Please review the fix, that is to load klass (and may be call runtime) at
> beginning of the LIR instruction, when all oops are in place expected by GC.
> >
> > Thanks,
> > Anton
> >