Likely a bug in G1BarrierSetAssembler::oop_store_at

[Doerr, Martin]

Hi,

I agree with Ioi, tmp2 only gets modified after new_val was read for the last time.
So I can't see any bug here.

> 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 think we could skip saving and restoring it if we use CompressedOops (because it's just a temp register).
If running with -XX:-UseCompressedOops, I think we should preserve the register "val".
Users of store_at may not expect val to get killed.

Best regards,
Martin


> -----Original Message-----
> From: hotspot-gc-dev <hotspot-gc-dev-bounces at openjdk.java.net> On
> Behalf Of Ioi Lam
> Sent: Dienstag, 3. September 2019 01:41
> To: valhalla-dev at openjdk.java.net; hotspot-gc-dev <hotspot-gc-
> dev at openjdk.java.net>
> Subject: Re: Likely a bug in G1BarrierSetAssembler::oop_store_at
> 
> 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 movptr is necessary because BarrierSetAssembler::store_at may modify
> the contents of val, so we save its value into a temp 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 commented out the push/pop of new_val and ran all tests under
> test/hotspot/jtreg/gc without problem.
> 
> 
> Thanks
> - Ioi
> 
> On 9/1/19 9:06 AM, Dmitry Samersoff wrote:
> > Hello Everybody,
> >
> > I found a following code in x86 G1BarrierSetAssembler::oop_store_at
> >
> > It looks like we pass the same register as both new_val and tmp2 to
> > g1_write_barrier_post.
> >
> > I don't have x86 setup in hands so can't say how critical it is.
> >
> >
> >   // 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);
> >
> >      if (needs_post_barrier) {
> >        g1_write_barrier_post(masm /*masm*/,
> >                              tmp1 /* store_adr */,
> >                              new_val /* new_val */,
> >                              rthread /* thread */,
> >                              tmp3 /* tmp */,
> >                              tmp2 /* tmp2 */);
> >      }
> >
> > -Dmitry
> >