RFR: 8263361: Incorrect arraycopy stub selected by C2 for SATB collectors

Erik Österlund eosterlund at openjdk.java.net
Mon Mar 15 11:29:08 UTC 2021


On Mon, 15 Mar 2021 10:38:58 GMT, Nils Eliasson <neliasso at openjdk.org> wrote:

> This bug has only been reproduced with -XX:+StressReflectiveCode and -XX:-ReduceInitialCardMarks. If StressReflectiveCode is necessary isn't obvious.
> 
> I am fixing three things:
> 
> 1) When running with ReduceInitialCardMarks enabled the stubroutines for int- or long-arraycopy will be used for arraycopies to tightly allocated arrays (like when cloning). With ReduceInitialCardMarks disabled the oop-version will be used with barriers.
> 
> The generate_arraycopy method in PhaseMacroExpand have a bool dest_uninitialized field - but that field denotes that we have proven that no zeroing is necessary (since all fields will be written anyway). 
> 
> To fix this bug generate_unchecked_arraycopy with the param dest_uninitialized will need to be appended with a check for tighly coupled object-copies when ReduceInitialCardMarks is disabled.
> 
> 2) I also fix a bug in library_call kit where arraycopy for clones doesn't get marked as tightly coupled - The call to tightly_coupled_allocation can't be used when the IR isn't constructed yet.
> 
> 3) In stubroutines.cpp I fix so that the name of the arraycopy stubroutines gets appended with _uninit when appropriate. This shows up in ir-dumps and IGV.
> 
> Please review,
> Nils Eliasson

Changes requested by eosterlund (Reviewer).

src/hotspot/share/opto/macroArrayCopy.cpp line 666:

> 664:     // A tightly coupled arraycopy of reference types might touch uninitialized memory
> 665:     // which will make cardbarriers fail.
> 666:     bool may_be_uninitialized = dest_uninitialized || (ac->is_alloc_tightly_coupled() && !ReduceInitialCardMarks && is_reference_type(basic_elem_type));

I wonder if you can just adjust the way that dest_uninitialized is computed way above to unconditionally check if ac->is_alloc_tightly_coupled(), so that the value of dest_uninitialized makes sense in all cases.
Part of the reason I'm asking is that the embedded information about !ReduceInitialCardMarks really belongs to the GC, and is utilized by some but not all GCs, sometimes in not trivial ways, in BarrierSetC2::array_copy_requires_gc_barriers(), which I'm guessing is what you are referring to here. So in a way we rely on the implementation details of BarrierSetC2::array_copy_requires_gc_barriers() to compute whether we need to correctly annotate the arraycopy stub or not, regarding whether the destination is uninitialized or not. It would be nice if it was always annotated correctly, regardless of whether correct annotation is required by the GC implementation backend or not. By unconditionally performing a correct computation of dest_uninitialized, it seems like we would solve that. Or are there non-obvious reasons why that can not be done?
I guess what I'm proposing is to modify the condition where dest_uninitialized is computed, around where this hilarious comment is: "You break it, you buy it."
In fact, reading the comment just above that, it is kind of hinting at how something like is_alloc_tightly_coupled() had been used there before. It's saying that "Note:  Because tightly_coupled_allocation performs checks on the out-edges of the dest, we need to avoid making derived pointers from it until we have checked its uses.", however there is no such check to be found there, which seems like the cause of the bug.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3008


More information about the hotspot-compiler-dev mailing list