8383810: Shenandoah: Simplify native CAS barriers

Reviewed-by: rkennke, xpeng, kdnilsen

Author: shipilev

src/hotspot/share/gc/shenandoah/shenandoahBarrierSet.inline.hpp

@@ -249,29 +249,35 @@ inline oop ShenandoahBarrierSet::oop_load(DecoratorSet decorators, T* addr) {
 
 template <typename T>
 inline oop ShenandoahBarrierSet::oop_cmpxchg(DecoratorSet decorators, T* addr, oop compare_value, oop new_value) {
-  oop res;
-  oop expected = compare_value;
-  do {
-    compare_value = expected;
-    res = RawAccess<>::oop_atomic_cmpxchg(addr, compare_value, new_value);
-    expected = res;
-  } while ((compare_value != expected) && (resolve_forwarded(compare_value) == resolve_forwarded(expected)));
+  shenandoah_assert_not_in_cset_except(nullptr, compare_value, (compare_value == nullptr || ShenandoahHeap::heap()->cancelled_gc()));
+  shenandoah_assert_not_in_cset_except(nullptr, new_value, (new_value == nullptr || ShenandoahHeap::heap()->cancelled_gc()));
 
-  // 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 = load_reference_barrier(decorators, res, static_cast<T*>(nullptr));
-  satb_enqueue(res);
-  return res;
+  // Handle the previous value through SATB, as we are about to perform the store.
+  oop prev = RawAccess<>::oop_load(addr);
+  satb_enqueue(prev);
+
+  // Perform LRB on location to fix it up for this and all following accesses.
+  // This guarantees there are no false negatives due to concurrent evacuation,
+  // and the value loaded later by CAS is sanitized by some LRB, or is null.
+  load_reference_barrier(decorators, prev, addr);
+
+  return RawAccess<>::oop_atomic_cmpxchg(addr, compare_value, new_value);
 }
 
 template <typename T>
 inline oop ShenandoahBarrierSet::oop_xchg(DecoratorSet decorators, T* addr, oop new_value) {
-  oop previous = RawAccess<>::oop_atomic_xchg(addr, new_value);
-  // 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.
-  previous = load_reference_barrier<T>(decorators, previous, static_cast<T*>(nullptr));
-  satb_enqueue(previous);
-  return previous;
+  shenandoah_assert_not_in_cset_except(nullptr, new_value, (new_value == nullptr || ShenandoahHeap::heap()->cancelled_gc()));
+
+  // Handle the previous value through SATB, as we are about to perform the store.
+  oop prev = RawAccess<>::oop_load(addr);
+  satb_enqueue(prev);
+
+  // Perform LRB on location to fix it up for this and all following accesses.
+  // This is purely opportunistic: we would not have any false negatives here.
+  // This guarantees the value loaded later by XCHG is sanitized by some LRB, or is null.
+  load_reference_barrier(decorators, prev, addr);
+
+  return RawAccess<>::oop_atomic_xchg(addr, new_value);
 }
 
 template <DecoratorSet decorators, typename BarrierSetT>
@@ -338,15 +344,17 @@ inline void ShenandoahBarrierSet::AccessBarrier<decorators, BarrierSetT>::oop_st
 template <DecoratorSet decorators, typename BarrierSetT>
 template <typename T>
 inline oop ShenandoahBarrierSet::AccessBarrier<decorators, BarrierSetT>::oop_atomic_cmpxchg_not_in_heap(T* addr, oop compare_value, oop new_value) {
-  assert((decorators & (AS_NO_KEEPALIVE | ON_UNKNOWN_OOP_REF)) == 0, "must be absent");
+  assert((decorators & AS_NO_KEEPALIVE) == 0, "CAS only with keep-alive");
+  assert((decorators & ON_STRONG_OOP_REF) != 0, "CAS only for strong refs");
   ShenandoahBarrierSet* bs = ShenandoahBarrierSet::barrier_set();
   return bs->oop_cmpxchg(decorators, addr, compare_value, new_value);
 }
 
 template <DecoratorSet decorators, typename BarrierSetT>
 template <typename T>
 inline oop ShenandoahBarrierSet::AccessBarrier<decorators, BarrierSetT>::oop_atomic_cmpxchg_in_heap(T* addr, oop compare_value, oop new_value) {
-  assert((decorators & (AS_NO_KEEPALIVE | ON_UNKNOWN_OOP_REF)) == 0, "must be absent");
+  assert((decorators & AS_NO_KEEPALIVE) == 0, "CAS only with keep-alive");
+  assert((decorators & ON_STRONG_OOP_REF) != 0, "CAS only for strong refs");
   ShenandoahBarrierSet* bs = ShenandoahBarrierSet::barrier_set();
   oop result = bs->oop_cmpxchg(decorators, addr, compare_value, new_value);
   if (ShenandoahCardBarrier) {
@@ -357,9 +365,20 @@ inline oop ShenandoahBarrierSet::AccessBarrier<decorators, BarrierSetT>::oop_ato
 
 template <DecoratorSet decorators, typename BarrierSetT>
 inline oop ShenandoahBarrierSet::AccessBarrier<decorators, BarrierSetT>::oop_atomic_cmpxchg_in_heap_at(oop base, ptrdiff_t offset, oop compare_value, oop new_value) {
-  assert((decorators & AS_NO_KEEPALIVE) == 0, "must be absent");
+  assert((decorators & AS_NO_KEEPALIVE) == 0, "CAS only with keep-alive");
+  assert((decorators & (ON_STRONG_OOP_REF | ON_UNKNOWN_OOP_REF)) != 0, "CAS only for strong refs OR unknown refs (Unsafe)");
   ShenandoahBarrierSet* bs = ShenandoahBarrierSet::barrier_set();
+
+  // Unsafe.compareAndExchange/Set come here with ON_UNKNOWN_OOP_REF set.
+  // These are normally strong refs, but one can use Unsafe on Reference.referent.
+  // We cannot deal with that case. If application does Unsafe operations on
+  // Reference.referent field, this likely breaks weak reference semantics already.
+  // We upgrade the access to strong in (sometimes futile) attempt to maintain heap
+  // integrity, and assert in debug builds for better diagnostics.
   DecoratorSet resolved_decorators = AccessBarrierSupport::resolve_possibly_unknown_oop_ref_strength<decorators>(base, offset);
+  assert((resolved_decorators & ON_STRONG_OOP_REF) != 0, "Application error: CAS on weak location");
+  resolved_decorators = (resolved_decorators & ~ON_DECORATOR_MASK) | ON_STRONG_OOP_REF;
+
   auto addr = AccessInternal::oop_field_addr<decorators>(base, offset);
   oop result = bs->oop_cmpxchg(resolved_decorators, addr, compare_value, new_value);
   if (ShenandoahCardBarrier) {
@@ -371,15 +390,17 @@ inline oop ShenandoahBarrierSet::AccessBarrier<decorators, BarrierSetT>::oop_ato
 template <DecoratorSet decorators, typename BarrierSetT>
 template <typename T>
 inline oop ShenandoahBarrierSet::AccessBarrier<decorators, BarrierSetT>::oop_atomic_xchg_not_in_heap(T* addr, oop new_value) {
-  assert((decorators & (AS_NO_KEEPALIVE | ON_UNKNOWN_OOP_REF)) == 0, "must be absent");
+  assert((decorators & AS_NO_KEEPALIVE) == 0, "XCHG only with keep-alive");
+  assert((decorators & ON_STRONG_OOP_REF) != 0, "XCHG only for strong refs");
   ShenandoahBarrierSet* bs = ShenandoahBarrierSet::barrier_set();
   return bs->oop_xchg(decorators, addr, new_value);
 }
 
 template <DecoratorSet decorators, typename BarrierSetT>
 template <typename T>
 inline oop ShenandoahBarrierSet::AccessBarrier<decorators, BarrierSetT>::oop_atomic_xchg_in_heap(T* addr, oop new_value) {
-  assert((decorators & (AS_NO_KEEPALIVE | ON_UNKNOWN_OOP_REF)) == 0, "must be absent");
+  assert((decorators & AS_NO_KEEPALIVE) == 0, "XCHG only with keep-alive");
+  assert((decorators & ON_STRONG_OOP_REF) != 0, "XCHG only for strong refs");
   ShenandoahBarrierSet* bs = ShenandoahBarrierSet::barrier_set();
   oop result = bs->oop_xchg(decorators, addr, new_value);
   if (ShenandoahCardBarrier) {
@@ -390,9 +411,20 @@ inline oop ShenandoahBarrierSet::AccessBarrier<decorators, BarrierSetT>::oop_ato
 
 template <DecoratorSet decorators, typename BarrierSetT>
 inline oop ShenandoahBarrierSet::AccessBarrier<decorators, BarrierSetT>::oop_atomic_xchg_in_heap_at(oop base, ptrdiff_t offset, oop new_value) {
-  assert((decorators & AS_NO_KEEPALIVE) == 0, "must be absent");
+  assert((decorators & AS_NO_KEEPALIVE) == 0, "XCHG only with keep-alive");
+  assert((decorators & (ON_STRONG_OOP_REF | ON_UNKNOWN_OOP_REF)) != 0, "XCHG only for strong refs OR unknown refs (Unsafe)");
   ShenandoahBarrierSet* bs = ShenandoahBarrierSet::barrier_set();
+
+  // Unsafe.getAndSet comes here with ON_UNKNOWN_OOP_REF set.
+  // These are normally strong refs, but one can use Unsafe on Reference.referent.
+  // We cannot deal with that case. If application does Unsafe operations on
+  // Reference.referent field, this likely breaks weak reference semantics already.
+  // We upgrade the access to strong in (sometimes futile) attempt to maintain heap
+  // integrity, and assert in debug builds for better diagnostics.
   DecoratorSet resolved_decorators = AccessBarrierSupport::resolve_possibly_unknown_oop_ref_strength<decorators>(base, offset);
+  assert((resolved_decorators & ON_STRONG_OOP_REF) != 0, "Application error: XCHG on weak location");
+  resolved_decorators = (resolved_decorators & ~ON_DECORATOR_MASK) | ON_STRONG_OOP_REF;
+
   auto addr = AccessInternal::oop_field_addr<decorators>(base, offset);
   oop result = bs->oop_xchg(resolved_decorators, addr, new_value);
   if (ShenandoahCardBarrier) {

test/hotspot/jtreg/gc/shenandoah/TestWeakReferenceCAS.java

@@ -0,0 +1,117 @@
+/*
+ * Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/**
+ * @test
+ * @summary Test that weak CAS attempt on Reference.referent is handled properly
+ * @requires vm.gc.Shenandoah
+ * @library /test/lib
+ * @modules java.base/jdk.internal.misc:+open
+ *
+ * @run driver TestWeakReferenceCAS
+ */
+
+import java.util.*;
+import java.lang.ref.*;
+import java.lang.reflect.*;
+import jdk.internal.misc.Unsafe;
+
+import jdk.test.lib.Platform;
+import jdk.test.lib.process.ProcessTools;
+import jdk.test.lib.process.OutputAnalyzer;
+
+public class TestWeakReferenceCAS {
+
+    public static void main(String[] args) throws Exception {
+        if (args.length == 0) {
+            driver("CAS");
+            driver("CAE");
+            driver("GAS");
+        } else {
+            switch (args[0]) {
+                case "CAS":
+                    Test.testCAS();
+                    break;
+                case "CAE":
+                    Test.testCAE();
+                    break;
+                case "GAS":
+                    Test.testGAS();
+                    break;
+                default:
+                    throw new IllegalArgumentException("Unknown test mode: " + args[0]);
+            }
+        }
+    }
+
+    private static void driver(String test) throws Exception {
+        OutputAnalyzer output = ProcessTools.executeLimitedTestJava(
+            "-Xmx128m",
+            "--add-exports", "java.base/jdk.internal.misc=ALL-UNNAMED",
+            "--add-opens", "java.base/java.lang.ref=ALL-UNNAMED",
+            "-XX:+UseShenandoahGC",
+            TestWeakReferenceCAS.class.getName(),
+            test);
+        if (Platform.isDebugBuild()) {
+            output.shouldNotHaveExitValue(0);
+            output.shouldContain("Application error:");
+        } else {
+            output.shouldHaveExitValue(0);
+        }
+    }
+
+    static class Test {
+        static final Unsafe UNSAFE = Unsafe.getUnsafe();
+        static final long OFFSET;
+
+        static {
+            try {
+                Field f = Reference.class.getDeclaredField("referent");
+                OFFSET = UNSAFE.objectFieldOffset(f);
+            } catch (Exception e) {
+                throw new RuntimeException(e);
+            }
+        }
+
+        static void testCAS() {
+            Object obj = new Object();
+            Object next = new Object();
+            WeakReference<Object> ref = new WeakReference<>(obj);
+            UNSAFE.compareAndSetReference(ref, OFFSET, obj, next);
+        }
+
+        static void testCAE() {
+            Object obj = new Object();
+            Object next = new Object();
+            WeakReference<Object> ref = new WeakReference<>(obj);
+            UNSAFE.compareAndExchangeReference(ref, OFFSET, obj, next);
+        }
+
+        static void testGAS() {
+            Object obj = new Object();
+            Object next = new Object();
+            WeakReference<Object> ref = new WeakReference<>(obj);
+            UNSAFE.getAndSetReference(ref, OFFSET, next);
+        }
+    }
+}