Fix heap poisoning in UnsafeCASObject x86/x86-64 intrinsic.
Properly handle the case when the same object is passed to
sun.misc.Unsafe.compareAndSwapObject for the `obj` and
`newValue` arguments (named `base` and `value` in the
intrinsic implementation) and re-enable this intrinsic.
Also convert some reinterpret_casts to down_casts.
Bug: 12687968
Change-Id: I82167cfa77840ae2cdb45b9f19f5f530858fe7e8
diff --git a/compiler/optimizing/intrinsics_x86.cc b/compiler/optimizing/intrinsics_x86.cc
index 8a7aded..040bf6a 100644
--- a/compiler/optimizing/intrinsics_x86.cc
+++ b/compiler/optimizing/intrinsics_x86.cc
@@ -45,7 +45,7 @@
X86Assembler* IntrinsicCodeGeneratorX86::GetAssembler() {
- return reinterpret_cast<X86Assembler*>(codegen_->GetAssembler());
+ return down_cast<X86Assembler*>(codegen_->GetAssembler());
}
ArenaAllocator* IntrinsicCodeGeneratorX86::GetAllocator() {
@@ -1728,7 +1728,7 @@
Primitive::Type type,
bool is_volatile,
CodeGeneratorX86* codegen) {
- X86Assembler* assembler = reinterpret_cast<X86Assembler*>(codegen->GetAssembler());
+ X86Assembler* assembler = down_cast<X86Assembler*>(codegen->GetAssembler());
Register base = locations->InAt(1).AsRegister<Register>();
Register offset = locations->InAt(2).AsRegisterPairLow<Register>();
Location value_loc = locations->InAt(3);
@@ -1822,7 +1822,7 @@
locations->SetOut(Location::RegisterLocation(EAX));
if (type == Primitive::kPrimNot) {
// Need temp registers for card-marking.
- locations->AddTemp(Location::RequiresRegister());
+ locations->AddTemp(Location::RequiresRegister()); // Possibly used for reference poisoning too.
// Need a byte register for marking.
locations->AddTemp(Location::RegisterLocation(ECX));
}
@@ -1837,20 +1837,11 @@
}
void IntrinsicLocationsBuilderX86::VisitUnsafeCASObject(HInvoke* invoke) {
- // The UnsafeCASObject intrinsic does not always work when heap
- // poisoning is enabled (it breaks several libcore tests); turn it
- // off temporarily as a quick fix.
- // TODO(rpl): Fix it and turn it back on.
- if (kPoisonHeapReferences) {
- return;
- }
-
CreateIntIntIntIntIntToInt(arena_, Primitive::kPrimNot, invoke);
}
static void GenCAS(Primitive::Type type, HInvoke* invoke, CodeGeneratorX86* codegen) {
- X86Assembler* assembler =
- reinterpret_cast<X86Assembler*>(codegen->GetAssembler());
+ X86Assembler* assembler = down_cast<X86Assembler*>(codegen->GetAssembler());
LocationSummary* locations = invoke->GetLocations();
Register base = locations->InAt(1).AsRegister<Register>();
@@ -1858,47 +1849,92 @@
Location out = locations->Out();
DCHECK_EQ(out.AsRegister<Register>(), EAX);
- if (type == Primitive::kPrimLong) {
- DCHECK_EQ(locations->InAt(3).AsRegisterPairLow<Register>(), EAX);
- DCHECK_EQ(locations->InAt(3).AsRegisterPairHigh<Register>(), EDX);
- DCHECK_EQ(locations->InAt(4).AsRegisterPairLow<Register>(), EBX);
- DCHECK_EQ(locations->InAt(4).AsRegisterPairHigh<Register>(), ECX);
- __ LockCmpxchg8b(Address(base, offset, TIMES_1, 0));
- } else {
- // Integer or object.
+ if (type == Primitive::kPrimNot) {
Register expected = locations->InAt(3).AsRegister<Register>();
+ // Ensure `expected` is in EAX (required by the CMPXCHG instruction).
DCHECK_EQ(expected, EAX);
Register value = locations->InAt(4).AsRegister<Register>();
- if (type == Primitive::kPrimNot) {
- // Mark card for object assuming new value is stored.
- bool value_can_be_null = true; // TODO: Worth finding out this information?
- codegen->MarkGCCard(locations->GetTemp(0).AsRegister<Register>(),
- locations->GetTemp(1).AsRegister<Register>(),
- base,
- value,
- value_can_be_null);
- if (kPoisonHeapReferences) {
- __ PoisonHeapReference(expected);
- __ PoisonHeapReference(value);
+ // Mark card for object assuming new value is stored.
+ bool value_can_be_null = true; // TODO: Worth finding out this information?
+ codegen->MarkGCCard(locations->GetTemp(0).AsRegister<Register>(),
+ locations->GetTemp(1).AsRegister<Register>(),
+ base,
+ value,
+ value_can_be_null);
+
+ bool base_equals_value = (base == value);
+ if (kPoisonHeapReferences) {
+ if (base_equals_value) {
+ // If `base` and `value` are the same register location, move
+ // `value` to a temporary register. This way, poisoning
+ // `value` won't invalidate `base`.
+ value = locations->GetTemp(0).AsRegister<Register>();
+ __ movl(value, base);
}
+
+ // Check that the register allocator did not assign the location
+ // of `expected` (EAX) to `value` nor to `base`, so that heap
+ // poisoning (when enabled) works as intended below.
+ // - If `value` were equal to `expected`, both references would
+ // be poisoned twice, meaning they would not be poisoned at
+ // all, as heap poisoning uses address negation.
+ // - If `base` were equal to `expected`, poisoning `expected`
+ // would invalidate `base`.
+ DCHECK_NE(value, expected);
+ DCHECK_NE(base, expected);
+
+ __ PoisonHeapReference(expected);
+ __ PoisonHeapReference(value);
}
__ LockCmpxchgl(Address(base, offset, TIMES_1, 0), value);
- }
- // locked cmpxchg has full barrier semantics, and we don't need scheduling
- // barriers at this time.
+ // locked cmpxchg has full barrier semantics, and we don't need
+ // scheduling barriers at this time.
- // Convert ZF into the boolean result.
- __ setb(kZero, out.AsRegister<Register>());
- __ movzxb(out.AsRegister<Register>(), out.AsRegister<ByteRegister>());
+ // Convert ZF into the boolean result.
+ __ setb(kZero, out.AsRegister<Register>());
+ __ movzxb(out.AsRegister<Register>(), out.AsRegister<ByteRegister>());
- if (kPoisonHeapReferences && type == Primitive::kPrimNot) {
- Register value = locations->InAt(4).AsRegister<Register>();
- __ UnpoisonHeapReference(value);
- // Do not unpoison the reference contained in register `expected`,
- // as it is the same as register `out`.
+ if (kPoisonHeapReferences) {
+ if (base_equals_value) {
+ // `value` has been moved to a temporary register, no need to
+ // unpoison it.
+ } else {
+ // Ensure `value` is different from `out`, so that unpoisoning
+ // the former does not invalidate the latter.
+ DCHECK_NE(value, out.AsRegister<Register>());
+ __ UnpoisonHeapReference(value);
+ }
+ // Do not unpoison the reference contained in register
+ // `expected`, as it is the same as register `out` (EAX).
+ }
+ } else {
+ if (type == Primitive::kPrimInt) {
+ // Ensure the expected value is in EAX (required by the CMPXCHG
+ // instruction).
+ DCHECK_EQ(locations->InAt(3).AsRegister<Register>(), EAX);
+ __ LockCmpxchgl(Address(base, offset, TIMES_1, 0),
+ locations->InAt(4).AsRegister<Register>());
+ } else if (type == Primitive::kPrimLong) {
+ // Ensure the expected value is in EAX:EDX and that the new
+ // value is in EBX:ECX (required by the CMPXCHG8B instruction).
+ DCHECK_EQ(locations->InAt(3).AsRegisterPairLow<Register>(), EAX);
+ DCHECK_EQ(locations->InAt(3).AsRegisterPairHigh<Register>(), EDX);
+ DCHECK_EQ(locations->InAt(4).AsRegisterPairLow<Register>(), EBX);
+ DCHECK_EQ(locations->InAt(4).AsRegisterPairHigh<Register>(), ECX);
+ __ LockCmpxchg8b(Address(base, offset, TIMES_1, 0));
+ } else {
+ LOG(FATAL) << "Unexpected CAS type " << type;
+ }
+
+ // locked cmpxchg has full barrier semantics, and we don't need
+ // scheduling barriers at this time.
+
+ // Convert ZF into the boolean result.
+ __ setb(kZero, out.AsRegister<Register>());
+ __ movzxb(out.AsRegister<Register>(), out.AsRegister<ByteRegister>());
}
}
@@ -1936,8 +1972,7 @@
}
void IntrinsicCodeGeneratorX86::VisitIntegerReverse(HInvoke* invoke) {
- X86Assembler* assembler =
- reinterpret_cast<X86Assembler*>(codegen_->GetAssembler());
+ X86Assembler* assembler = down_cast<X86Assembler*>(codegen_->GetAssembler());
LocationSummary* locations = invoke->GetLocations();
Register reg = locations->InAt(0).AsRegister<Register>();
@@ -1968,8 +2003,7 @@
}
void IntrinsicCodeGeneratorX86::VisitLongReverse(HInvoke* invoke) {
- X86Assembler* assembler =
- reinterpret_cast<X86Assembler*>(codegen_->GetAssembler());
+ X86Assembler* assembler = down_cast<X86Assembler*>(codegen_->GetAssembler());
LocationSummary* locations = invoke->GetLocations();
Register reg_low = locations->InAt(0).AsRegisterPairLow<Register>();
diff --git a/compiler/optimizing/intrinsics_x86_64.cc b/compiler/optimizing/intrinsics_x86_64.cc
index 7a1d92d..14c65c9 100644
--- a/compiler/optimizing/intrinsics_x86_64.cc
+++ b/compiler/optimizing/intrinsics_x86_64.cc
@@ -41,7 +41,7 @@
X86_64Assembler* IntrinsicCodeGeneratorX86_64::GetAssembler() {
- return reinterpret_cast<X86_64Assembler*>(codegen_->GetAssembler());
+ return down_cast<X86_64Assembler*>(codegen_->GetAssembler());
}
ArenaAllocator* IntrinsicCodeGeneratorX86_64::GetAllocator() {
@@ -1822,7 +1822,7 @@
// memory model.
static void GenUnsafePut(LocationSummary* locations, Primitive::Type type, bool is_volatile,
CodeGeneratorX86_64* codegen) {
- X86_64Assembler* assembler = reinterpret_cast<X86_64Assembler*>(codegen->GetAssembler());
+ X86_64Assembler* assembler = down_cast<X86_64Assembler*>(codegen->GetAssembler());
CpuRegister base = locations->InAt(1).AsRegister<CpuRegister>();
CpuRegister offset = locations->InAt(2).AsRegister<CpuRegister>();
CpuRegister value = locations->InAt(3).AsRegister<CpuRegister>();
@@ -1895,7 +1895,7 @@
locations->SetOut(Location::RequiresRegister());
if (type == Primitive::kPrimNot) {
// Need temp registers for card-marking.
- locations->AddTemp(Location::RequiresRegister());
+ locations->AddTemp(Location::RequiresRegister()); // Possibly used for reference poisoning too.
locations->AddTemp(Location::RequiresRegister());
}
}
@@ -1909,61 +1909,95 @@
}
void IntrinsicLocationsBuilderX86_64::VisitUnsafeCASObject(HInvoke* invoke) {
- // The UnsafeCASObject intrinsic does not always work when heap
- // poisoning is enabled (it breaks several libcore tests); turn it
- // off temporarily as a quick fix.
- // TODO(rpl): Fix it and turn it back on.
- if (kPoisonHeapReferences) {
- return;
- }
-
CreateIntIntIntIntIntToInt(arena_, Primitive::kPrimNot, invoke);
}
static void GenCAS(Primitive::Type type, HInvoke* invoke, CodeGeneratorX86_64* codegen) {
- X86_64Assembler* assembler =
- reinterpret_cast<X86_64Assembler*>(codegen->GetAssembler());
+ X86_64Assembler* assembler = down_cast<X86_64Assembler*>(codegen->GetAssembler());
LocationSummary* locations = invoke->GetLocations();
CpuRegister base = locations->InAt(1).AsRegister<CpuRegister>();
CpuRegister offset = locations->InAt(2).AsRegister<CpuRegister>();
CpuRegister expected = locations->InAt(3).AsRegister<CpuRegister>();
+ // Ensure `expected` is in RAX (required by the CMPXCHG instruction).
DCHECK_EQ(expected.AsRegister(), RAX);
CpuRegister value = locations->InAt(4).AsRegister<CpuRegister>();
CpuRegister out = locations->Out().AsRegister<CpuRegister>();
- if (type == Primitive::kPrimLong) {
- __ LockCmpxchgq(Address(base, offset, TIMES_1, 0), value);
- } else {
- // Integer or object.
- if (type == Primitive::kPrimNot) {
- // Mark card for object assuming new value is stored.
- bool value_can_be_null = true; // TODO: Worth finding out this information?
- codegen->MarkGCCard(locations->GetTemp(0).AsRegister<CpuRegister>(),
- locations->GetTemp(1).AsRegister<CpuRegister>(),
- base,
- value,
- value_can_be_null);
+ if (type == Primitive::kPrimNot) {
+ // Mark card for object assuming new value is stored.
+ bool value_can_be_null = true; // TODO: Worth finding out this information?
+ codegen->MarkGCCard(locations->GetTemp(0).AsRegister<CpuRegister>(),
+ locations->GetTemp(1).AsRegister<CpuRegister>(),
+ base,
+ value,
+ value_can_be_null);
- if (kPoisonHeapReferences) {
- __ PoisonHeapReference(expected);
- __ PoisonHeapReference(value);
+ bool base_equals_value = (base.AsRegister() == value.AsRegister());
+ Register value_reg = value.AsRegister();
+ if (kPoisonHeapReferences) {
+ if (base_equals_value) {
+ // If `base` and `value` are the same register location, move
+ // `value_reg` to a temporary register. This way, poisoning
+ // `value_reg` won't invalidate `base`.
+ value_reg = locations->GetTemp(0).AsRegister<CpuRegister>().AsRegister();
+ __ movl(CpuRegister(value_reg), base);
}
+
+ // Check that the register allocator did not assign the location
+ // of `expected` (RAX) to `value` nor to `base`, so that heap
+ // poisoning (when enabled) works as intended below.
+ // - If `value` were equal to `expected`, both references would
+ // be poisoned twice, meaning they would not be poisoned at
+ // all, as heap poisoning uses address negation.
+ // - If `base` were equal to `expected`, poisoning `expected`
+ // would invalidate `base`.
+ DCHECK_NE(value_reg, expected.AsRegister());
+ DCHECK_NE(base.AsRegister(), expected.AsRegister());
+
+ __ PoisonHeapReference(expected);
+ __ PoisonHeapReference(CpuRegister(value_reg));
}
- __ LockCmpxchgl(Address(base, offset, TIMES_1, 0), value);
- }
+ __ LockCmpxchgl(Address(base, offset, TIMES_1, 0), CpuRegister(value_reg));
- // locked cmpxchg has full barrier semantics, and we don't need scheduling
- // barriers at this time.
+ // locked cmpxchg has full barrier semantics, and we don't need
+ // scheduling barriers at this time.
- // Convert ZF into the boolean result.
- __ setcc(kZero, out);
- __ movzxb(out, out);
+ // Convert ZF into the boolean result.
+ __ setcc(kZero, out);
+ __ movzxb(out, out);
- if (kPoisonHeapReferences && type == Primitive::kPrimNot) {
- __ UnpoisonHeapReference(value);
- __ UnpoisonHeapReference(expected);
+ if (kPoisonHeapReferences) {
+ if (base_equals_value) {
+ // `value_reg` has been moved to a temporary register, no need
+ // to unpoison it.
+ } else {
+ // Ensure `value` is different from `out`, so that unpoisoning
+ // the former does not invalidate the latter.
+ DCHECK_NE(value_reg, out.AsRegister());
+ __ UnpoisonHeapReference(CpuRegister(value_reg));
+ }
+ // Ensure `expected` is different from `out`, so that unpoisoning
+ // the former does not invalidate the latter.
+ DCHECK_NE(expected.AsRegister(), out.AsRegister());
+ __ UnpoisonHeapReference(expected);
+ }
+ } else {
+ if (type == Primitive::kPrimInt) {
+ __ LockCmpxchgl(Address(base, offset, TIMES_1, 0), value);
+ } else if (type == Primitive::kPrimLong) {
+ __ LockCmpxchgq(Address(base, offset, TIMES_1, 0), value);
+ } else {
+ LOG(FATAL) << "Unexpected CAS type " << type;
+ }
+
+ // locked cmpxchg has full barrier semantics, and we don't need
+ // scheduling barriers at this time.
+
+ // Convert ZF into the boolean result.
+ __ setcc(kZero, out);
+ __ movzxb(out, out);
}
}
@@ -2001,8 +2035,7 @@
}
void IntrinsicCodeGeneratorX86_64::VisitIntegerReverse(HInvoke* invoke) {
- X86_64Assembler* assembler =
- reinterpret_cast<X86_64Assembler*>(codegen_->GetAssembler());
+ X86_64Assembler* assembler = down_cast<X86_64Assembler*>(codegen_->GetAssembler());
LocationSummary* locations = invoke->GetLocations();
CpuRegister reg = locations->InAt(0).AsRegister<CpuRegister>();
@@ -2046,8 +2079,7 @@
}
void IntrinsicCodeGeneratorX86_64::VisitLongReverse(HInvoke* invoke) {
- X86_64Assembler* assembler =
- reinterpret_cast<X86_64Assembler*>(codegen_->GetAssembler());
+ X86_64Assembler* assembler = down_cast<X86_64Assembler*>(codegen_->GetAssembler());
LocationSummary* locations = invoke->GetLocations();
CpuRegister reg = locations->InAt(0).AsRegister<CpuRegister>();
diff --git a/test/004-UnsafeTest/src/Main.java b/test/004-UnsafeTest/src/Main.java
index c93db50..5b22e88 100644
--- a/test/004-UnsafeTest/src/Main.java
+++ b/test/004-UnsafeTest/src/Main.java
@@ -129,13 +129,36 @@
System.out.println("Unexpectedly not succeeding compareAndSwapLong...");
}
- if (unsafe.compareAndSwapObject(t, objectOffset, null, new Object())) {
+ // We do not use `null` as argument to sun.misc.Unsafe.compareAndSwapObject
+ // in those tests, as this value is not affected by heap poisoning
+ // (which uses address negation to poison and unpoison heap object
+ // references). This way, when heap poisoning is enabled, we can
+ // better exercise its implementation within that method.
+ if (unsafe.compareAndSwapObject(t, objectOffset, new Object(), new Object())) {
System.out.println("Unexpectedly succeeding compareAndSwapObject...");
}
- if (!unsafe.compareAndSwapObject(t, objectOffset, objectValue, null)) {
+ Object objectValue2 = new Object();
+ if (!unsafe.compareAndSwapObject(t, objectOffset, objectValue, objectValue2)) {
System.out.println("Unexpectedly not succeeding compareAndSwapObject...");
}
- if (!unsafe.compareAndSwapObject(t, objectOffset, null, new Object())) {
+ Object objectValue3 = new Object();
+ if (!unsafe.compareAndSwapObject(t, objectOffset, objectValue2, objectValue3)) {
+ System.out.println("Unexpectedly not succeeding compareAndSwapObject...");
+ }
+
+ // Exercise sun.misc.Unsafe.compareAndSwapObject using the same
+ // object (`t`) for the `obj` and `newValue` arguments.
+ if (!unsafe.compareAndSwapObject(t, objectOffset, objectValue3, t)) {
+ System.out.println("Unexpectedly not succeeding compareAndSwapObject...");
+ }
+ // Exercise sun.misc.Unsafe.compareAndSwapObject using the same
+ // object (`t`) for the `obj`, `expectedValue` and `newValue` arguments.
+ if (!unsafe.compareAndSwapObject(t, objectOffset, t, t)) {
+ System.out.println("Unexpectedly not succeeding compareAndSwapObject...");
+ }
+ // Exercise sun.misc.Unsafe.compareAndSwapObject using the same
+ // object (`t`) for the `obj` and `expectedValue` arguments.
+ if (!unsafe.compareAndSwapObject(t, objectOffset, t, new Object())) {
System.out.println("Unexpectedly not succeeding compareAndSwapObject...");
}
}
