RFR(S): 8216989 - CardTableBarrierSetAssembler::gen_write_ref_array_post_barrier() does not check for zero length on AARCH64

[Andrew Dinn]

Hello Dmitrij,

On 18/03/2019 17:03, Dmitrij Pochepko wrote:
> Hi all,
> 
> please review patch for JDK-8216989
> CardTableBarrierSetAssembler::gen_write_ref_array_post_barrier() does
> not check for zero length on AARCH64
> webrev: http://cr.openjdk.java.net/~dpochepk/8216989/webrev.01/
> 
> All platforms except AARCH64 performs zero length check in arraycopy
> post barrier and skip card marking for zero length arrays. Missing check
> can lead to wrong card marking. This patch adds such check.
> 
> Testing (using parallel gc, because default g1 is not affected):
> 
> - JCK
> - jtreg hotspot tests: compiler/*, gc/* and runtime/*
> - jtreg jdk tier1-3 tests
> 
> no regressions found.
> 
> CR: https://bugs.openjdk.java.net/browse/JDK-8216989

The patch looks good on its own. However, I think there is a bigger
problem here and a better solution is available.

On x86_64 the stub code calls arraycopy_prologue passing in a start +
dest address (dst) and count (cnt). The implementation in
ShenandoahBarrierSetAssembler calls out to a C runtime handler
(ShenandoahRuntime::write_ref_array_post_entry), passing dst and cnt.
The one in ModRefBarrierSetAssembler virtually invokes either
G1BarrierSetAssembler::gen_write_ref_array_post_barrier or
CardTableBarrierSetAssembler::gen_write_ref_array_post_barrier, passing
in dst and count.

The G1 implementation calls out to C runtime code passing dst and count.
The CardTable implementation subtracts 1 from count and then adds dst to
cnt to get an inclusive end address and then processes the dst entries
in a loop. So, the latter bails out at the start if cnt == 0 as there is
no work to do.

On AArch64 for some unexplained reason it is the stub code which
modifies cnt, performing the decrement and address add. So, cnt is
passed in to the ModRef and Shenandoah barrier set implementations of
arraycopy_prologue as an inclusive end pointer. The implementation of
arraycopy_prologue in ModRefBarrierSetAssembler passes these values on
through to G1BarrierSetAssembler::gen_write_ref_array_post_barrier and
CardTableBarrierSetAssembler::gen_write_ref_array_post_barrier -- the
AArch64 versions -- using a virtual invoke, just as for x86

So, the AArch64 CardTable implementation now needs to bail out if end <
start where the x86 version bails out if cnt == 0. And, of course, it
doesn't include code for the decrement and pointer add. However, the
AArch64 ShenandoahBarrierSetAssembler::arraycopy_epilogue implementation
now has to convert its inclusive end pointer argument back to a count.
So, it adds BytesPerHeapOop to end, subtracts start and then shifts by
LogBytesPerHeapOop. Similarly, the G1 implementation of
gen_write_ref_array_post_barrier has to convert its end pointer back to
a count performing the same operations.

This looks like a pointless divergence from x86_64 that simply adds more
work and complicates the code. So, I suggest fixing AArch64 by following
x86_64 i.e.

  relocating the conversion of count to an inclusive end pointer back
into the CardTable implementation of gen_write_ref_array_post_barrier

  removing the pointer arithmetic to re-establish the count in
ShenandoahBarrierSetAssembler::arraycopy_epilogue and
G1BarrierSetAssembler::gen_write_ref_array_post_barrier

  rewriting your bail out test to detect cnt == 0

regards,


Andrew Dinn
-----------
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander