Disable the UnsafeCASObject intrinsic with read barriers.

The current implementations of the UnsafeCASObject
intrinsics are missing a read barrier.  Temporarily disable
them when read barriers are enabled.

Also re-enable the jsr166.LinkedTransferQueueTest tests that
were failing on the concurrent collector configuration, as
the UnsafeCASObject JNI implementation now correctly
implements the read barrier which was missing.

Bug: 25883050
Bug: 26205973
Change-Id: Iaf5d515532949662d0ac6702c9452a00aa0a23e6

diff --git a/compiler/optimizing/intrinsics_arm.cc b/compiler/optimizing/intrinsics_arm.cc
index e8181bb..4683aee 100644 (file)
--- a/compiler/optimizing/intrinsics_arm.cc
+++ b/compiler/optimizing/intrinsics_arm.cc
@@ -825,8 +825,15 @@ static void GenCas(LocationSummary* locations, Primitive::Type type, CodeGenerat
   Label loop_head;
   __ Bind(&loop_head);
 
+  // TODO: When `type == Primitive::kPrimNot`, add a read barrier for
+  // the reference stored in the object before attempting the CAS,
+  // similar to the one in the art::Unsafe_compareAndSwapObject JNI
+  // implementation.
+  //
+  // Note that this code is not (yet) used when read barriers are
+  // enabled (see IntrinsicLocationsBuilderARM::VisitUnsafeCASObject).
+  DCHECK(!(type == Primitive::kPrimNot && kEmitCompilerReadBarrier));
   __ ldrex(tmp_lo, tmp_ptr);
-  // TODO: Do we need a read barrier here when `type == Primitive::kPrimNot`?
 
   __ subs(tmp_lo, tmp_lo, ShifterOperand(expected_lo));
 
@@ -852,15 +859,17 @@ void IntrinsicLocationsBuilderARM::VisitUnsafeCASInt(HInvoke* invoke) {
   CreateIntIntIntIntIntToIntPlusTemps(arena_, invoke);
 }
 void IntrinsicLocationsBuilderARM::VisitUnsafeCASObject(HInvoke* invoke) {
-  // The UnsafeCASObject intrinsic does not always work when heap
-  // poisoning is enabled (it breaks run-test 004-UnsafeTest); turn it
-  // off temporarily as a quick fix.
+  // The UnsafeCASObject intrinsic is missing a read barrier, and
+  // therefore sometimes does not work as expected (b/25883050).
+  // Turn it off temporarily as a quick fix, until the read barrier is
+  // implemented (see TODO in GenCAS below).
   //
-  // TODO(rpl): Fix it and turn it back on.
+  // Also, the UnsafeCASObject intrinsic does not always work when heap
+  // poisoning is enabled (it breaks run-test 004-UnsafeTest); turn it
+  // off temporarily as a quick fix (b/26204023).
   //
-  // TODO(rpl): Also, we should investigate whether we need a read
-  // barrier in the generated code.
-  if (kPoisonHeapReferences) {
+  // TODO(rpl): Fix these two issues and re-enable this intrinsic.
+  if (kEmitCompilerReadBarrier || kPoisonHeapReferences) {
     return;
   }
 

diff --git a/compiler/optimizing/intrinsics_arm64.cc b/compiler/optimizing/intrinsics_arm64.cc
index 6b34daa..9f6863c 100644 (file)
--- a/compiler/optimizing/intrinsics_arm64.cc
+++ b/compiler/optimizing/intrinsics_arm64.cc
@@ -1031,10 +1031,15 @@ static void GenCas(LocationSummary* locations, Primitive::Type type, CodeGenerat
   } else {
     __ Dmb(InnerShareable, BarrierWrites);
     __ Bind(&loop_head);
-    __ Ldxr(tmp_value, MemOperand(tmp_ptr));
-    // TODO: Do we need a read barrier here when `type == Primitive::kPrimNot`?
+    // TODO: When `type == Primitive::kPrimNot`, add a read barrier for
+    // the reference stored in the object before attempting the CAS,
+    // similar to the one in the art::Unsafe_compareAndSwapObject JNI
+    // implementation.
+    //
     // Note that this code is not (yet) used when read barriers are
     // enabled (see IntrinsicLocationsBuilderARM64::VisitUnsafeCASObject).
+    DCHECK(!(type == Primitive::kPrimNot && kEmitCompilerReadBarrier));
+    __ Ldxr(tmp_value, MemOperand(tmp_ptr));
     __ Cmp(tmp_value, expected);
     __ B(&exit_loop, ne);
     __ Stxr(tmp_32, value, MemOperand(tmp_ptr));
@@ -1057,15 +1062,17 @@ void IntrinsicLocationsBuilderARM64::VisitUnsafeCASLong(HInvoke* invoke) {
   CreateIntIntIntIntIntToInt(arena_, invoke);
 }
 void IntrinsicLocationsBuilderARM64::VisitUnsafeCASObject(HInvoke* invoke) {
-  // The UnsafeCASObject intrinsic does not always work when heap
-  // poisoning is enabled (it breaks run-test 004-UnsafeTest); turn it
-  // off temporarily as a quick fix.
+  // The UnsafeCASObject intrinsic is missing a read barrier, and
+  // therefore sometimes does not work as expected (b/25883050).
+  // Turn it off temporarily as a quick fix, until the read barrier is
+  // implemented (see TODO in GenCAS below).
   //
-  // TODO(rpl): Fix it and turn it back on.
+  // Also, the UnsafeCASObject intrinsic does not always work when heap
+  // poisoning is enabled (it breaks run-test 004-UnsafeTest); turn it
+  // off temporarily as a quick fix (b/26204023).
   //
-  // TODO(rpl): Also, we should investigate whether we need a read
-  // barrier in the generated code.
-  if (kPoisonHeapReferences) {
+  // TODO(rpl): Fix these two issues and re-enable this intrinsic.
+  if (kEmitCompilerReadBarrier || kPoisonHeapReferences) {
     return;
   }
 

diff --git a/compiler/optimizing/intrinsics_mips64.cc b/compiler/optimizing/intrinsics_mips64.cc
index 8aa7d9f..8b45ea7 100644 (file)
--- a/compiler/optimizing/intrinsics_mips64.cc
+++ b/compiler/optimizing/intrinsics_mips64.cc
@@ -1299,6 +1299,8 @@ static void GenCas(LocationSummary* locations, Primitive::Type type, CodeGenerat
   if (type == Primitive::kPrimLong) {
     __ Lld(out, TMP);
   } else {
+    // Note: We will need a read barrier here, when read barrier
+    // support is added to the MIPS64 back end.
     __ Ll(out, TMP);
   }
   __ Dsubu(out, out, expected);         // If we didn't get the 'expected'

diff --git a/compiler/optimizing/intrinsics_x86.cc b/compiler/optimizing/intrinsics_x86.cc
index fd454d8..0d9adbf 100644 (file)
--- a/compiler/optimizing/intrinsics_x86.cc
+++ b/compiler/optimizing/intrinsics_x86.cc
@@ -2085,6 +2085,17 @@ void IntrinsicLocationsBuilderX86::VisitUnsafeCASLong(HInvoke* invoke) {
 }
 
 void IntrinsicLocationsBuilderX86::VisitUnsafeCASObject(HInvoke* invoke) {
+  // The UnsafeCASObject intrinsic is missing a read barrier, and
+  // therefore sometimes does not work as expected (b/25883050).
+  // Turn it off temporarily as a quick fix, until the read barrier is
+  // implemented.
+  //
+  // TODO(rpl): Implement a read barrier in GenCAS below and re-enable
+  // this intrinsic.
+  if (kEmitCompilerReadBarrier) {
+    return;
+  }
+
   CreateIntIntIntIntIntToInt(arena_, Primitive::kPrimNot, invoke);
 }
 
@@ -2136,6 +2147,13 @@ static void GenCAS(Primitive::Type type, HInvoke* invoke, CodeGeneratorX86* code
       __ PoisonHeapReference(value);
     }
 
+    // TODO: Add a read barrier for the reference stored in the object
+    // before attempting the CAS, similar to the one in the
+    // art::Unsafe_compareAndSwapObject JNI implementation.
+    //
+    // Note that this code is not (yet) used when read barriers are
+    // enabled (see IntrinsicLocationsBuilderX86::VisitUnsafeCASObject).
+    DCHECK(!kEmitCompilerReadBarrier);
     __ LockCmpxchgl(Address(base, offset, TIMES_1, 0), value);
 
     // LOCK CMPXCHG has full barrier semantics, and we don't need
@@ -2145,11 +2163,8 @@ static void GenCAS(Primitive::Type type, HInvoke* invoke, CodeGeneratorX86* code
     __ setb(kZero, out.AsRegister<Register>());
     __ movzxb(out.AsRegister<Register>(), out.AsRegister<ByteRegister>());
 
-    // In the case of the `UnsafeCASObject` intrinsic, accessing an
-    // object in the heap with LOCK CMPXCHG does not require a read
-    // barrier, as we do not keep a reference to this heap location.
-    // However, if heap poisoning is enabled, we need to unpoison the
-    // values that were poisoned earlier.
+    // If heap poisoning is enabled, we need to unpoison the values
+    // that were poisoned earlier.
     if (kPoisonHeapReferences) {
       if (base_equals_value) {
         // `value` has been moved to a temporary register, no need to

diff --git a/compiler/optimizing/intrinsics_x86_64.cc b/compiler/optimizing/intrinsics_x86_64.cc
index ce737e3..2986dc8 100644 (file)
--- a/compiler/optimizing/intrinsics_x86_64.cc
+++ b/compiler/optimizing/intrinsics_x86_64.cc
@@ -2150,6 +2150,17 @@ void IntrinsicLocationsBuilderX86_64::VisitUnsafeCASLong(HInvoke* invoke) {
 }
 
 void IntrinsicLocationsBuilderX86_64::VisitUnsafeCASObject(HInvoke* invoke) {
+  // The UnsafeCASObject intrinsic is missing a read barrier, and
+  // therefore sometimes does not work as expected (b/25883050).
+  // Turn it off temporarily as a quick fix, until the read barrier is
+  // implemented.
+  //
+  // TODO(rpl): Implement a read barrier in GenCAS below and re-enable
+  // this intrinsic.
+  if (kEmitCompilerReadBarrier) {
+    return;
+  }
+
   CreateIntIntIntIntIntToInt(arena_, Primitive::kPrimNot, invoke);
 }
 
@@ -2200,6 +2211,13 @@ static void GenCAS(Primitive::Type type, HInvoke* invoke, CodeGeneratorX86_64* c
       __ PoisonHeapReference(CpuRegister(value_reg));
     }
 
+    // TODO: Add a read barrier for the reference stored in the object
+    // before attempting the CAS, similar to the one in the
+    // art::Unsafe_compareAndSwapObject JNI implementation.
+    //
+    // Note that this code is not (yet) used when read barriers are
+    // enabled (see IntrinsicLocationsBuilderX86_64::VisitUnsafeCASObject).
+    DCHECK(!kEmitCompilerReadBarrier);
     __ LockCmpxchgl(Address(base, offset, TIMES_1, 0), CpuRegister(value_reg));
 
     // LOCK CMPXCHG has full barrier semantics, and we don't need
@@ -2209,11 +2227,8 @@ static void GenCAS(Primitive::Type type, HInvoke* invoke, CodeGeneratorX86_64* c
     __ setcc(kZero, out);
     __ movzxb(out, out);
 
-    // In the case of the `UnsafeCASObject` intrinsic, accessing an
-    // object in the heap with LOCK CMPXCHG does not require a read
-    // barrier, as we do not keep a reference to this heap location.
-    // However, if heap poisoning is enabled, we need to unpoison the
-    // values that were poisoned earlier.
+    // If heap poisoning is enabled, we need to unpoison the values
+    // that were poisoned earlier.
     if (kPoisonHeapReferences) {
       if (base_equals_value) {
         // `value_reg` has been moved to a temporary register, no need

diff --git a/tools/libcore_failures.txt b/tools/libcore_failures.txt
index a2d2f23..a5476f7 100644 (file)
--- a/tools/libcore_failures.txt
+++ b/tools/libcore_failures.txt
@@ -170,12 +170,5 @@
   result: EXEC_FAILED,
   names: ["org.apache.harmony.tests.java.util.WeakHashMapTest#test_keySet"],
   bug: 25437292
-},
-{
-  description: "JSR166TestCase.waitForThreadToEnterWaitState seems to time out; needs investigation.",
-  result: EXEC_FAILED,
-  names: ["jsr166.LinkedTransferQueueTest#testTransfer2",
-          "jsr166.LinkedTransferQueueTest#testWaitingConsumer"],
-  bug: 25883050
 }
 ]