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

[Nils Eliasson]

On Mon, 15 Mar 2021 13:19:46 GMT, Nils Eliasson <neliasso at openjdk.org> wrote:

>> 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.
>
> There is a bit of a conceptual disconnect in the code. In generate_arraycopy dest_uninitialized is set only when the allocation isn't already "complete" - that is we haven't already been able to prove that no zeroing is neccessary. Clones will for example create complete allocations from the beginning - but they aren't initialized.
> 
> The code that is guarded by dest_uninitialized will add zeroing for the array allocation that is outside the copy range. The part that is inside the copy range will still not be initialized. (But that is ok - since dest_uninitialized is set).
> 
> The problem here is that there are acopies that are proven to be complete from the beginning (like clones) that will copy uninitialized memory - and they don't need anything of the zeroing code that is guarded by dest_uninitialized.

My plan is to rename dest_uninitialized to dest_needs_zeroing and add a separate variable for when the acopy is done to uninitilialized memory.

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

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