8219582: PPC: Crash after C1 checkcast patched and GC

[Doerr, Martin]

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
>