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
--- a/compiler/optimizing/intrinsics_arm.cc
+++ b/compiler/optimizing/intrinsics_arm.cc
@@ -825,8 +825,15 @@
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 @@
CreateIntIntIntIntIntToIntPlusTemps(arena_, invoke);
}
void IntrinsicLocationsBuilderARM::VisitUnsafeCASObject(HInvoke* invoke) {
- // The UnsafeCASObject intrinsic does not always work when heap
+ // 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).
+ //
+ // 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.
+ // off temporarily as a quick fix (b/26204023).
//
- // TODO(rpl): Fix it and turn it back on.
- //
- // 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
--- a/compiler/optimizing/intrinsics_arm64.cc
+++ b/compiler/optimizing/intrinsics_arm64.cc
@@ -1031,10 +1031,15 @@
} 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 @@
CreateIntIntIntIntIntToInt(arena_, invoke);
}
void IntrinsicLocationsBuilderARM64::VisitUnsafeCASObject(HInvoke* invoke) {
- // The UnsafeCASObject intrinsic does not always work when heap
+ // 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).
+ //
+ // 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.
+ // off temporarily as a quick fix (b/26204023).
//
- // TODO(rpl): Fix it and turn it back on.
- //
- // 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
--- a/compiler/optimizing/intrinsics_mips64.cc
+++ b/compiler/optimizing/intrinsics_mips64.cc
@@ -1299,6 +1299,8 @@
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
--- a/compiler/optimizing/intrinsics_x86.cc
+++ b/compiler/optimizing/intrinsics_x86.cc
@@ -2085,6 +2085,17 @@
}
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 @@
__ 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 @@
__ 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
--- a/compiler/optimizing/intrinsics_x86_64.cc
+++ b/compiler/optimizing/intrinsics_x86_64.cc
@@ -2150,6 +2150,17 @@
}
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 @@
__ 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 @@
__ 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
--- 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
}
]