[aarch64-port-dev ] Against hidden read barriers in intrinsics

[Dmitrij Pochepko]

Hi,

(I'm not a shenandoah expert and might get things wrong, so correct me 
in this case).

We need to resolve brook pointers before accessing any data, potentially 
causing copy into to-space.

If that's the case, then fix for "JDK-8203157: Object equals abstraction 
for BarrierSetAssembler" ( 
http://hg.openjdk.java.net/jdk/jdk/rev/8434981a4137 ) seems to be wrong.

Specifically: 
hg.openjdk.java.net/jdk/jdk/annotate/8434981a4137/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp#l5141

This code (lines 5125-5134) is a bit tricky. It checks pointers for 
non-null value and load array length from array header to fire load 
instruction into ld pipelines as early as possible. And *after* that 
checks pointers for equality in parallel with loads, because such code 
pattern shows better result. In case of shenandoah it can cause load of 
potentially uninitialized data via brook pointers.

Do I understand it correctly?


Thanks,

Dmitrij


On 20.06.2018 17:32, Roman Kennke wrote:
> Am 20.06.2018 um 16:23 schrieb Andrew Haley:
>> We have some hand-coded AArch64 intrinsics, and they're being
>> converted to the modular GC interface.  However, I've noticed a
>> dangerous coding practice that we should stop before it starts.
>> In mainline jdk's MacroAssembler::arrays_equals, we have
>>
>>      cmpoop(a1, a2);
>>      br(EQ, SAME);
>>      ...
>>
>> As of today, MacroAssembler::cmpoop() does a Brooks pointer
>> dereference on *both* operands, and the following code relies on that
>> for correctness.  So, it's correct even for Shenandoah, but is a
>> ticking bomb.  If any programmer comes along and moves some code
>> before that innocuous-looking cmpoop(), it'd be looking at stale
>> copies of the data.
>>
>> When Shendandoah is merged into mainline we must not do this.  Let us
>> instead EXPLICITLY do
>>
>>    resolve_for_read(IN_HEAP, a1);
>>    resolve_for_read(IN_HEAP, a2);
>>
>>    ...
>>    cmp(a1, a2);
>>
>> This has the additional advantage that the read barrier is done as
>> early as possible.
>>
> Hmm, but this would not be correct. There are two correct ways to do
> object comparison with Shenandoah:
>
> resolve_for_write(IN_HEAP, a1);
> resolve_for_write(IN_HEAP, a2);
> cmp(a1, a2);
>
> I.e. *ensure* that both objects a1 and a2 are stable in to-space, then
> compare. This is a fairly big hammer though: especially in the case of
> potentially large arrays, this might cause copying those arrays before
> comparing them. The saner alternative is:
>
> cmp(a1, a2);
> je(done);
> resolve_for_read(IN_HEAP, a1);
> resolve_for_read(IN_HEAP, a2);
> bind(done);
>
> I.e. it only employs read-barriers, and it only does so on the
> false-path. The logic behind this is a little tricky: we want to avoid
> false negatives. If we know that a1!=a2, it can only be because:
> - they are truly !=
> - we're comparing from- and to-space copy of the same object
>
> we want to avoid the latter case. However, in this case we know that one
> copy is already in to-space. If they are the same object, doing
> read-barrier is sufficient to ensure we don't get false negatives. I
> believe Aarch64 we need additional load-load-membar:
>
>
> cmp(a1, a2);
> je(done);
> membar();
> resolve_for_read(IN_HEAP, a1);
> resolve_for_read(IN_HEAP, a2);
> cmp(a1, a2);
> bind(done);
>
> to prevent re-ordering the RBs before the cmp.
>
> Do you think it's better to write out all this, possibly with #if
> INCLUDE_SHENANDOAHGC, in all the places where it needs to be? Or call
> some abstraction layer into cmpoop() to do the right thing? Maybe better
> document cmpoop() what it might or might not do, and what the user might
> or might not do with it?
>
> Roman