8219582: PPC: Crash after C1 checkcast patched and GC

[Doerr, Martin]

Hi Götz,

thank you for the review. Pushed.

Best regards,
Martin


-----Original Message-----
From: Lindenmaier, Goetz 
Sent: Freitag, 1. März 2019 12:28
To: Doerr, Martin <martin.doerr at sap.com>; Anton Kozlov <akozlov at azul.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, 

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