Fix ClinitCheck pruning.
Make sure we merge the ClinitCheck only with LoadClass and
HInvokeStaticOrDirect that is a part of the very same dex
instruction. This fixes incorrect stack traces from class
initializers (wrong dex pcs).
Rewrite the pruning to do all the ClinitCheck merging when
we see the ClinitCheck, instead of merging ClinitCheck into
LoadClass and then LoadClass into HInvokeStaticOrDirect.
When we later see an HInvokeStaticOrDirect with an explicit
check (i.e. not merged), we know that some other instruction
is doing the check and the invoke doesn't need to, so we
mark it as not requiring the check at all. (Previously it
would have been marked as having an implicit check.)
Remove the restriction on merging with inlined invoke static
as this is not necessary anymore. This was a workaround for
X.test():
invoke-static C.foo() [1]
C.foo():
invoke-static C.bar() [2]
After inlining and GVN we have
X.test():
LoadClass C (from [1])
ClinitCheck C (from [1], to be merged to LoadClass)
InvokeStaticOrDirect C.bar() (from [2])
and the LoadClass must not be merged into the invoke as this
would cause the resolution trampoline to see an inlined
frame from the not-yet-loaded class C during the stack walk
and try to load the class. However, we're not allowed to
load new classes at that point, so an attempt to do so leads
to an assertion failure. With this CL, LoadClass is not
merged when it comes from a different instruction, so we can
guarantee that all inlined frames seen by the stack walk in
the resolution trampoline belong to already loaded classes.
Change-Id: I2b8da8d4f295355dce17141f0fab2dace126684d
diff --git a/compiler/optimizing/graph_visualizer.cc b/compiler/optimizing/graph_visualizer.cc
index 2b77901..af3ecb1 100644
--- a/compiler/optimizing/graph_visualizer.cc
+++ b/compiler/optimizing/graph_visualizer.cc
@@ -397,6 +397,9 @@
<< invoke->IsRecursive()
<< std::noboolalpha;
StartAttributeStream("intrinsic") << invoke->GetIntrinsic();
+ if (invoke->IsStatic()) {
+ StartAttributeStream("clinit_check") << invoke->GetClinitCheckRequirement();
+ }
}
void VisitUnresolvedInstanceFieldGet(HUnresolvedInstanceFieldGet* field_access) OVERRIDE {
diff --git a/compiler/optimizing/nodes.cc b/compiler/optimizing/nodes.cc
index 73a44ee..0a39ff3 100644
--- a/compiler/optimizing/nodes.cc
+++ b/compiler/optimizing/nodes.cc
@@ -2068,6 +2068,19 @@
}
}
+std::ostream& operator<<(std::ostream& os, HInvokeStaticOrDirect::ClinitCheckRequirement rhs) {
+ switch (rhs) {
+ case HInvokeStaticOrDirect::ClinitCheckRequirement::kExplicit:
+ return os << "explicit";
+ case HInvokeStaticOrDirect::ClinitCheckRequirement::kImplicit:
+ return os << "implicit";
+ case HInvokeStaticOrDirect::ClinitCheckRequirement::kNone:
+ return os << "none";
+ default:
+ return os << "unknown:" << static_cast<int>(rhs);
+ }
+}
+
void HInstruction::RemoveEnvironmentUsers() {
for (HUseIterator<HEnvironment*> use_it(GetEnvUses()); !use_it.Done(); use_it.Advance()) {
HUseListNode<HEnvironment*>* user_node = use_it.Current();
diff --git a/compiler/optimizing/nodes.h b/compiler/optimizing/nodes.h
index 2878ac9..2160601 100644
--- a/compiler/optimizing/nodes.h
+++ b/compiler/optimizing/nodes.h
@@ -3505,20 +3505,19 @@
return GetInvokeType() == kStatic;
}
- // Remove the art::HLoadClass instruction set as last input by
- // art::PrepareForRegisterAllocation::VisitClinitCheck in lieu of
- // the initial art::HClinitCheck instruction (only relevant for
- // static calls with explicit clinit check).
- void RemoveLoadClassAsLastInput() {
+ // Remove the HClinitCheck or the replacement HLoadClass (set as last input by
+ // PrepareForRegisterAllocation::VisitClinitCheck() in lieu of the initial HClinitCheck)
+ // instruction; only relevant for static calls with explicit clinit check.
+ void RemoveExplicitClinitCheck(ClinitCheckRequirement new_requirement) {
DCHECK(IsStaticWithExplicitClinitCheck());
size_t last_input_index = InputCount() - 1;
HInstruction* last_input = InputAt(last_input_index);
DCHECK(last_input != nullptr);
- DCHECK(last_input->IsLoadClass()) << last_input->DebugName();
+ DCHECK(last_input->IsLoadClass() || last_input->IsClinitCheck()) << last_input->DebugName();
RemoveAsUserOfInput(last_input_index);
inputs_.pop_back();
- clinit_check_requirement_ = ClinitCheckRequirement::kImplicit;
- DCHECK(IsStaticWithImplicitClinitCheck());
+ clinit_check_requirement_ = new_requirement;
+ DCHECK(!IsStaticWithExplicitClinitCheck());
}
bool IsStringFactoryFor(HFakeString* str) const {
@@ -3539,7 +3538,7 @@
}
// Is this a call to a static method whose declaring class has an
- // explicit intialization check in the graph?
+ // explicit initialization check in the graph?
bool IsStaticWithExplicitClinitCheck() const {
return IsStatic() && (clinit_check_requirement_ == ClinitCheckRequirement::kExplicit);
}
@@ -3583,6 +3582,7 @@
DISALLOW_COPY_AND_ASSIGN(HInvokeStaticOrDirect);
};
+std::ostream& operator<<(std::ostream& os, HInvokeStaticOrDirect::ClinitCheckRequirement rhs);
class HInvokeVirtual : public HInvoke {
public:
diff --git a/compiler/optimizing/prepare_for_register_allocation.cc b/compiler/optimizing/prepare_for_register_allocation.cc
index ca928ae..f3d075c 100644
--- a/compiler/optimizing/prepare_for_register_allocation.cc
+++ b/compiler/optimizing/prepare_for_register_allocation.cc
@@ -48,12 +48,46 @@
}
void PrepareForRegisterAllocation::VisitClinitCheck(HClinitCheck* check) {
- HLoadClass* cls = check->GetLoadClass();
- check->ReplaceWith(cls);
- if (check->GetPrevious() == cls) {
+ // Try to find a static invoke from which this check originated.
+ HInvokeStaticOrDirect* invoke = nullptr;
+ for (HUseIterator<HInstruction*> it(check->GetUses()); !it.Done(); it.Advance()) {
+ HInstruction* user = it.Current()->GetUser();
+ if (user->IsInvokeStaticOrDirect() && CanMoveClinitCheck(check, user)) {
+ invoke = user->AsInvokeStaticOrDirect();
+ DCHECK(invoke->IsStaticWithExplicitClinitCheck());
+ invoke->RemoveExplicitClinitCheck(HInvokeStaticOrDirect::ClinitCheckRequirement::kImplicit);
+ break;
+ }
+ }
+ // If we found a static invoke for merging, remove the check from all other static invokes.
+ if (invoke != nullptr) {
+ for (HUseIterator<HInstruction*> it(check->GetUses()); !it.Done(); ) {
+ HInstruction* user = it.Current()->GetUser();
+ DCHECK(invoke->StrictlyDominates(user)); // All other uses must be dominated.
+ it.Advance(); // Advance before we remove the node, reference to the next node is preserved.
+ if (user->IsInvokeStaticOrDirect()) {
+ user->AsInvokeStaticOrDirect()->RemoveExplicitClinitCheck(
+ HInvokeStaticOrDirect::ClinitCheckRequirement::kNone);
+ }
+ }
+ }
+
+ HLoadClass* load_class = check->GetLoadClass();
+ bool can_merge_with_load_class = CanMoveClinitCheck(load_class, check);
+
+ check->ReplaceWith(load_class);
+
+ if (invoke != nullptr) {
+ // Remove the check from the graph. It has been merged into the invoke.
+ check->GetBlock()->RemoveInstruction(check);
+ // Check if we can merge the load class as well.
+ if (can_merge_with_load_class && !load_class->HasUses()) {
+ load_class->GetBlock()->RemoveInstruction(load_class);
+ }
+ } else if (can_merge_with_load_class) {
// Pass the initialization duty to the `HLoadClass` instruction,
// and remove the instruction from the graph.
- cls->SetMustGenerateClinitCheck(true);
+ load_class->SetMustGenerateClinitCheck(true);
check->GetBlock()->RemoveInstruction(check);
}
}
@@ -86,30 +120,60 @@
DCHECK(last_input != nullptr)
<< "Last input is not HLoadClass. It is " << last_input->DebugName();
- // Remove a load class instruction as last input of a static
- // invoke, which has been added (along with a clinit check,
- // removed by PrepareForRegisterAllocation::VisitClinitCheck
- // previously) by the graph builder during the creation of the
- // static invoke instruction, but is no longer required at this
- // stage (i.e., after inlining has been performed).
- invoke->RemoveLoadClassAsLastInput();
+ // Detach the explicit class initialization check from the invoke.
+ // Keeping track of the initializing instruction is no longer required
+ // at this stage (i.e., after inlining has been performed).
+ invoke->RemoveExplicitClinitCheck(HInvokeStaticOrDirect::ClinitCheckRequirement::kNone);
- // The static call will initialize the class so there's no need for a clinit check if
- // it's the first user.
- // There is one special case where we still need the clinit check, when inlining. Because
- // currently the callee is responsible for reporting parameters to the GC, the code
- // that walks the stack during `artQuickResolutionTrampoline` cannot be interrupted for GC.
- // Therefore we cannot allocate any object in that code, including loading a new class.
- if (last_input == invoke->GetPrevious() && !invoke->IsFromInlinedInvoke()) {
- last_input->SetMustGenerateClinitCheck(false);
+ // Merging with load class should have happened in VisitClinitCheck().
+ DCHECK(!CanMoveClinitCheck(last_input, invoke));
+ }
+}
- // If the load class instruction is no longer used, remove it from
- // the graph.
- if (!last_input->HasUses()) {
- last_input->GetBlock()->RemoveInstruction(last_input);
- }
+bool PrepareForRegisterAllocation::CanMoveClinitCheck(HInstruction* input, HInstruction* user) {
+ // Determine if input and user come from the same dex instruction, so that we can move
+ // the clinit check responsibility from one to the other, i.e. from HClinitCheck (user)
+ // to HLoadClass (input), or from HClinitCheck (input) to HInvokeStaticOrDirect (user).
+
+ // Start with a quick dex pc check.
+ if (user->GetDexPc() != input->GetDexPc()) {
+ return false;
+ }
+
+ // Now do a thorough environment check that this is really coming from the same instruction in
+ // the same inlined graph. Unfortunately, we have to go through the whole environment chain.
+ HEnvironment* user_environment = user->GetEnvironment();
+ HEnvironment* input_environment = input->GetEnvironment();
+ while (user_environment != nullptr || input_environment != nullptr) {
+ if (user_environment == nullptr || input_environment == nullptr) {
+ // Different environment chain length. This happens when a method is called
+ // once directly and once indirectly through another inlined method.
+ return false;
+ }
+ if (user_environment->GetDexPc() != input_environment->GetDexPc() ||
+ user_environment->GetMethodIdx() != input_environment->GetMethodIdx() ||
+ !IsSameDexFile(user_environment->GetDexFile(), input_environment->GetDexFile())) {
+ return false;
+ }
+ user_environment = user_environment->GetParent();
+ input_environment = input_environment->GetParent();
+ }
+
+ // Check for code motion taking the input to a different block.
+ if (user->GetBlock() != input->GetBlock()) {
+ return false;
+ }
+
+ // In debug mode, check that we have not inserted a throwing instruction
+ // or an instruction with side effects between input and user.
+ if (kIsDebugBuild) {
+ for (HInstruction* between = input->GetNext(); between != user; between = between->GetNext()) {
+ CHECK(between != nullptr); // User must be after input in the same block.
+ CHECK(!between->CanThrow());
+ CHECK(!between->HasSideEffects());
}
}
+ return true;
}
} // namespace art
diff --git a/compiler/optimizing/prepare_for_register_allocation.h b/compiler/optimizing/prepare_for_register_allocation.h
index d7f277f..a70fb30 100644
--- a/compiler/optimizing/prepare_for_register_allocation.h
+++ b/compiler/optimizing/prepare_for_register_allocation.h
@@ -41,6 +41,8 @@
void VisitCondition(HCondition* condition) OVERRIDE;
void VisitInvokeStaticOrDirect(HInvokeStaticOrDirect* invoke) OVERRIDE;
+ bool CanMoveClinitCheck(HInstruction* input, HInstruction* user);
+
DISALLOW_COPY_AND_ASSIGN(PrepareForRegisterAllocation);
};
diff --git a/test/478-checker-clinit-check-pruning/expected.txt b/test/478-checker-clinit-check-pruning/expected.txt
index 387e1a7..7de097f 100644
--- a/test/478-checker-clinit-check-pruning/expected.txt
+++ b/test/478-checker-clinit-check-pruning/expected.txt
@@ -4,3 +4,9 @@
Main$ClassWithClinit4's static initializer
Main$ClassWithClinit5's static initializer
Main$ClassWithClinit6's static initializer
+Main$ClassWithClinit7's static initializer
+Main$ClassWithClinit8's static initializer
+Main$ClassWithClinit9's static initializer
+Main$ClassWithClinit10's static initializer
+Main$ClassWithClinit11's static initializer
+Main$ClassWithClinit12's static initializer
diff --git a/test/478-checker-clinit-check-pruning/src/Main.java b/test/478-checker-clinit-check-pruning/src/Main.java
index cff6273..7993513 100644
--- a/test/478-checker-clinit-check-pruning/src/Main.java
+++ b/test/478-checker-clinit-check-pruning/src/Main.java
@@ -83,7 +83,7 @@
// before the next pass (liveness analysis) instead.
/// CHECK-START: void Main.invokeStaticNotInlined() liveness (before)
- /// CHECK: InvokeStaticOrDirect
+ /// CHECK: InvokeStaticOrDirect clinit_check:implicit
/// CHECK-START: void Main.invokeStaticNotInlined() liveness (before)
/// CHECK-NOT: LoadClass
@@ -269,7 +269,7 @@
/// CHECK-START: void Main.noClinitBecauseOfInvokeStatic() liveness (before)
/// CHECK-DAG: <<IntConstant:i\d+>> IntConstant 0
/// CHECK-DAG: <<LoadClass:l\d+>> LoadClass gen_clinit_check:false
- /// CHECK-DAG: InvokeStaticOrDirect
+ /// CHECK-DAG: InvokeStaticOrDirect clinit_check:implicit
/// CHECK-DAG: StaticFieldSet [<<LoadClass>>,<<IntConstant>>]
/// CHECK-START: void Main.noClinitBecauseOfInvokeStatic() liveness (before)
@@ -289,7 +289,7 @@
/// CHECK-DAG: <<IntConstant:i\d+>> IntConstant 0
/// CHECK-DAG: <<LoadClass:l\d+>> LoadClass gen_clinit_check:true
/// CHECK-DAG: StaticFieldSet [<<LoadClass>>,<<IntConstant>>]
- /// CHECK-DAG: InvokeStaticOrDirect
+ /// CHECK-DAG: InvokeStaticOrDirect clinit_check:none
/// CHECK-START: void Main.clinitBecauseOfFieldAccess() liveness (before)
/// CHECK-NOT: ClinitCheck
@@ -298,6 +298,206 @@
ClassWithClinit2.$noinline$staticMethod();
}
+ /*
+ * Verify that LoadClass from const-class is not merged with
+ * later invoke-static (or it's ClinitCheck).
+ */
+
+ /// CHECK-START: void Main.constClassAndInvokeStatic(java.lang.Iterable) liveness (before)
+ /// CHECK: LoadClass gen_clinit_check:false
+ /// CHECK: InvokeStaticOrDirect clinit_check:implicit
+
+ /// CHECK-START: void Main.constClassAndInvokeStatic(java.lang.Iterable) liveness (before)
+ /// CHECK-NOT: ClinitCheck
+
+ static void constClassAndInvokeStatic(Iterable it) {
+ $opt$inline$ignoreClass(ClassWithClinit7.class);
+ ClassWithClinit7.someStaticMethod(it);
+ }
+
+ static void $opt$inline$ignoreClass(Class c) {
+ }
+
+ static class ClassWithClinit7 {
+ static {
+ System.out.println("Main$ClassWithClinit7's static initializer");
+ }
+
+ // Note: not inlined from constClassAndInvokeStatic() but fully inlined from main().
+ static void someStaticMethod(Iterable it) {
+ // We're not inlining invoke-interface at the moment.
+ it.iterator();
+ }
+ }
+
+ /*
+ * Verify that LoadClass from sget is not merged with later invoke-static.
+ */
+
+ /// CHECK-START: void Main.sgetAndInvokeStatic(java.lang.Iterable) liveness (before)
+ /// CHECK: LoadClass gen_clinit_check:true
+ /// CHECK: InvokeStaticOrDirect clinit_check:none
+
+ /// CHECK-START: void Main.sgetAndInvokeStatic(java.lang.Iterable) liveness (before)
+ /// CHECK-NOT: ClinitCheck
+
+ static void sgetAndInvokeStatic(Iterable it) {
+ $opt$inline$ignoreInt(ClassWithClinit8.value);
+ ClassWithClinit8.someStaticMethod(it);
+ }
+
+ static void $opt$inline$ignoreInt(int i) {
+ }
+
+ static class ClassWithClinit8 {
+ public static int value = 0;
+ static {
+ System.out.println("Main$ClassWithClinit8's static initializer");
+ }
+
+ // Note: not inlined from sgetAndInvokeStatic() but fully inlined from main().
+ static void someStaticMethod(Iterable it) {
+ // We're not inlining invoke-interface at the moment.
+ it.iterator();
+ }
+ }
+
+ /*
+ * Verify that LoadClass from const-class, ClinitCheck from sget and
+ * InvokeStaticOrDirect from invoke-static are not merged.
+ */
+
+ /// CHECK-START: void Main.constClassSgetAndInvokeStatic(java.lang.Iterable) liveness (before)
+ /// CHECK: LoadClass gen_clinit_check:false
+ /// CHECK: ClinitCheck
+ /// CHECK: InvokeStaticOrDirect clinit_check:none
+
+ static void constClassSgetAndInvokeStatic(Iterable it) {
+ $opt$inline$ignoreClass(ClassWithClinit9.class);
+ $opt$inline$ignoreInt(ClassWithClinit9.value);
+ ClassWithClinit9.someStaticMethod(it);
+ }
+
+ static class ClassWithClinit9 {
+ public static int value = 0;
+ static {
+ System.out.println("Main$ClassWithClinit9's static initializer");
+ }
+
+ // Note: not inlined from constClassSgetAndInvokeStatic() but fully inlined from main().
+ static void someStaticMethod(Iterable it) {
+ // We're not inlining invoke-interface at the moment.
+ it.iterator();
+ }
+ }
+
+ /*
+ * Verify that LoadClass from a fully-inlined invoke-static is not merged
+ * with InvokeStaticOrDirect from a later invoke-static to the same method.
+ */
+
+ /// CHECK-START: void Main.inlinedInvokeStaticViaNonStatic(java.lang.Iterable) liveness (before)
+ /// CHECK: LoadClass gen_clinit_check:true
+ /// CHECK: InvokeStaticOrDirect clinit_check:none
+
+ /// CHECK-START: void Main.inlinedInvokeStaticViaNonStatic(java.lang.Iterable) liveness (before)
+ /// CHECK-NOT: ClinitCheck
+
+ static void inlinedInvokeStaticViaNonStatic(Iterable it) {
+ inlinedInvokeStaticViaNonStaticHelper(null);
+ inlinedInvokeStaticViaNonStaticHelper(it);
+ }
+
+ static void inlinedInvokeStaticViaNonStaticHelper(Iterable it) {
+ ClassWithClinit10.inlinedForNull(it);
+ }
+
+ static class ClassWithClinit10 {
+ public static int value = 0;
+ static {
+ System.out.println("Main$ClassWithClinit10's static initializer");
+ }
+
+ static void inlinedForNull(Iterable it) {
+ if (it != null) {
+ // We're not inlining invoke-interface at the moment.
+ it.iterator();
+ }
+ }
+ }
+
+ /*
+ * Check that the LoadClass from an invoke-static C.foo() doesn't get merged with
+ * an invoke-static inside C.foo(). This would mess up the stack walk in the
+ * resolution trampoline where we would have to load C (if C isn't loaded yet)
+ * which is not permitted there.
+ *
+ * Note: In case of failure, we would get an failed assertion during compilation,
+ * so we wouldn't really get to the checker tests below.
+ */
+
+ /// CHECK-START: void Main.inlinedInvokeStaticViaStatic(java.lang.Iterable) liveness (before)
+ /// CHECK: LoadClass gen_clinit_check:true
+ /// CHECK: InvokeStaticOrDirect clinit_check:none
+
+ /// CHECK-START: void Main.inlinedInvokeStaticViaStatic(java.lang.Iterable) liveness (before)
+ /// CHECK-NOT: ClinitCheck
+
+ static void inlinedInvokeStaticViaStatic(Iterable it) {
+ ClassWithClinit11.callInlinedForNull(it);
+ }
+
+ static class ClassWithClinit11 {
+ public static int value = 0;
+ static {
+ System.out.println("Main$ClassWithClinit11's static initializer");
+ }
+
+ static void callInlinedForNull(Iterable it) {
+ inlinedForNull(it);
+ }
+
+ static void inlinedForNull(Iterable it) {
+ // We're not inlining invoke-interface at the moment.
+ it.iterator();
+ }
+ }
+
+ /*
+ * A test similar to inlinedInvokeStaticViaStatic() but doing the indirect invoke
+ * twice with the first one to be fully inlined.
+ */
+
+ /// CHECK-START: void Main.inlinedInvokeStaticViaStaticTwice(java.lang.Iterable) liveness (before)
+ /// CHECK: LoadClass gen_clinit_check:true
+ /// CHECK: InvokeStaticOrDirect clinit_check:none
+
+ /// CHECK-START: void Main.inlinedInvokeStaticViaStaticTwice(java.lang.Iterable) liveness (before)
+ /// CHECK-NOT: ClinitCheck
+
+ static void inlinedInvokeStaticViaStaticTwice(Iterable it) {
+ ClassWithClinit12.callInlinedForNull(null);
+ ClassWithClinit12.callInlinedForNull(it);
+ }
+
+ static class ClassWithClinit12 {
+ public static int value = 0;
+ static {
+ System.out.println("Main$ClassWithClinit12's static initializer");
+ }
+
+ static void callInlinedForNull(Iterable it) {
+ inlinedForNull(it);
+ }
+
+ static void inlinedForNull(Iterable it) {
+ if (it != null) {
+ // We're not inlining invoke-interface at the moment.
+ it.iterator();
+ }
+ }
+ }
+
// TODO: Add a test for the case of a static method whose declaring
// class type index is not available (i.e. when `storage_index`
// equals `DexFile::kDexNoIndex` in
@@ -310,5 +510,12 @@
ClassWithClinit4.invokeStaticNotInlined();
SubClassOfClassWithClinit5.invokeStaticInlined();
SubClassOfClassWithClinit6.invokeStaticNotInlined();
+ Iterable it = new Iterable() { public java.util.Iterator iterator() { return null; } };
+ constClassAndInvokeStatic(it);
+ sgetAndInvokeStatic(it);
+ constClassSgetAndInvokeStatic(it);
+ inlinedInvokeStaticViaNonStatic(it);
+ inlinedInvokeStaticViaStatic(it);
+ inlinedInvokeStaticViaStaticTwice(it);
}
}
