8219582: PPC: Crash after C1 checkcast patched and GC

[Anton Kozlov]

Hi, Martin, Goetz,

thank you for your efforts on fixing the bug.

Best regards,
Anton

On 01.03.2019 19:58, Doerr, Martin wrote:
> 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
>>>