[aarch64-port-dev ] RFR: Fix out by one in writing array barriers

[Andrew Haley]

On 12/12/2013 01:03 PM, Edward Nevill wrote:
> Hi,
> 
> The following patch fixes some problems I am seeing with GC in the JTReg hotspot tests, specifically compiler/8010927/Test8010927
> 
> The problem seems to be an out by one error in gen_write_ref_array_post_barrier
> 
> The original code reads
> 
>            __ lsr(start, start, CardTableModRefBS::card_shift);
>            __ add(end, end, BytesPerHeapOop);
>            __ lsr(end, end, CardTableModRefBS::card_shift);
>            __ sub(end, end, start); // number of bytes to copy
> 
>           const Register count = end; // 'end' register contains bytes count now
>           __ mov(scratch, (address)ct->byte_map_base);
>           __ add(start, start, scratch);
>           __ BIND(L_loop);
>           __ strb(zr, Address(start, count));
>           __ subs(count, count, 1);
>           __ br(Assembler::HI, L_loop);
> 
> This seems to me to be broken. The last store in the loop is always
> 
> strb zr, [start, 1]
> 
> because of the BHI. However it must store to [start, 0] because for
> each HeapOOP it must do
> 
> [base + [addr >> card_shift]] = 0
> 
> However in the above the last store is done to
> 
> [base + [addr >> card_shift] + 1] = 0
> 
> The end condition of the loop should therefore be BHS, not BHI to
> include 0.
> 
> However, this sometimes stores 1 too many (and sometimes not). The
> problem is the
> 
> add end, end, BytesPerHeapOop
> 
> which converts the inclusive pointer to an exclusive
> pointer. However this is not what we want. What we want is to store
> 0 in all the bytes in the range
> 
> [base + [start >> card_shift]] to [base + [end >> card_shift]]
> 
> The following patch fixes this.
> 
> OK to push?

I agree with this diagnosis.  However:

The first store is done to [base + [end >> card_shift]].  I think this
is wrong, because [base + [end >> card_shift]] is outside the range.
We need to make in an inclusive pointer.  So, we need to subtract 1
from end, not add it:

           __ lsr(start, start, CardTableModRefBS::card_shift);
           __ sub(end, end, BytesPerHeapOop); // end - 1 to make inclusive
           __ lsr(end, end, CardTableModRefBS::card_shift);
           __ sub(end, end, start); // number of bytes to copy

          const Register count = end; // 'end' register contains bytes count now
	  __ mov(scratch, (address)ct->byte_map_base);
          __ add(start, start, scratch);
	  __ BIND(L_loop);
	  __ strb(zr, Address(start, count));
          __ subs(count, count, 1);
          __ br(Assembler::HS, L_loop);

Andrew.