[jdk17] RFR: 8269897: Shenandoah: Treat UNKNOWN refs access as strong [v2]

[Aleksey Shipilev]

On Wed, 7 Jul 2021 12:37:16 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:
> 
>   Treat UNKNOWN as STRONG more selectively only on CAE

Need synopsis change after limiting a fix?
Need to handle `xchg` case too. 

This patch passes `tier1` with Shenandoah for me, but fails jcstress bundle when run with C1. A relevant `ShenandoahBarrierSetC1` change is quite probably missing.

Also, probably `Unsafe` `get` and `set` require the same handling: under concurrent weak roots, it is safer to return overly strong value, not NULL for strong refs. Let it be users' problem accessing weak values with Unsafe?

src/hotspot/share/gc/shenandoah/c2/shenandoahBarrierSetC2.cpp line 643:

> 641:     }
> 642: #endif
> 643:     load_store = kit->gvn().transform(new ShenandoahLoadReferenceBarrierNode(NULL, load_store, access.decorators() & ~ON_UNKNOWN_OOP_REF));

Add a comment:


    // Convert ON_UNKNOWN_OOP_REF to strong, so that we don't return NULL for weak values.
    // See ShenandoahBarrierSet::See oop_atomic_cmpxchg_not_in_heap for explanation.

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

> 268:   // Note: We don't need a keep-alive-barrier here. We already enqueue any loaded reference for SATB anyway,
> 269:   // because it must be the previous value.
> 270:   res = ShenandoahBarrierSet::barrier_set()->load_reference_barrier<decorators & ~ON_UNKNOWN_OOP_REF, T>(res, NULL);

Add a comment: 


  // Note: We don't need a keep-alive-barrier here. We already enqueue any loaded reference for SATB anyway,
  // because it must be the previous value. Convert ON_UNKNOWN_OOP_REF to strong, so that we don't return
  // NULL for weak values, breaking CAS contract. The downside is this method effectively resurrecting
  // weak values without GC knowing about it. If Unsafe/JNI users are using this method on weak values
  // (i.e. doing unsafe accesses to Reference.referent), they are on their own.

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

Changes requested by shade (Reviewer).

PR: https://git.openjdk.java.net/jdk17/pull/219