Likely a bug in G1BarrierSetAssembler::oop_store_at
Thomas Schatzl
thomas.schatzl at oracle.com
Tue Sep 3 13:40:43 UTC 2019
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
More information about the valhalla-dev
mailing list