Fix issue with Partial LSE and casts/instanceof
If PartialLSE encounters an instanceof or check-cast before the
escapes it could not correctly handle it. Due to how java language
code is typically developed and compiled this is generally not a
problem but could lead to incorrect codegen on release builds or
DCHECK failures on debug builds. This fixes the issues by (1) causing
partial LSE to consider check-cast and instance-ofs to be escaping.
This also updates the instruction simplifier to be much more
aggressive in removing instance-of and check-casts.
Test: ./test.py --host
Bug: 186041085
Change-Id: Ia513c4210a87a0dfa92f10adc530e17ee631d006
diff --git a/compiler/optimizing/escape.cc b/compiler/optimizing/escape.cc
index f3f5b15..617833c 100644
--- a/compiler/optimizing/escape.cc
+++ b/compiler/optimizing/escape.cc
@@ -41,6 +41,13 @@
if (!escape_visitor(user)) {
return;
}
+ } else if (user->IsCheckCast() || user->IsInstanceOf()) {
+ // TODO Currently we'll just be conservative for Partial LSE and avoid
+ // optimizing check-cast things since we'd need to add blocks otherwise.
+ // Normally the simplifier should be able to just get rid of them
+ if (!escape_visitor(user)) {
+ return;
+ }
} else if (user->IsPhi() ||
user->IsSelect() ||
(user->IsInvoke() && user->GetSideEffects().DoesAnyWrite()) ||
@@ -108,6 +115,9 @@
// Ignore already known inherent escapes and escapes client supplied
// analysis knows is safe. Continue on.
return true;
+ } else if (escape->IsInstanceOf() || escape->IsCheckCast()) {
+ // Ignore since these are not relevant for regular LSE.
+ return true;
} else if (escape->IsReturn()) {
// value is returned but might still be singleton. Continue on.
*is_singleton_and_not_returned = false;
diff --git a/compiler/optimizing/instruction_simplifier.cc b/compiler/optimizing/instruction_simplifier.cc
index 938a775..23a432e 100644
--- a/compiler/optimizing/instruction_simplifier.cc
+++ b/compiler/optimizing/instruction_simplifier.cc
@@ -611,7 +611,16 @@
if (!class_rti.IsValid()) {
// Happens when the loaded class is unresolved.
- return false;
+ if (obj_rti.IsExact()) {
+ // outcome == 'true' && obj_rti is valid implies that class_rti is valid.
+ // Since that's a contradiction we must not pass this check.
+ *outcome = false;
+ return true;
+ } else {
+ // We aren't able to say anything in particular since we don't know the
+ // exact type of the object.
+ return false;
+ }
}
DCHECK(class_rti.IsExact());
if (class_rti.IsSupertypeOf(obj_rti)) {
@@ -633,12 +642,6 @@
void InstructionSimplifierVisitor::VisitCheckCast(HCheckCast* check_cast) {
HInstruction* object = check_cast->InputAt(0);
- if (check_cast->GetTypeCheckKind() != TypeCheckKind::kBitstringCheck &&
- check_cast->GetTargetClass()->NeedsAccessCheck()) {
- // If we need to perform an access check we cannot remove the instruction.
- return;
- }
-
if (CanEnsureNotNullAt(object, check_cast)) {
check_cast->ClearMustDoNullCheck();
}
@@ -649,6 +652,11 @@
return;
}
+ // Minor correctness check.
+ DCHECK(check_cast->GetTargetClass()->StrictlyDominates(check_cast))
+ << "Illegal graph!\n"
+ << check_cast->DumpWithArgs();
+
// Historical note: The `outcome` was initialized to please Valgrind - the compiler can reorder
// the return value check with the `outcome` check, b/27651442.
bool outcome = false;
@@ -658,27 +666,23 @@
MaybeRecordStat(stats_, MethodCompilationStat::kRemovedCheckedCast);
if (check_cast->GetTypeCheckKind() != TypeCheckKind::kBitstringCheck) {
HLoadClass* load_class = check_cast->GetTargetClass();
- if (!load_class->HasUses()) {
+ if (!load_class->HasUses() && !load_class->NeedsAccessCheck()) {
// We cannot rely on DCE to remove the class because the `HLoadClass` thinks it can throw.
- // However, here we know that it cannot because the checkcast was successfull, hence
+ // However, here we know that it cannot because the checkcast was successful, hence
// the class was already loaded.
load_class->GetBlock()->RemoveInstruction(load_class);
}
}
} else {
- // Don't do anything for exceptional cases for now. Ideally we should remove
- // all instructions and blocks this instruction dominates.
+ // TODO Don't do anything for exceptional cases for now. Ideally we should
+ // remove all instructions and blocks this instruction dominates and
+ // replace it with a manual throw.
}
}
}
void InstructionSimplifierVisitor::VisitInstanceOf(HInstanceOf* instruction) {
HInstruction* object = instruction->InputAt(0);
- if (instruction->GetTypeCheckKind() != TypeCheckKind::kBitstringCheck &&
- instruction->GetTargetClass()->NeedsAccessCheck()) {
- // If we need to perform an access check we cannot remove the instruction.
- return;
- }
bool can_be_null = true;
if (CanEnsureNotNullAt(object, instruction)) {
@@ -695,6 +699,11 @@
return;
}
+ // Minor correctness check.
+ DCHECK(instruction->GetTargetClass()->StrictlyDominates(instruction))
+ << "Illegal graph!\n"
+ << instruction->DumpWithArgs();
+
// Historical note: The `outcome` was initialized to please Valgrind - the compiler can reorder
// the return value check with the `outcome` check, b/27651442.
bool outcome = false;
@@ -713,10 +722,11 @@
instruction->GetBlock()->RemoveInstruction(instruction);
if (outcome && instruction->GetTypeCheckKind() != TypeCheckKind::kBitstringCheck) {
HLoadClass* load_class = instruction->GetTargetClass();
- if (!load_class->HasUses()) {
- // We cannot rely on DCE to remove the class because the `HLoadClass` thinks it can throw.
- // However, here we know that it cannot because the instanceof check was successfull, hence
- // the class was already loaded.
+ if (!load_class->HasUses() && !load_class->NeedsAccessCheck()) {
+ // We cannot rely on DCE to remove the class because the `HLoadClass`
+ // thinks it can throw. However, here we know that it cannot because the
+ // instanceof check was successful and we don't need to check the
+ // access, hence the class was already loaded.
load_class->GetBlock()->RemoveInstruction(load_class);
}
}
diff --git a/compiler/optimizing/instruction_simplifier_test.cc b/compiler/optimizing/instruction_simplifier_test.cc
index 7d93809..ac0bdb9 100644
--- a/compiler/optimizing/instruction_simplifier_test.cc
+++ b/compiler/optimizing/instruction_simplifier_test.cc
@@ -20,25 +20,100 @@
#include <tuple>
#include "gtest/gtest.h"
+
+#include "class_root-inl.h"
#include "nodes.h"
#include "optimizing/data_type.h"
#include "optimizing_unit_test.h"
namespace art {
-class InstructionSimplifierTest : public CommonCompilerTest, public OptimizingUnitTestHelper {
+namespace mirror {
+class ClassExt;
+class Throwable;
+} // namespace mirror
+
+template<typename SuperClass>
+class InstructionSimplifierTestBase : public SuperClass, public OptimizingUnitTestHelper {
public:
void SetUp() override {
- CommonCompilerTest::SetUp();
+ SuperClass::SetUp();
gLogVerbosity.compiler = true;
}
void TearDown() override {
- CommonCompilerTest::TearDown();
+ SuperClass::TearDown();
gLogVerbosity.compiler = false;
}
};
+class InstructionSimplifierTest : public InstructionSimplifierTestBase<CommonCompilerTest> {};
+
+// Various configs we can use for testing. Currently used in PartialComparison tests.
+enum class InstanceOfKind {
+ kSelf,
+ kUnrelatedLoaded,
+ kUnrelatedUnloaded,
+ kSupertype,
+};
+
+std::ostream& operator<<(std::ostream& os, const InstanceOfKind& comp) {
+ switch (comp) {
+ case InstanceOfKind::kSupertype:
+ return os << "kSupertype";
+ case InstanceOfKind::kSelf:
+ return os << "kSelf";
+ case InstanceOfKind::kUnrelatedLoaded:
+ return os << "kUnrelatedLoaded";
+ case InstanceOfKind::kUnrelatedUnloaded:
+ return os << "kUnrelatedUnloaded";
+ }
+}
+
+class InstanceOfInstructionSimplifierTestGroup
+ : public InstructionSimplifierTestBase<CommonCompilerTestWithParam<InstanceOfKind>> {
+ public:
+ bool GetConstantResult() const {
+ switch (GetParam()) {
+ case InstanceOfKind::kSupertype:
+ case InstanceOfKind::kSelf:
+ return true;
+ case InstanceOfKind::kUnrelatedLoaded:
+ case InstanceOfKind::kUnrelatedUnloaded:
+ return false;
+ }
+ }
+
+ std::pair<HLoadClass*, HLoadClass*> GetLoadClasses(VariableSizedHandleScope* vshs) {
+ InstanceOfKind kind = GetParam();
+ ScopedObjectAccess soa(Thread::Current());
+ // New inst always needs to have a valid rti since we dcheck that.
+ HLoadClass* new_inst = MakeClassLoad(
+ /* ti= */ std::nullopt, vshs->NewHandle<mirror::Class>(GetClassRoot<mirror::ClassExt>()));
+ new_inst->SetValidLoadedClassRTI();
+ if (kind == InstanceOfKind::kSelf) {
+ return {new_inst, new_inst};
+ }
+ if (kind == InstanceOfKind::kUnrelatedUnloaded) {
+ HLoadClass* target_class = MakeClassLoad();
+ EXPECT_FALSE(target_class->GetLoadedClassRTI().IsValid());
+ return {new_inst, target_class};
+ }
+ // Force both classes to be a real classes.
+ // For simplicity we use class-roots as the types. The new-inst will always
+ // be a ClassExt, unrelated-loaded will always be Throwable and super will
+ // always be Object
+ HLoadClass* target_class = MakeClassLoad(
+ /* ti= */ std::nullopt,
+ vshs->NewHandle<mirror::Class>(kind == InstanceOfKind::kSupertype ?
+ GetClassRoot<mirror::Object>() :
+ GetClassRoot<mirror::Throwable>()));
+ target_class->SetValidLoadedClassRTI();
+ EXPECT_TRUE(target_class->GetLoadedClassRTI().IsValid());
+ return {new_inst, target_class};
+ }
+};
+
// // ENTRY
// switch (param) {
// case 1:
@@ -272,6 +347,7 @@
HPhi* val_phi = MakePhi({c3, c10});
HPhi* obj_phi = MakePhi({obj1, obj2});
+ obj_phi->SetCanBeNull(false);
HInstruction* read_end = new (GetAllocator()) HPredicatedInstanceFieldGet(obj_phi,
nullptr,
val_phi,
@@ -307,4 +383,174 @@
EXPECT_INS_EQ(ifget->InputAt(0), obj_phi);
}
+// // ENTRY
+// obj = new Obj();
+// // Make sure this graph isn't broken
+// if (obj instanceof <other>) {
+// // LEFT
+// } else {
+// // RIGHT
+// }
+// EXIT
+// return obj.field
+TEST_P(InstanceOfInstructionSimplifierTestGroup, ExactClassInstanceOfOther) {
+ VariableSizedHandleScope vshs(Thread::Current());
+ InitGraph(/*handles=*/&vshs);
+
+ AdjacencyListGraph blks(SetupFromAdjacencyList("entry",
+ "exit",
+ {{"entry", "left"},
+ {"entry", "right"},
+ {"left", "breturn"},
+ {"right", "breturn"},
+ {"breturn", "exit"}}));
+#define GET_BLOCK(name) HBasicBlock* name = blks.Get(#name)
+ GET_BLOCK(entry);
+ GET_BLOCK(exit);
+ GET_BLOCK(breturn);
+ GET_BLOCK(left);
+ GET_BLOCK(right);
+#undef GET_BLOCK
+ EnsurePredecessorOrder(breturn, {left, right});
+ HInstruction* test_res = graph_->GetIntConstant(GetConstantResult() ? 1 : 0);
+
+ auto [new_inst_klass, target_klass] = GetLoadClasses(&vshs);
+ HInstruction* new_inst = MakeNewInstance(new_inst_klass);
+ new_inst->SetReferenceTypeInfo(
+ ReferenceTypeInfo::Create(new_inst_klass->GetClass(), /*is_exact=*/true));
+ HInstanceOf* instance_of = new (GetAllocator()) HInstanceOf(new_inst,
+ target_klass,
+ TypeCheckKind::kClassHierarchyCheck,
+ target_klass->GetClass(),
+ 0u,
+ GetAllocator(),
+ nullptr,
+ nullptr);
+ if (target_klass->GetLoadedClassRTI().IsValid()) {
+ instance_of->SetValidTargetClassRTI();
+ }
+ HInstruction* if_inst = new (GetAllocator()) HIf(instance_of);
+ entry->AddInstruction(new_inst_klass);
+ if (new_inst_klass != target_klass) {
+ entry->AddInstruction(target_klass);
+ }
+ entry->AddInstruction(new_inst);
+ entry->AddInstruction(instance_of);
+ entry->AddInstruction(if_inst);
+ ManuallyBuildEnvFor(new_inst_klass, {});
+ if (new_inst_klass != target_klass) {
+ target_klass->CopyEnvironmentFrom(new_inst_klass->GetEnvironment());
+ }
+ new_inst->CopyEnvironmentFrom(new_inst_klass->GetEnvironment());
+
+ HInstruction* goto_left = new (GetAllocator()) HGoto();
+ left->AddInstruction(goto_left);
+
+ HInstruction* goto_right = new (GetAllocator()) HGoto();
+ right->AddInstruction(goto_right);
+
+ HInstruction* read_bottom = MakeIFieldGet(new_inst, DataType::Type::kInt32, MemberOffset(32));
+ HInstruction* return_exit = new (GetAllocator()) HReturn(read_bottom);
+ breturn->AddInstruction(read_bottom);
+ breturn->AddInstruction(return_exit);
+
+ SetupExit(exit);
+
+ // PerformLSE expects this to be empty.
+ graph_->ClearDominanceInformation();
+
+ LOG(INFO) << "Pre simplification " << blks;
+ graph_->ClearDominanceInformation();
+ graph_->BuildDominatorTree();
+ InstructionSimplifier simp(graph_, /*codegen=*/nullptr);
+ simp.Run();
+
+ LOG(INFO) << "Post simplify " << blks;
+
+ if (!GetConstantResult() || GetParam() == InstanceOfKind::kSelf) {
+ EXPECT_INS_RETAINED(target_klass);
+ } else {
+ EXPECT_INS_REMOVED(target_klass);
+ }
+ EXPECT_INS_REMOVED(instance_of);
+ EXPECT_INS_EQ(if_inst->InputAt(0), test_res);
+}
+
+// // ENTRY
+// obj = new Obj();
+// (<other>)obj;
+// // Make sure this graph isn't broken
+// EXIT
+// return obj
+TEST_P(InstanceOfInstructionSimplifierTestGroup, ExactClassCheckCastOther) {
+ VariableSizedHandleScope vshs(Thread::Current());
+ InitGraph(/*handles=*/&vshs);
+
+ AdjacencyListGraph blks(SetupFromAdjacencyList("entry", "exit", {{"entry", "exit"}}));
+#define GET_BLOCK(name) HBasicBlock* name = blks.Get(#name)
+ GET_BLOCK(entry);
+ GET_BLOCK(exit);
+#undef GET_BLOCK
+
+ auto [new_inst_klass, target_klass] = GetLoadClasses(&vshs);
+ HInstruction* new_inst = MakeNewInstance(new_inst_klass);
+ new_inst->SetReferenceTypeInfo(
+ ReferenceTypeInfo::Create(new_inst_klass->GetClass(), /*is_exact=*/true));
+ HCheckCast* check_cast = new (GetAllocator()) HCheckCast(new_inst,
+ target_klass,
+ TypeCheckKind::kClassHierarchyCheck,
+ target_klass->GetClass(),
+ 0u,
+ GetAllocator(),
+ nullptr,
+ nullptr);
+ if (target_klass->GetLoadedClassRTI().IsValid()) {
+ check_cast->SetValidTargetClassRTI();
+ }
+ HInstruction* entry_return = new (GetAllocator()) HReturn(new_inst);
+ entry->AddInstruction(new_inst_klass);
+ if (new_inst_klass != target_klass) {
+ entry->AddInstruction(target_klass);
+ }
+ entry->AddInstruction(new_inst);
+ entry->AddInstruction(check_cast);
+ entry->AddInstruction(entry_return);
+ ManuallyBuildEnvFor(new_inst_klass, {});
+ if (new_inst_klass != target_klass) {
+ target_klass->CopyEnvironmentFrom(new_inst_klass->GetEnvironment());
+ }
+ new_inst->CopyEnvironmentFrom(new_inst_klass->GetEnvironment());
+
+ SetupExit(exit);
+
+ // PerformLSE expects this to be empty.
+ graph_->ClearDominanceInformation();
+
+ LOG(INFO) << "Pre simplification " << blks;
+ graph_->ClearDominanceInformation();
+ graph_->BuildDominatorTree();
+ InstructionSimplifier simp(graph_, /*codegen=*/nullptr);
+ simp.Run();
+
+ LOG(INFO) << "Post simplify " << blks;
+
+ if (!GetConstantResult() || GetParam() == InstanceOfKind::kSelf) {
+ EXPECT_INS_RETAINED(target_klass);
+ } else {
+ EXPECT_INS_REMOVED(target_klass);
+ }
+ if (GetConstantResult()) {
+ EXPECT_INS_REMOVED(check_cast);
+ } else {
+ EXPECT_INS_RETAINED(check_cast);
+ }
+}
+
+INSTANTIATE_TEST_SUITE_P(InstructionSimplifierTest,
+ InstanceOfInstructionSimplifierTestGroup,
+ testing::Values(InstanceOfKind::kSelf,
+ InstanceOfKind::kUnrelatedLoaded,
+ InstanceOfKind::kUnrelatedUnloaded,
+ InstanceOfKind::kSupertype));
+
} // namespace art
diff --git a/compiler/optimizing/load_store_analysis_test.cc b/compiler/optimizing/load_store_analysis_test.cc
index fd15802..67abc0f 100644
--- a/compiler/optimizing/load_store_analysis_test.cc
+++ b/compiler/optimizing/load_store_analysis_test.cc
@@ -38,9 +38,9 @@
namespace art {
-class LoadStoreAnalysisTest : public OptimizingUnitTest {
+class LoadStoreAnalysisTest : public CommonCompilerTest, public OptimizingUnitTestHelper {
public:
- LoadStoreAnalysisTest() : graph_(CreateGraph()) {}
+ LoadStoreAnalysisTest() {}
AdjacencyListGraph SetupFromAdjacencyList(
const std::string_view entry_name,
@@ -58,11 +58,10 @@
}
void CheckReachability(const AdjacencyListGraph& adj,
const std::vector<AdjacencyListGraph::Edge>& reach);
-
- HGraph* graph_;
};
TEST_F(LoadStoreAnalysisTest, ArrayHeapLocations) {
+ CreateGraph();
HBasicBlock* entry = new (GetAllocator()) HBasicBlock(graph_);
graph_->AddBlock(entry);
graph_->SetEntryBlock(entry);
@@ -145,6 +144,7 @@
}
TEST_F(LoadStoreAnalysisTest, FieldHeapLocations) {
+ CreateGraph();
HBasicBlock* entry = new (GetAllocator()) HBasicBlock(graph_);
graph_->AddBlock(entry);
graph_->SetEntryBlock(entry);
@@ -225,6 +225,7 @@
}
TEST_F(LoadStoreAnalysisTest, ArrayIndexAliasingTest) {
+ CreateGraph();
HBasicBlock* entry = new (GetAllocator()) HBasicBlock(graph_);
graph_->AddBlock(entry);
graph_->SetEntryBlock(entry);
@@ -318,6 +319,7 @@
}
TEST_F(LoadStoreAnalysisTest, ArrayAliasingTest) {
+ CreateGraph();
HBasicBlock* entry = new (GetAllocator()) HBasicBlock(graph_);
graph_->AddBlock(entry);
graph_->SetEntryBlock(entry);
@@ -529,6 +531,7 @@
}
TEST_F(LoadStoreAnalysisTest, ArrayIndexCalculationOverflowTest) {
+ CreateGraph();
HBasicBlock* entry = new (GetAllocator()) HBasicBlock(graph_);
graph_->AddBlock(entry);
graph_->SetEntryBlock(entry);
@@ -647,6 +650,7 @@
}
TEST_F(LoadStoreAnalysisTest, TestHuntOriginalRef) {
+ CreateGraph();
HBasicBlock* entry = new (GetAllocator()) HBasicBlock(graph_);
graph_->AddBlock(entry);
graph_->SetEntryBlock(entry);
@@ -753,6 +757,7 @@
}
TEST_F(LoadStoreAnalysisTest, ReachabilityTest1) {
+ CreateGraph();
AdjacencyListGraph blks(SetupFromAdjacencyList(
"entry",
"exit",
@@ -768,6 +773,7 @@
}
TEST_F(LoadStoreAnalysisTest, ReachabilityTest2) {
+ CreateGraph();
AdjacencyListGraph blks(SetupFromAdjacencyList(
"entry",
"exit",
@@ -784,6 +790,7 @@
}
TEST_F(LoadStoreAnalysisTest, ReachabilityTest3) {
+ CreateGraph();
AdjacencyListGraph blks(SetupFromAdjacencyList("entry",
"exit",
{ { "entry", "loop-header" },
@@ -839,6 +846,7 @@
// // EXIT
// obj.field;
TEST_F(LoadStoreAnalysisTest, PartialEscape) {
+ CreateGraph();
AdjacencyListGraph blks(SetupFromAdjacencyList(
"entry",
"exit",
@@ -946,6 +954,7 @@
// // EXIT
// obj.field2;
TEST_F(LoadStoreAnalysisTest, PartialEscape2) {
+ CreateGraph();
AdjacencyListGraph blks(SetupFromAdjacencyList(
"entry",
"exit",
@@ -1054,6 +1063,7 @@
// // EXIT
// obj.field;
TEST_F(LoadStoreAnalysisTest, PartialEscape3) {
+ CreateGraph();
AdjacencyListGraph blks(SetupFromAdjacencyList(
"entry",
"exit",
@@ -1162,6 +1172,171 @@
ASSERT_TRUE(contents.find(blks.Get("exit")) != contents.end());
}
+// For simplicity Partial LSE considers check-casts to escape. It means we don't
+// need to worry about inserting throws.
+// // ENTRY
+// obj = new Obj();
+// obj.field = 10;
+// if (parameter_value) {
+// // LEFT
+// (Foo)obj;
+// } else {
+// // RIGHT
+// obj.field = 20;
+// }
+// // EXIT
+// obj.field;
+TEST_F(LoadStoreAnalysisTest, PartialEscape4) {
+ CreateGraph();
+ AdjacencyListGraph blks(SetupFromAdjacencyList(
+ "entry",
+ "exit",
+ { { "entry", "left" }, { "entry", "right" }, { "left", "exit" }, { "right", "exit" } }));
+ HBasicBlock* entry = blks.Get("entry");
+ HBasicBlock* left = blks.Get("left");
+ HBasicBlock* right = blks.Get("right");
+ HBasicBlock* exit = blks.Get("exit");
+
+ HInstruction* bool_value = new (GetAllocator())
+ HParameterValue(graph_->GetDexFile(), dex::TypeIndex(1), 1, DataType::Type::kBool);
+ HInstruction* c10 = graph_->GetIntConstant(10);
+ HInstruction* c20 = graph_->GetIntConstant(20);
+ HInstruction* cls = MakeClassLoad();
+ HInstruction* new_inst = MakeNewInstance(cls);
+
+ HInstruction* write_entry = MakeIFieldSet(new_inst, c10, MemberOffset(32));
+ HInstruction* if_inst = new (GetAllocator()) HIf(bool_value);
+ entry->AddInstruction(bool_value);
+ entry->AddInstruction(cls);
+ entry->AddInstruction(new_inst);
+ entry->AddInstruction(write_entry);
+ entry->AddInstruction(if_inst);
+
+ ScopedNullHandle<mirror::Class> null_klass_;
+ HInstruction* cls2 = MakeClassLoad();
+ HInstruction* check_cast = new (GetAllocator()) HCheckCast(
+ new_inst, cls2, TypeCheckKind::kExactCheck, null_klass_, 0, GetAllocator(), nullptr, nullptr);
+ HInstruction* goto_left = new (GetAllocator()) HGoto();
+ left->AddInstruction(cls2);
+ left->AddInstruction(check_cast);
+ left->AddInstruction(goto_left);
+
+ HInstruction* write_right = MakeIFieldSet(new_inst, c20, MemberOffset(32));
+ HInstruction* goto_right = new (GetAllocator()) HGoto();
+ right->AddInstruction(write_right);
+ right->AddInstruction(goto_right);
+
+ HInstruction* read_final = MakeIFieldGet(new_inst, DataType::Type::kInt32, MemberOffset(32));
+ exit->AddInstruction(read_final);
+
+ ScopedArenaAllocator allocator(graph_->GetArenaStack());
+ LoadStoreAnalysis lsa(graph_, nullptr, &allocator, LoadStoreAnalysisType::kFull);
+ lsa.Run();
+
+ const HeapLocationCollector& heap_location_collector = lsa.GetHeapLocationCollector();
+ ReferenceInfo* info = heap_location_collector.FindReferenceInfoOf(new_inst);
+ const ExecutionSubgraph* esg = info->GetNoEscapeSubgraph();
+
+ ASSERT_TRUE(esg->IsValid());
+ ASSERT_TRUE(IsValidSubgraph(esg));
+ ASSERT_TRUE(AreExclusionsIndependent(graph_, esg));
+ std::unordered_set<const HBasicBlock*> contents(esg->ReachableBlocks().begin(),
+ esg->ReachableBlocks().end());
+
+ ASSERT_EQ(contents.size(), 3u);
+ ASSERT_TRUE(contents.find(blks.Get("left")) == contents.end());
+
+ ASSERT_TRUE(contents.find(blks.Get("right")) != contents.end());
+ ASSERT_TRUE(contents.find(blks.Get("entry")) != contents.end());
+ ASSERT_TRUE(contents.find(blks.Get("exit")) != contents.end());
+}
+
+// For simplicity Partial LSE considers instance-ofs with bitvectors to escape.
+// // ENTRY
+// obj = new Obj();
+// obj.field = 10;
+// if (parameter_value) {
+// // LEFT
+// obj instanceof /*bitvector*/ Foo;
+// } else {
+// // RIGHT
+// obj.field = 20;
+// }
+// // EXIT
+// obj.field;
+TEST_F(LoadStoreAnalysisTest, PartialEscape5) {
+ VariableSizedHandleScope vshs(Thread::Current());
+ CreateGraph(&vshs);
+ AdjacencyListGraph blks(SetupFromAdjacencyList(
+ "entry",
+ "exit",
+ { { "entry", "left" }, { "entry", "right" }, { "left", "exit" }, { "right", "exit" } }));
+ HBasicBlock* entry = blks.Get("entry");
+ HBasicBlock* left = blks.Get("left");
+ HBasicBlock* right = blks.Get("right");
+ HBasicBlock* exit = blks.Get("exit");
+
+ HInstruction* bool_value = new (GetAllocator())
+ HParameterValue(graph_->GetDexFile(), dex::TypeIndex(1), 1, DataType::Type::kBool);
+ HInstruction* c10 = graph_->GetIntConstant(10);
+ HInstruction* c20 = graph_->GetIntConstant(20);
+ HIntConstant* bs1 = graph_->GetIntConstant(0xffff);
+ HIntConstant* bs2 = graph_->GetIntConstant(0x00ff);
+ HInstruction* cls = MakeClassLoad();
+ HInstruction* null_const = graph_->GetNullConstant();
+ HInstruction* new_inst = MakeNewInstance(cls);
+
+ HInstruction* write_entry = MakeIFieldSet(new_inst, c10, MemberOffset(32));
+ HInstruction* if_inst = new (GetAllocator()) HIf(bool_value);
+ entry->AddInstruction(bool_value);
+ entry->AddInstruction(cls);
+ entry->AddInstruction(new_inst);
+ entry->AddInstruction(write_entry);
+ entry->AddInstruction(if_inst);
+
+ ScopedNullHandle<mirror::Class> null_klass_;
+ HInstruction* instanceof = new (GetAllocator()) HInstanceOf(new_inst,
+ null_const,
+ TypeCheckKind::kBitstringCheck,
+ null_klass_,
+ 0,
+ GetAllocator(),
+ bs1,
+ bs2);
+ HInstruction* goto_left = new (GetAllocator()) HGoto();
+ left->AddInstruction(instanceof);
+ left->AddInstruction(goto_left);
+
+ HInstruction* write_right = MakeIFieldSet(new_inst, c20, MemberOffset(32));
+ HInstruction* goto_right = new (GetAllocator()) HGoto();
+ right->AddInstruction(write_right);
+ right->AddInstruction(goto_right);
+
+ HInstruction* read_final = MakeIFieldGet(new_inst, DataType::Type::kInt32, MemberOffset(32));
+ exit->AddInstruction(read_final);
+
+ ScopedArenaAllocator allocator(graph_->GetArenaStack());
+ LoadStoreAnalysis lsa(graph_, nullptr, &allocator, LoadStoreAnalysisType::kFull);
+ lsa.Run();
+
+ const HeapLocationCollector& heap_location_collector = lsa.GetHeapLocationCollector();
+ ReferenceInfo* info = heap_location_collector.FindReferenceInfoOf(new_inst);
+ const ExecutionSubgraph* esg = info->GetNoEscapeSubgraph();
+
+ ASSERT_TRUE(esg->IsValid());
+ ASSERT_TRUE(IsValidSubgraph(esg));
+ ASSERT_TRUE(AreExclusionsIndependent(graph_, esg));
+ std::unordered_set<const HBasicBlock*> contents(esg->ReachableBlocks().begin(),
+ esg->ReachableBlocks().end());
+
+ ASSERT_EQ(contents.size(), 3u);
+ ASSERT_TRUE(contents.find(blks.Get("left")) == contents.end());
+
+ ASSERT_TRUE(contents.find(blks.Get("right")) != contents.end());
+ ASSERT_TRUE(contents.find(blks.Get("entry")) != contents.end());
+ ASSERT_TRUE(contents.find(blks.Get("exit")) != contents.end());
+}
+
// before we had predicated-set we needed to be able to remove the store as
// well. This test makes sure that still works.
// // ENTRY
@@ -1177,6 +1352,7 @@
// // call_func prevents the elimination of this store.
// obj.f2 = 0;
TEST_F(LoadStoreAnalysisTest, TotalEscapeAdjacentNoPredicated) {
+ CreateGraph();
AdjacencyListGraph blks(SetupFromAdjacencyList(
"entry",
"exit",
@@ -1288,6 +1464,7 @@
// // call_func prevents the elimination of this store.
// obj.f2 = 0;
TEST_F(LoadStoreAnalysisTest, TotalEscapeAdjacent) {
+ CreateGraph();
AdjacencyListGraph blks(SetupFromAdjacencyList(
"entry",
"exit",
@@ -1397,6 +1574,7 @@
// // EXIT
// obj.f0;
TEST_F(LoadStoreAnalysisTest, TotalEscape) {
+ CreateGraph();
AdjacencyListGraph blks(SetupFromAdjacencyList(
"entry",
"exit",
@@ -1509,6 +1687,7 @@
// // EXIT
// return obj;
TEST_F(LoadStoreAnalysisTest, TotalEscape2) {
+ CreateGraph();
AdjacencyListGraph blks(SetupFromAdjacencyList("entry", "exit", { { "entry", "exit" } }));
HBasicBlock* entry = blks.Get("entry");
HBasicBlock* exit = blks.Get("exit");
@@ -1587,6 +1766,7 @@
// // EXIT
// obj.f0
TEST_F(LoadStoreAnalysisTest, DoubleDiamondEscape) {
+ CreateGraph();
AdjacencyListGraph blks(SetupFromAdjacencyList("entry",
"exit",
{ { "entry", "high_left" },
diff --git a/compiler/optimizing/optimizing_unit_test.h b/compiler/optimizing/optimizing_unit_test.h
index 12c1b98..6600ff3 100644
--- a/compiler/optimizing/optimizing_unit_test.h
+++ b/compiler/optimizing/optimizing_unit_test.h
@@ -406,11 +406,12 @@
OptimizingUnitTestHelper::ManuallyBuildEnvFor(ins, ¤t_locals);
}
- HLoadClass* MakeClassLoad(std::optional<dex::TypeIndex> ti = std::nullopt) {
+ HLoadClass* MakeClassLoad(std::optional<dex::TypeIndex> ti = std::nullopt,
+ std::optional<Handle<mirror::Class>> klass = std::nullopt) {
return new (GetAllocator()) HLoadClass(graph_->GetCurrentMethod(),
ti ? *ti : dex::TypeIndex(class_idx_++),
graph_->GetDexFile(),
- /* klass= */ null_klass_,
+ /* klass= */ klass ? *klass : null_klass_,
/* is_referrers_class= */ false,
/* dex_pc= */ 0,
/* needs_access_check= */ false);
diff --git a/test/530-checker-instance-of-simplifier/expected-stderr.txt b/test/530-checker-instance-of-simplifier/expected-stderr.txt
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/test/530-checker-instance-of-simplifier/expected-stderr.txt
diff --git a/test/530-checker-instance-of-simplifier/expected-stdout.txt b/test/530-checker-instance-of-simplifier/expected-stdout.txt
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/test/530-checker-instance-of-simplifier/expected-stdout.txt
diff --git a/test/530-checker-instance-of-simplifier/info.txt b/test/530-checker-instance-of-simplifier/info.txt
new file mode 100644
index 0000000..61b4741
--- /dev/null
+++ b/test/530-checker-instance-of-simplifier/info.txt
@@ -0,0 +1 @@
+Checker test for testing load-store elimination in presence of VecLoad and VecStore.
diff --git a/test/530-checker-instance-of-simplifier/jasmin/Foo.j b/test/530-checker-instance-of-simplifier/jasmin/Foo.j
new file mode 100644
index 0000000..f64b083
--- /dev/null
+++ b/test/530-checker-instance-of-simplifier/jasmin/Foo.j
@@ -0,0 +1,26 @@
+; Copyright (C) 2021 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.
+
+.source Foo.java
+.class public Foo
+.super java/lang/Object
+
+.field public intField I
+
+.method public <init>()V
+ .limit stack 3
+ aload_0
+ invokespecial java/lang/Object/<init>()V
+ return
+.end method
diff --git a/test/530-checker-instance-of-simplifier/jasmin/Main.j b/test/530-checker-instance-of-simplifier/jasmin/Main.j
new file mode 100644
index 0000000..07152ed
--- /dev/null
+++ b/test/530-checker-instance-of-simplifier/jasmin/Main.j
@@ -0,0 +1,134 @@
+; Copyright (C) 2021 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.
+
+.class public Main
+.super java/lang/Object
+
+; Just do simple check that we remove the instance-of. Well formedness
+; checks will be done in gtests.
+;; CHECK-START: int Main.$noinline$test(boolean) instruction_simplifier (before)
+;; CHECK-DAG: LoadClass
+;; CHECK-DAG: LoadClass
+;; CHECK-DAG: InstanceOf
+;
+;; CHECK-START: int Main.$noinline$test(boolean) instruction_simplifier (after)
+;; CHECK-DAG: LoadClass
+;; CHECK-DAG: LoadClass
+;
+;; CHECK-START: int Main.$noinline$test(boolean) instruction_simplifier (after)
+;; CHECK-NOT: InstanceOf
+
+; public static int $noinline$test(boolean escape) {
+; Foo f = new Foo();
+; f.intField = 7
+; if (escape) {
+; if (f instanceof Bar) {
+; $noinline$escape(f);
+; }
+; }
+; return f.intField;
+; }
+.method public static $noinline$test(Z)I
+ .limit stack 3
+ new Foo
+ ; Stack: [f]
+ dup
+ ; Stack: [f, f]
+ invokespecial Foo/<init>()V
+ ; Stack: [f]
+ dup
+ ; Stack: [f, f]
+ ldc 7
+ ; Stack: [f, f, 7]
+ putfield Foo/intField I
+ ; Stack: [f]
+ iload_0
+ ; Stack: [f, escape]
+ ifeq finish
+ ; Stack: [f]
+ dup
+ ; Stack: [f, f]
+ ; NB Baz does not exist
+ instanceof Baz
+ ; Stack: [f, is_instance]
+ ifeq finish
+ ; Stack: [f]
+ dup
+ ; Stack: [f, f]
+ invokestatic Main/$noinline$escape(Ljava/lang/Object;)V
+ ; Stack: [f]
+finish: ; Stack: [f]
+ getfield Foo/intField I
+ ; Stack: [f.intField]
+ ireturn
+.end method
+
+.method public static $noinline$escape(Ljava/lang/Object;)V
+ .limit stack 0
+ return
+.end method
+
+; public static void main(String[] args) {
+; PrintStream out = System.out;
+; int i = $noinline$test(false);
+; if (i != 7) {
+; out.print("FAIL! GOT ");
+; out.println(i);
+; }
+; }
+.method public static main([Ljava/lang/String;)V
+ .limit stack 5
+ ; Stack: []
+ ; locals: [args]
+ getstatic java/lang/System/out Ljava/io/PrintStream;
+ ; Stack: [out]
+ ; locals: [args]
+ astore_0
+ ; Stack: []
+ ; locals: [out]
+ bipush 0
+ ; Stack: [0]
+ ; locals: [out]
+ invokestatic Main/$noinline$test(Z)I
+ ; Stack: [res]
+ ; locals: [out]
+ dup
+ ; Stack: [res, res]
+ ; locals: [out]
+ bipush 7
+ ; Stack: [res, res, 7]
+ ; locals: [out]
+ if_icmpeq finish
+ ; Stack: [res]
+ ; locals: [out]
+ aload_0
+ ; Stack: [res, out]
+ ; locals: [out]
+ dup2
+ ; Stack: [res, out, res, out]
+ ; locals: [out]
+ ldc "FAIL! GOT "
+ ; Stack: [res, out, res, out, "FAIL! GOT "]
+ ; locals: [out]
+ invokevirtual java/io/PrintStream/print(Ljava/lang/String;)V
+ ; Stack: [res, out, res]
+ ; locals: [out]
+ invokevirtual java/io/PrintStream/println(I)V
+ ; Stack: [res]
+ ; locals: [out]
+finish:
+ ; Stack: [res]
+ ; locals: [out]
+ return
+.end method
diff --git a/test/536-checker-needs-access-check/expected-stdout.txt b/test/536-checker-needs-access-check/expected-stdout.txt
index 4acae95..4992f43 100644
--- a/test/536-checker-needs-access-check/expected-stdout.txt
+++ b/test/536-checker-needs-access-check/expected-stdout.txt
@@ -1,4 +1,5 @@
Got expected error instanceof
Got expected error instanceof null
Got expected error checkcast null
+Got expected error checkcast object
Got expected error instanceof (keep LoadClass with access check)
diff --git a/test/536-checker-needs-access-check/src/Main.java b/test/536-checker-needs-access-check/src/Main.java
index 00934e4..0dc2ab1 100644
--- a/test/536-checker-needs-access-check/src/Main.java
+++ b/test/536-checker-needs-access-check/src/Main.java
@@ -38,6 +38,12 @@
}
try {
+ $noinline$testCheckCast(new Object());
+ } catch (IllegalAccessError e) {
+ System.out.println("Got expected error checkcast object");
+ }
+
+ try {
testDontGvnLoadClassWithAccessChecks(new Object());
} catch (IllegalAccessError e) {
System.out.println("Got expected error instanceof (keep LoadClass with access check)");
@@ -48,13 +54,13 @@
}
/// CHECK-START: boolean Main.testInstanceOf() register (after)
- /// CHECK: InstanceOf
+ /// CHECK: LoadClass class_name:other.InaccessibleClass
public static boolean testInstanceOf() {
return ic instanceof InaccessibleClass;
}
/// CHECK-START: boolean Main.testInstanceOfNull() register (after)
- /// CHECK: InstanceOf
+ /// CHECK: LoadClass class_name:other.InaccessibleClass
public static boolean testInstanceOfNull() {
return null instanceof InaccessibleClass;
}
@@ -62,11 +68,17 @@
// TODO: write a test for for CheckCast with not null constant (after RTP can parse arguments).
/// CHECK-START: other.InaccessibleClass Main.testCheckCastNull() register (after)
- /// CHECK: CheckCast
+ /// CHECK: LoadClass class_name:other.InaccessibleClass
public static InaccessibleClass testCheckCastNull() {
return (InaccessibleClass) null;
}
+ /// CHECK-START: other.InaccessibleClass Main.$noinline$testCheckCast(java.lang.Object) register (after)
+ /// CHECK: LoadClass class_name:other.InaccessibleClass
+ public static InaccessibleClass $noinline$testCheckCast(Object o) {
+ return (InaccessibleClass) o;
+ }
+
/// CHECK-START: boolean Main.testDontGvnLoadClassWithAccessChecks(java.lang.Object) inliner (before)
/// CHECK: InvokeStaticOrDirect
diff --git a/tools/checker/checker.py b/tools/checker/checker.py
index 1e03141..ec933f6 100755
--- a/tools/checker/checker.py
+++ b/tools/checker/checker.py
@@ -72,7 +72,7 @@
def find_checker_files(path):
""" Returns a list of files to scan for check annotations in the given path.
Path to a file is returned as a single-element list, directories are
- recursively traversed and all '.java' and '.smali' files returned.
+ recursively traversed and all '.java', '.j', and '.smali' files returned.
"""
if not path:
Logger.fail("No source path provided")
@@ -83,7 +83,7 @@
for root, dirs, files in os.walk(path):
for file in files:
extension = os.path.splitext(file)[1]
- if extension in [".java", ".smali"]:
+ if extension in [".java", ".smali", ".j"]:
found_files.append(os.path.join(root, file))
return found_files
else:
diff --git a/tools/checker/file_format/checker/parser.py b/tools/checker/file_format/checker/parser.py
index a2c52d3..92e9b02 100644
--- a/tools/checker/file_format/checker/parser.py
+++ b/tools/checker/file_format/checker/parser.py
@@ -21,7 +21,7 @@
def _is_checker_line(line):
- return line.startswith("///") or line.startswith("##")
+ return line.startswith("///") or line.startswith("##") or line.startswith(";;")
def _extract_line(prefix, line, arch=None, debuggable=False):
@@ -30,7 +30,7 @@
beginning of the line. Whitespaces are ignored.
"""
r_ignore_whitespace = r"\s*"
- r_comment_symbols = ["///", "##"]
+ r_comment_symbols = ["///", "##", ";;"]
arch_specifier = "-{}".format(arch) if arch is not None else ""
dbg_specifier = "-DEBUGGABLE" if debuggable else ""
regex_prefix = (r_ignore_whitespace +