Skip to content
This repository has been archived by the owner. It is now read-only.

8269897: Shenandoah: Treat UNKNOWN refs access as strong #219

Closed
wants to merge 4 commits into from
Closed
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -640,7 +640,7 @@ Node* ShenandoahBarrierSetC2::atomic_cmpxchg_val_at_resolved(C2AtomicParseAccess
load_store = kit->gvn().transform(new DecodeNNode(load_store, load_store->get_ptr_type()));
}
#endif
load_store = kit->gvn().transform(new ShenandoahLoadReferenceBarrierNode(NULL, load_store, access.decorators()));
load_store = kit->gvn().transform(new ShenandoahLoadReferenceBarrierNode(NULL, load_store, access.decorators() & ~ON_UNKNOWN_OOP_REF));
Copy link
Contributor

@shipilev shipilev Jul 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@zhengyu123 zhengyu123 Jul 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This load should go through slow path when GC is in progress, right? Should let slow path to resolve the strength? sounds there is possibility to resurrect dead oop.

return load_store;
}
return BarrierSetC2::atomic_cmpxchg_val_at_resolved(access, expected_val, new_val, value_type);
@@ -267,7 +267,7 @@ inline oop ShenandoahBarrierSet::AccessBarrier<decorators, BarrierSetT>::oop_ato

// 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.
res = ShenandoahBarrierSet::barrier_set()->load_reference_barrier<decorators, T>(res, NULL);
res = ShenandoahBarrierSet::barrier_set()->load_reference_barrier<decorators & ~ON_UNKNOWN_OOP_REF, T>(res, NULL);
Copy link
Contributor

@shipilev shipilev Jul 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@shipilev shipilev Jul 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also believe "xchg" needs the same fix and 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. ShenandoahBarrierSet::See oop_atomic_cmpxchg_not_in_heap for explanation.

bs->satb_enqueue(res);
return res;
}