Likely a bug in G1BarrierSetAssembler::oop_store_at

[Ioi Lam]

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
>