Quick: Fix LVN/GVN handling of acquire operations.
Acquire operations, i.e. MONITOR_ENTER and volatile GETs,
change the thread's view of the memory, so subsequent loads
must get new value names in LVN/GVN. Release operations do
not affect this thread's view of the memory, they the only
push the modifications for other threads to see.
Bug: 17689750
Change-Id: I9442d89b1d2c5252b99b02851b71bb85f871d734
diff --git a/compiler/dex/local_value_numbering.cc b/compiler/dex/local_value_numbering.cc
index 4279955..eb0806b 100644
--- a/compiler/dex/local_value_numbering.cc
+++ b/compiler/dex/local_value_numbering.cc
@@ -1169,8 +1169,9 @@
const MirFieldInfo& field_info = gvn_->GetMirGraph()->GetIFieldLoweringInfo(mir);
uint16_t res;
if (!field_info.IsResolved() || field_info.IsVolatile()) {
- // Volatile fields always get a new memory version; field id is irrelevant.
// Unresolved fields may be volatile, so handle them as such to be safe.
+ HandleInvokeOrClInitOrAcquireOp(mir); // Volatile GETs have acquire semantics.
+ // Volatile fields always get a new memory version; field id is irrelevant.
// Use result s_reg - will be unique.
res = gvn_->LookupValue(kNoValue, mir->ssa_rep->defs[0], kNoValue, kNoValue);
} else {
@@ -1269,14 +1270,16 @@
uint16_t LocalValueNumbering::HandleSGet(MIR* mir, uint16_t opcode) {
const MirSFieldLoweringInfo& field_info = gvn_->GetMirGraph()->GetSFieldLoweringInfo(mir);
- if (!field_info.IsInitialized() && (mir->optimization_flags & MIR_IGNORE_CLINIT_CHECK) == 0) {
- // Class initialization can call arbitrary functions, we need to wipe aliasing values.
- HandleInvokeOrClInit(mir);
+ if (!field_info.IsResolved() || field_info.IsVolatile() ||
+ (!field_info.IsInitialized() && (mir->optimization_flags & MIR_IGNORE_CLINIT_CHECK) == 0)) {
+ // Volatile SGETs (and unresolved fields are potentially volatile) have acquire semantics
+ // and class initialization can call arbitrary functions, we need to wipe aliasing values.
+ HandleInvokeOrClInitOrAcquireOp(mir);
}
uint16_t res;
if (!field_info.IsResolved() || field_info.IsVolatile()) {
- // Volatile fields always get a new memory version; field id is irrelevant.
// Unresolved fields may be volatile, so handle them as such to be safe.
+ // Volatile fields always get a new memory version; field id is irrelevant.
// Use result s_reg - will be unique.
res = gvn_->LookupValue(kNoValue, mir->ssa_rep->defs[0], kNoValue, kNoValue);
} else {
@@ -1306,7 +1309,7 @@
const MirSFieldLoweringInfo& field_info = gvn_->GetMirGraph()->GetSFieldLoweringInfo(mir);
if (!field_info.IsInitialized() && (mir->optimization_flags & MIR_IGNORE_CLINIT_CHECK) == 0) {
// Class initialization can call arbitrary functions, we need to wipe aliasing values.
- HandleInvokeOrClInit(mir);
+ HandleInvokeOrClInitOrAcquireOp(mir);
}
uint16_t type = opcode - Instruction::SPUT;
if (!field_info.IsResolved()) {
@@ -1351,7 +1354,7 @@
}
}
-void LocalValueNumbering::HandleInvokeOrClInit(MIR* mir) {
+void LocalValueNumbering::HandleInvokeOrClInitOrAcquireOp(MIR* mir) {
// Use mir->offset as modifier; without elaborate inlining, it will be unique.
global_memory_version_ =
gvn_->LookupValue(kInvokeMemoryVersionBumpOp, 0u, 0u, mir->offset);
@@ -1404,9 +1407,7 @@
case Instruction::MONITOR_ENTER:
HandleNullCheck(mir, GetOperandValue(mir->ssa_rep->uses[0]));
- // NOTE: Keeping all aliasing values intact. Programs that rely on loads/stores of the
- // same non-volatile locations outside and inside a synchronized block being different
- // contain races that we cannot fix.
+ HandleInvokeOrClInitOrAcquireOp(mir); // Acquire operation.
break;
case Instruction::MONITOR_EXIT:
@@ -1468,7 +1469,7 @@
uint16_t reg = GetOperandValue(mir->ssa_rep->uses[i]);
non_aliasing_refs_.erase(reg);
}
- HandleInvokeOrClInit(mir);
+ HandleInvokeOrClInitOrAcquireOp(mir);
}
break;
diff --git a/compiler/dex/local_value_numbering.h b/compiler/dex/local_value_numbering.h
index e11c6e5..c60da32 100644
--- a/compiler/dex/local_value_numbering.h
+++ b/compiler/dex/local_value_numbering.h
@@ -308,7 +308,7 @@
uint16_t HandleSGet(MIR* mir, uint16_t opcode);
void HandleSPut(MIR* mir, uint16_t opcode);
void RemoveSFieldsForType(uint16_t type);
- void HandleInvokeOrClInit(MIR* mir);
+ void HandleInvokeOrClInitOrAcquireOp(MIR* mir);
bool SameMemoryVersion(const LocalValueNumbering& other) const;
diff --git a/compiler/dex/local_value_numbering_test.cc b/compiler/dex/local_value_numbering_test.cc
index e53c640..067bea2 100644
--- a/compiler/dex/local_value_numbering_test.cc
+++ b/compiler/dex/local_value_numbering_test.cc
@@ -338,16 +338,19 @@
DEF_IGET(Instruction::IGET, 1u, 0u, 0u), // Non-volatile.
DEF_IGET(Instruction::IGET, 2u, 10u, 1u), // Volatile.
DEF_IGET(Instruction::IGET, 3u, 2u, 1u), // Non-volatile.
+ DEF_IGET(Instruction::IGET, 4u, 0u, 0u), // Non-volatile.
};
PrepareIFields(ifields);
PrepareMIRs(mirs);
PerformLVN();
- ASSERT_EQ(value_names_.size(), 4u);
+ ASSERT_EQ(value_names_.size(), 5u);
EXPECT_NE(value_names_[0], value_names_[2]); // Volatile has always different value name.
EXPECT_NE(value_names_[1], value_names_[3]); // Used different base because of volatile.
+ EXPECT_NE(value_names_[1], value_names_[4]); // Not guaranteed to be the same after "acquire".
+
for (size_t i = 0; i != arraysize(mirs); ++i) {
- EXPECT_EQ((i == 2u) ? MIR_IGNORE_NULL_CHECK : 0,
+ EXPECT_EQ((i == 2u || i == 4u) ? MIR_IGNORE_NULL_CHECK : 0,
mirs_[i].optimization_flags) << i;
}
}
@@ -363,7 +366,7 @@
DEF_IGET(Instruction::IGET, 1u, 20u, 0u), // Resolved field #1, unique object.
DEF_IGET(Instruction::IGET, 2u, 21u, 0u), // Resolved field #1.
DEF_IGET_WIDE(Instruction::IGET_WIDE, 3u, 21u, 1u), // Resolved field #2.
- DEF_IGET(Instruction::IGET, 4u, 22u, 2u), // IGET doesn't clobber anything.
+ DEF_IGET(Instruction::IGET, 4u, 22u, 2u), // Unresolved IGET can be "acquire".
DEF_IGET(Instruction::IGET, 5u, 20u, 0u), // Resolved field #1, unique object.
DEF_IGET(Instruction::IGET, 6u, 21u, 0u), // Resolved field #1.
DEF_IGET_WIDE(Instruction::IGET_WIDE, 7u, 21u, 1u), // Resolved field #2.
@@ -381,14 +384,15 @@
PrepareMIRs(mirs);
PerformLVN();
ASSERT_EQ(value_names_.size(), 16u);
- EXPECT_EQ(value_names_[1], value_names_[5]);
- EXPECT_EQ(value_names_[2], value_names_[6]);
- EXPECT_EQ(value_names_[3], value_names_[7]);
- EXPECT_EQ(value_names_[1], value_names_[9]);
- EXPECT_NE(value_names_[2], value_names_[10]); // This aliased with unresolved IPUT.
- EXPECT_EQ(value_names_[3], value_names_[11]);
- EXPECT_EQ(value_names_[12], value_names_[15]);
- EXPECT_NE(value_names_[1], value_names_[14]); // This aliased with unresolved IPUT.
+ // Unresolved field is potentially volatile, so we need to adhere to the volatile semantics.
+ EXPECT_EQ(value_names_[1], value_names_[5]); // Unique object.
+ EXPECT_NE(value_names_[2], value_names_[6]); // Not guaranteed to be the same after "acquire".
+ EXPECT_NE(value_names_[3], value_names_[7]); // Not guaranteed to be the same after "acquire".
+ EXPECT_EQ(value_names_[1], value_names_[9]); // Unique object.
+ EXPECT_NE(value_names_[6], value_names_[10]); // This aliased with unresolved IPUT.
+ EXPECT_EQ(value_names_[7], value_names_[11]); // Still the same after "release".
+ EXPECT_EQ(value_names_[12], value_names_[15]); // Still the same after "release".
+ EXPECT_NE(value_names_[1], value_names_[14]); // This aliased with unresolved IPUT.
EXPECT_EQ(mirs_[0].optimization_flags, 0u);
EXPECT_EQ(mirs_[1].optimization_flags, MIR_IGNORE_NULL_CHECK);
EXPECT_EQ(mirs_[2].optimization_flags, 0u);
@@ -409,7 +413,7 @@
static const MIRDef mirs[] = {
DEF_SGET(Instruction::SGET, 0u, 0u), // Resolved field #1.
DEF_SGET_WIDE(Instruction::SGET_WIDE, 1u, 1u), // Resolved field #2.
- DEF_SGET(Instruction::SGET, 2u, 2u), // SGET doesn't clobber anything.
+ DEF_SGET(Instruction::SGET, 2u, 2u), // Unresolved SGET can be "acquire".
DEF_SGET(Instruction::SGET, 3u, 0u), // Resolved field #1.
DEF_SGET_WIDE(Instruction::SGET_WIDE, 4u, 1u), // Resolved field #2.
DEF_SPUT(Instruction::SPUT, 5u, 2u), // SPUT clobbers field #1 (#2 is wide).
@@ -421,10 +425,11 @@
PrepareMIRs(mirs);
PerformLVN();
ASSERT_EQ(value_names_.size(), 8u);
- EXPECT_EQ(value_names_[0], value_names_[3]);
- EXPECT_EQ(value_names_[1], value_names_[4]);
- EXPECT_NE(value_names_[0], value_names_[6]); // This aliased with unresolved IPUT.
- EXPECT_EQ(value_names_[1], value_names_[7]);
+ // Unresolved field is potentially volatile, so we need to adhere to the volatile semantics.
+ EXPECT_NE(value_names_[0], value_names_[3]); // Not guaranteed to be the same after "acquire".
+ EXPECT_NE(value_names_[1], value_names_[4]); // Not guaranteed to be the same after "acquire".
+ EXPECT_NE(value_names_[3], value_names_[6]); // This aliased with unresolved IPUT.
+ EXPECT_EQ(value_names_[4], value_names_[7]); // Still the same after "release".
for (size_t i = 0u; i != mir_count_; ++i) {
EXPECT_EQ(0, mirs_[i].optimization_flags) << i;
}
diff --git a/test/123-compiler-regressions-mt/expected.txt b/test/123-compiler-regressions-mt/expected.txt
new file mode 100644
index 0000000..a11e5bf
--- /dev/null
+++ b/test/123-compiler-regressions-mt/expected.txt
@@ -0,0 +1,2 @@
+b17689750TestVolatile passed.
+b17689750TestMonitor passed.
diff --git a/test/123-compiler-regressions-mt/info.txt b/test/123-compiler-regressions-mt/info.txt
new file mode 100644
index 0000000..cac7e75
--- /dev/null
+++ b/test/123-compiler-regressions-mt/info.txt
@@ -0,0 +1,6 @@
+This is a test for bad optimizations affecting multi-threaded program
+behavior.
+
+This test covers fixed AOT/JIT bugs to prevent regressions.
+
+17689750 GVN assigns the same value names across MONITOR_ENTER and volatile reads.
diff --git a/test/123-compiler-regressions-mt/src/Main.java b/test/123-compiler-regressions-mt/src/Main.java
new file mode 100644
index 0000000..11fa021
--- /dev/null
+++ b/test/123-compiler-regressions-mt/src/Main.java
@@ -0,0 +1,117 @@
+/*
+ * Copyright (C) 2009 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import java.util.concurrent.*;
+import java.util.concurrent.atomic.AtomicLong;
+
+/**
+ * Test for Jit regressions.
+ */
+public class Main {
+ public static void main(String args[]) throws Exception {
+ b17689750TestVolatile();
+ b17689750TestMonitor();
+ }
+
+ static void b17689750TestVolatile() {
+ final B17689750TestVolatile test = new B17689750TestVolatile();
+ new Thread() {
+ public void run() {
+ test.thread1();
+ }
+ }.start();
+ try {
+ test.thread2();
+ } catch (NullPointerException expected) {
+ System.out.println("b17689750TestVolatile passed.");
+ }
+ }
+
+ static void b17689750TestMonitor() {
+ final B17689750TestMonitor test = new B17689750TestMonitor();
+ new Thread() {
+ public void run() {
+ test.thread1();
+ }
+ }.start();
+ try {
+ test.thread2();
+ } catch (NullPointerException expected) {
+ System.out.println("b17689750TestMonitor passed.");
+ }
+ }
+}
+
+class B17689750TestVolatile {
+ private volatile int state = 0;
+ private int[] values = { 42 };
+
+ void thread1() {
+ while (state != 1) { } // Busy loop.
+ values = null;
+ state = 2;
+ }
+
+ void thread2() {
+ int[] vs1 = values;
+ state = 1;
+ while (state != 2) { } // Busy loop.
+ int[] vs2 = values;
+ int v1 = vs1[0];
+ int v2 = vs2[0];
+ System.out.println("b17689750TestVolatile failed: " + v1 + ", " + v2);
+ }
+}
+
+class B17689750TestMonitor {
+ private int state = 0;
+ private Object lock = new Object();
+ private int[] values = { 42 };
+
+ void thread1() {
+ int s;
+ do {
+ synchronized (lock) {
+ s = state;
+ }
+ } while (s != 1); // Busy loop.
+
+ synchronized (lock) {
+ values = null;
+ state = 2;
+ }
+ }
+
+ void thread2() {
+ int[] vs1;
+ synchronized (lock) {
+ vs1 = values;
+ state = 1;
+ }
+
+ int s;
+ do {
+ synchronized (lock) {
+ s = state;
+ }
+ } while (s != 2); // Busy loop.
+
+ int[] vs2 = values;
+ int v1 = vs1[0];
+ int v2 = vs2[0];
+ System.out.println("b17689750TestMonitor failed: " + v1 + ", " + v2);
+ }
+}
