From: Vladimir Marko Date: Wed, 9 Jul 2014 13:45:36 +0000 (+0100) Subject: Handle potential () correctly in LVN. X-Git-Tag: android-x86-7.1-r1~889^2~3631^2 X-Git-Url: http://git.osdn.net/view?a=commitdiff_plain;h=f418f3227e0001c8d75257ceff0c248cc406d81a;p=android-x86%2Fart.git Handle potential () correctly in LVN. Bug: 16177324 Change-Id: I727ab6ce9aa9a608fe570cf391a6b732a12a8655 --- diff --git a/compiler/dex/local_value_numbering.cc b/compiler/dex/local_value_numbering.cc index 62594963f..69a94a54c 100644 --- a/compiler/dex/local_value_numbering.cc +++ b/compiler/dex/local_value_numbering.cc @@ -356,7 +356,11 @@ void LocalValueNumbering::HandleIPut(MIR* mir, uint16_t opcode) { } uint16_t LocalValueNumbering::HandleSGet(MIR* mir, uint16_t opcode) { - const MirFieldInfo& field_info = cu_->mir_graph->GetSFieldLoweringInfo(mir); + const MirSFieldLoweringInfo& field_info = cu_->mir_graph->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); + } uint16_t res; if (!field_info.IsResolved() || field_info.IsVolatile()) { // Volatile fields always get a new memory version; field id is irrelevant. @@ -381,8 +385,12 @@ uint16_t LocalValueNumbering::HandleSGet(MIR* mir, uint16_t opcode) { } void LocalValueNumbering::HandleSPut(MIR* mir, uint16_t opcode) { + const MirSFieldLoweringInfo& field_info = cu_->mir_graph->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); + } uint16_t type = opcode - Instruction::SPUT; - const MirFieldInfo& field_info = cu_->mir_graph->GetSFieldLoweringInfo(mir); if (!field_info.IsResolved()) { // Unresolved fields always alias with everything of the same type. // Use mir->offset as modifier; without elaborate inlining, it will be unique. @@ -405,6 +413,15 @@ void LocalValueNumbering::HandleSPut(MIR* mir, uint16_t opcode) { } } +void LocalValueNumbering::HandleInvokeOrClInit(MIR* mir) { + // Use mir->offset as modifier; without elaborate inlining, it will be unique. + global_memory_version_ = LookupValue(kInvokeMemoryVersionBumpOp, 0u, 0u, mir->offset); + // All fields of escaped references need to be treated as potentially modified. + non_aliasing_ifields_.clear(); + // Array elements may also have been modified via escaped array refs. + escaped_array_refs_.clear(); +} + uint16_t LocalValueNumbering::GetValueNumber(MIR* mir) { uint16_t res = kNoValue; uint16_t opcode = mir->dalvikInsn.opcode; @@ -475,17 +492,12 @@ uint16_t LocalValueNumbering::GetValueNumber(MIR* mir) { case Instruction::INVOKE_STATIC: case Instruction::INVOKE_STATIC_RANGE: if ((mir->optimization_flags & MIR_INLINED) == 0) { - // Use mir->offset as modifier; without elaborate inlining, it will be unique. - global_memory_version_ = LookupValue(kInvokeMemoryVersionBumpOp, 0u, 0u, mir->offset); // Make ref args aliasing. for (size_t i = 0u, count = mir->ssa_rep->num_uses; i != count; ++i) { uint16_t reg = GetOperandValue(mir->ssa_rep->uses[i]); non_aliasing_refs_.erase(reg); } - // All fields of escaped references need to be treated as potentially modified. - non_aliasing_ifields_.clear(); - // Array elements may also have been modified via escaped array refs. - escaped_array_refs_.clear(); + HandleInvokeOrClInit(mir); } break; diff --git a/compiler/dex/local_value_numbering.h b/compiler/dex/local_value_numbering.h index 2a815be1c..45700df79 100644 --- a/compiler/dex/local_value_numbering.h +++ b/compiler/dex/local_value_numbering.h @@ -269,6 +269,7 @@ class LocalValueNumbering { void HandleIPut(MIR* mir, uint16_t opcode); uint16_t HandleSGet(MIR* mir, uint16_t opcode); void HandleSPut(MIR* mir, uint16_t opcode); + void HandleInvokeOrClInit(MIR* mir); CompilationUnit* const cu_; diff --git a/compiler/dex/local_value_numbering_test.cc b/compiler/dex/local_value_numbering_test.cc index efc4fc8a3..a2e3f3daf 100644 --- a/compiler/dex/local_value_numbering_test.cc +++ b/compiler/dex/local_value_numbering_test.cc @@ -113,11 +113,13 @@ class LocalValueNumberingTest : public testing::Test { for (size_t i = 0u; i != count; ++i) { const SFieldDef* def = &defs[i]; MirSFieldLoweringInfo field_info(def->field_idx); + // Mark even unresolved fields as initialized. + field_info.flags_ = MirSFieldLoweringInfo::kFlagIsStatic | + MirSFieldLoweringInfo::kFlagIsInitialized; if (def->declaring_dex_file != 0u) { field_info.declaring_dex_file_ = reinterpret_cast(def->declaring_dex_file); field_info.declaring_field_idx_ = def->declaring_field_idx; - field_info.flags_ = MirSFieldLoweringInfo::kFlagIsStatic | - (def->is_volatile ? MirSFieldLoweringInfo::kFlagIsVolatile : 0u); + field_info.flags_ |= (def->is_volatile ? MirSFieldLoweringInfo::kFlagIsVolatile : 0u); } cu_.mir_graph->sfield_lowering_infos_.Insert(field_info); } @@ -168,6 +170,12 @@ class LocalValueNumberingTest : public testing::Test { DoPrepareMIRs(defs, count); } + void MakeSFieldUninitialized(uint32_t sfield_index) { + CHECK_LT(sfield_index, cu_.mir_graph->sfield_lowering_infos_.Size()); + cu_.mir_graph->sfield_lowering_infos_.GetRawStorage()[sfield_index].flags_ &= + ~MirSFieldLoweringInfo::kFlagIsInitialized; + } + void PerformLVN() { value_names_.resize(mir_count_); for (size_t i = 0; i != mir_count_; ++i) { @@ -597,4 +605,25 @@ TEST_F(LocalValueNumberingTest, StoringSameValueKeepsMemoryVersion) { } } +TEST_F(LocalValueNumberingTest, ClInitOnSget) { + static const SFieldDef sfields[] = { + { 0u, 1u, 0u, false }, + { 1u, 2u, 1u, false }, + }; + static const MIRDef mirs[] = { + DEF_SGET(Instruction::SGET_OBJECT, 0u, 0u), + DEF_AGET(Instruction::AGET, 1u, 0u, 100u), + DEF_SGET(Instruction::SGET_OBJECT, 2u, 1u), + DEF_SGET(Instruction::SGET_OBJECT, 3u, 0u), + DEF_AGET(Instruction::AGET, 4u, 3u, 100u), + }; + + PrepareSFields(sfields); + MakeSFieldUninitialized(1u); + PrepareMIRs(mirs); + PerformLVN(); + ASSERT_EQ(value_names_.size(), 5u); + EXPECT_NE(value_names_[0], value_names_[3]); +} + } // namespace art diff --git a/compiler/dex/mir_optimization.cc b/compiler/dex/mir_optimization.cc index 4b2bc4a48..6566e0368 100644 --- a/compiler/dex/mir_optimization.cc +++ b/compiler/dex/mir_optimization.cc @@ -998,11 +998,14 @@ bool MIRGraph::EliminateClassInitChecksGate() { mir->dalvikInsn.opcode <= Instruction::SPUT_SHORT) { const MirSFieldLoweringInfo& field_info = GetSFieldLoweringInfo(mir); uint16_t index = 0xffffu; - if (field_info.IsResolved() && !field_info.IsInitialized()) { + if (!field_info.IsInitialized()) { DCHECK_LT(class_to_index_map.size(), 0xffffu); MapEntry entry = { - field_info.DeclaringDexFile(), - field_info.DeclaringClassIndex(), + // Treat unresolved fields as if each had its own class. + field_info.IsResolved() ? field_info.DeclaringDexFile() + : nullptr, + field_info.IsResolved() ? field_info.DeclaringClassIndex() + : field_info.FieldIndex(), static_cast(class_to_index_map.size()) }; index = class_to_index_map.insert(entry).first->index; diff --git a/compiler/dex/mir_optimization_test.cc b/compiler/dex/mir_optimization_test.cc index 9b2e79838..8c70b5c75 100644 --- a/compiler/dex/mir_optimization_test.cc +++ b/compiler/dex/mir_optimization_test.cc @@ -248,7 +248,7 @@ TEST_F(ClassInitCheckEliminationTest, SingleBlock) { DEF_MIR(Instruction::SGET, 3u, 4u), }; static const bool expected_ignore_clinit_check[] = { - false, false, false, false, false, true, true, true, false, false, true + false, false, false, false, true, true, true, true, true, false, true }; PrepareSFields(sfields); @@ -312,7 +312,7 @@ TEST_F(ClassInitCheckEliminationTest, Diamond) { DEF_MIR(Instruction::SPUT, 6u, 9u), // Eliminated (with sfield[8] in block #4). }; static const bool expected_ignore_clinit_check[] = { - false, false, // Unresolved: sfield[10], method[2] + false, true, // Unresolved: sfield[10], method[2] false, true, // sfield[0] false, false, // sfield[1] false, true, // sfield[2] diff --git a/test/083-compiler-regressions/expected.txt b/test/083-compiler-regressions/expected.txt index 7576b0275..10406c7f8 100644 --- a/test/083-compiler-regressions/expected.txt +++ b/test/083-compiler-regressions/expected.txt @@ -13,6 +13,7 @@ b13679511Test starting 1 false b13679511Test finishing +b16177324TestWrapper caught NPE as expected. largeFrame passes largeFrameFloat passes mulBy1Test passes diff --git a/test/083-compiler-regressions/src/Main.java b/test/083-compiler-regressions/src/Main.java index 6a12ca93f..0f7527cd4 100644 --- a/test/083-compiler-regressions/src/Main.java +++ b/test/083-compiler-regressions/src/Main.java @@ -35,6 +35,7 @@ public class Main { b2487514Test(); b5884080Test(); b13679511Test(); + b16177324TestWrapper(); largeFrameTest(); largeFrameTestFloat(); mulBy1Test(); @@ -908,6 +909,24 @@ public class Main { System.out.println("b13679511Test finishing"); } + static void b16177324TestWrapper() { + try { + b16177324Test(); + } catch (NullPointerException expected) { + System.out.println("b16177324TestWrapper caught NPE as expected."); + } + } + + static void b16177324Test() { + // We need this to be a single BasicBlock. Putting it into a try block would cause it to + // be split at each insn that can throw. So we do the try-catch in a wrapper function. + int v1 = B16177324Values.values[0]; // Null-check on array element access. + int v2 = B16177324ValuesKiller.values[0]; // clinit<>() sets B16177324Values.values to null. + int v3 = B16177324Values.values[0]; // Should throw NPE. + // If the null-check for v3 was eliminated we should fail with SIGSEGV. + System.out.println("Unexpectedly retrieved all values: " + v1 + ", " + v2 + ", " + v3); + } + static double TooManyArgs( long l00, long l01, @@ -9743,3 +9762,14 @@ class LiveFlags { } } } + +class B16177324Values { + public static int values[] = { 42 }; +} + +class B16177324ValuesKiller { + public static int values[] = { 1234 }; + static { + B16177324Values.values = null; + } +}