RFR: 8269897: Shenandoah: Resolve UNKNOWN access strength, where possible [v3]

[Aleksey Shipilev]

On Fri, 9 Jul 2021 15:31:23 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

>> We've observed test failures in jcstress, see:
>> https://bugs.openjdk.java.net/browse/JDK-8269897
>> 
>> We used to treat UNKNOWN reference accesses like weak accesses. UNKNOWN is used for Unsafe, reflection and JNI accesses, where it cannot be determined at compilation-time if we are accessing a regular field or a Reflection.referent field. The rationale for treating UNKNOWN as weak was that if the reference is a regular reference, then the value would be strongly reachable anyway, and only if it is a referent field would reachability matter. However, it turns out that this assumption is wrong: the test shows that a reference that is only weakly reachable can be legitimately written into a field, thus resurrecting the reference, and when that weakly reachable reference is loaded, it would be (wrongly) filtered as NULL.
>> 
>> A fix is to treat UNKNOWN accesses as strong. Accessing Reference.referent via reflection, JNI or Unsafe is Bad Idea anyway.
>> This test shows the problem with CAS, but I believe it affects all accesses via reflection, JNI, etc.
>> 
>> Testing:
>>  - [x] the provided jcstress test
>>  - [x] hotspot_gc_shenandoah
>>  - [x] tier1
>
> Roman Kennke has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add asserts for various decorators to runtime barriers

src/hotspot/share/gc/shenandoah/shenandoahBarrierSet.hpp line 128:

> 126:   class AccessBarrier: public BarrierSet::AccessBarrier<decorators, BarrierSetT> {
> 127:     typedef BarrierSet::AccessBarrier<decorators, BarrierSetT> Raw;
> 128: 

This change looks unnecessary.

src/hotspot/share/gc/shenandoah/shenandoahBarrierSet.inline.hpp line 320:

> 318: template <DecoratorSet decorators, typename BarrierSetT>
> 319: inline oop ShenandoahBarrierSet::AccessBarrier<decorators, BarrierSetT>::oop_atomic_xchg_in_heap_at(oop base, ptrdiff_t offset, oop new_value) {
> 320:   assert((decorators & ON_STRONG_OOP_REF) != 0, "must be present");

Shouldn't this be `| ON_UNKNOWN_OOP_REF`? We are doing `resolve_possibly_unknown_oop_ref_strength` later.

In fact, I am not so sure about asserting `ON_STRONG_OOP_REF` here and elsewhere as well. The future `ON_WEAK_OOP_REF` code should work fine here too? I think only asserting that `ON_Uresolve_possibly_unknown_oop_ref_strengthNKNOWN_OOP_REF` is not passed on bad paths is enough?

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

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