Likely a bug in G1BarrierSetAssembler::oop_store_at

[Dmitry Samersoff]

Hello Everybody,

Thank you for the explanation.

On AARCH64 there are an assertion that all passed registers are
different, so the code similar to this one cause an assert crash.

I'll check the code and see how to arrange it on AArch64 side.

-Dmitry

On 03.09.2019 16:40, Thomas Schatzl wrote:
> Hi,
> 
> On Mon, 2019-09-02 at 16:41 -0700, Ioi Lam wrote:
>> CC-ing hotspot-gc-dev at openjdk.java.net.
>>
>>
>> Hi Dmitry,
>>
>> The code you mentioned also exists in the mainline repo:
>>
>>
> http://hg.openjdk.java.net/jdk/jdk/file/7cf02b2c1455/src/hotspot/cpu/x86/gc/g1/g1BarrierSetAssembler_x86.cpp#l392
>>
>> I think the code is OK, but maybe some from GC team can take a look
>> at it, too.
> 
> The code is okay. The only problem is that 
> 
>>
>> The movptr is necessary because BarrierSetAssembler::store_at may
>> modify the contents of val, so we save its value into a temp 
> 
> To be a little more exact, BarrierSetAssembler::store_at modifies the
> value in val if it is a reference, i.e. it compresses it with
> compressed oops enabled. For the inter-region filter we need the
> uncompressed value though, saving it in new_val/tmp2.
> 
>> register, tmp2.
>>
>>      if (needs_post_barrier) {
>>        // G1 barrier needs uncompressed oop for region cross check.
>>        if (UseCompressedOops) {
>>          new_val = tmp2;
>>          __ movptr(new_val, val);
>>        }
>>      }
>>      BarrierSetAssembler::store_at(masm, decorators, type,
>> Address(tmp1, 0), val, noreg, noreg, noreg);
>>
>>
>> Later, inside g1_write_barrier_post, tmp2 is written only here, but
>> we never read new_value after writing into tmp2, so we should be
>> safe.
>>
>>    __ jcc(Assembler::equal, runtime);
>>    __ subl(queue_index, wordSize);
>>    __ movptr(tmp2, buffer);
>> #ifdef _LP64
>>    __ movslq(rscratch1, queue_index);
>>    __ addq(tmp2, rscratch1);
>>    __ movq(Address(tmp2, 0), card_addr);
>> #else
>>    __ addl(tmp2, queue_index);
>>    __ movl(Address(tmp2, 0), card_addr);
>> #endif
>>    __ jmp(done);
>>
>>    __ bind(runtime);
>>    // save the live input values
>>    __ push(store_addr);
>>    __ push(new_val);
>> #ifdef _LP64
>>    __ call_VM_leaf(CAST_FROM_FN_PTR(address, 
>> G1BarrierSetRuntime::write_ref_field_post_entry), card_addr,
>> r15_thread);
>> #else
>>    __ push(thread);
>>    __ call_VM_leaf(CAST_FROM_FN_PTR(address, 
>> G1BarrierSetRuntime::write_ref_field_post_entry), card_addr, thread);
>>    __ pop(thread);
>> #endif
>>    __ pop(new_val);
>>    __ pop(store_addr);
>>
>>    __ bind(done);
>>
>>
>> My question is --- why are we saving new_value when making the
>> runtime call? new_val can only be val or tmp2, but it's OK to scratch
>> both registers in G1BarrierSetAssembler::oop_store_at(), and neither 
>> registers are used after the runtime call.
> 
> I can imagine that sometime/somewhere there has been an implicit
> contract that "val" should not be modified. Apparently G1 never did
> that anyway...
> 
> I will file a CR to look at this some more.
> 
>>
>> I commented out the push/pop of new_val and ran all tests under 
>> test/hotspot/jtreg/gc without problem.
> 
> Thanks,
>   Thomas
> 
>