RFR: Fix out by one in writing array barriers

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?

Ed.

--- CUT HERE ---
exporting patch:
# HG changeset patch
# User Edward Nevill edward.nevill@linaro.org
# Date 1386852655 0
#      Thu Dec 12 12:50:55 2013 +0000
# Node ID 36ec6f5b872338684a26d353b77d7b747558281d
# Parent  3c620760454c2c4ea1f871d178e7ca8700bf92d3
Fix out by 1 errors in writing array barriers

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.

On Thu, 2013-12-12 at 13:30 +0000, Andrew Haley wrote:

> 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);

But the end pointer is already inclusive, looking at the 3 calls to gen_write_ref_array_post_barrier in stubGenerator_aarch64.cpp

1)

   1268       __ sub(count, count, 1); // make an inclusive end pointer
   1269       __ lea(count, Address(d, count, Address::uxtw(exact_log2(size))));
   1270       gen_write_ref_array_post_barrier(d, count, rscratch1);

2)

   1320       __ sub(count, count, 1); // make an inclusive end pointer
   1321       __ lea(count, Address(d, count, Address::uxtw(exact_log2(size))));
   1322       gen_write_ref_array_post_barrier(d, count, rscratch1);

3)

   1699     __ add(to, to, -heapOopSize);         // make an inclusive end point        er
   1700     gen_write_ref_array_post_barrier(start_to, to, rscratch1);

Regards,
Ed.

On 12/12/2013 02:30 PM, Edward Nevill wrote:
> But the end pointer is already inclusive, looking at the 3 calls to gen_write_ref_array_post_barrier in stubGenerator_aarch64.cpp

OK, the patch is good.

Andrew.