From 391b866ce55b8e78b1f9a6b98321d837256e8d66 Mon Sep 17 00:00:00 2001 From: Roland Levillain Date: Fri, 18 Dec 2015 11:43:38 +0000 Subject: [PATCH] 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 --- compiler/optimizing/intrinsics_arm.cc | 25 +++++++++++++++++-------- compiler/optimizing/intrinsics_arm64.cc | 25 ++++++++++++++++--------- compiler/optimizing/intrinsics_mips64.cc | 2 ++ compiler/optimizing/intrinsics_x86.cc | 25 ++++++++++++++++++++----- compiler/optimizing/intrinsics_x86_64.cc | 25 ++++++++++++++++++++----- tools/libcore_failures.txt | 7 ------- 6 files changed, 75 insertions(+), 34 deletions(-) diff --git a/compiler/optimizing/intrinsics_arm.cc b/compiler/optimizing/intrinsics_arm.cc index e8181bbb0..4683aee60 100644 --- 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 6b34daadf..9f6863cf6 100644 --- 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 8aa7d9ff6..8b45ea7c4 100644 --- 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 fd454d832..0d9adbff6 100644 --- 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()); __ movzxb(out.AsRegister(), out.AsRegister()); - // 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 ce737e3f7..2986dc884 100644 --- 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 a2d2f239d..a5476f7c4 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 } ] -- 2.11.0