ART: Fix Quick/Optimizing suspend check assumption mismatch.

Quick's SuspendCheckElimination (SCE) expects that every
method contains a suspend check and it eliminates suspend
checks in loops containing an invoke. Optimizing eliminates
the suspend check from leaf methods, so the combination of
a Quick-compiled loop calling an Optimizing-compiled leaf
method can lead to missing suspend checks and potentially
leading to ANRs.

Enable Quick's kLeafOptimization flag to remove suspend
checks from leaf methods and disable Quick's SCE. This
aligns the suspend check placement for the two backends
and avoids the broken combination.

Currently, all methods containing a try-catch are compiled
with Quick, so it's relatively easy to create a regression
test. However, this test will not be valid when Optimizing
starts supporting try-catch.

Bug: 22657404

(cherry picked from commit d29e8487ff1774b6eb5f0e18d854415c1ee8f6b0)

Change-Id: I733c38bf68bfc2c618f2f2e6b59f8b0e015d7be1

diff --git a/compiler/dex/mir_graph.h b/compiler/dex/mir_graph.h
index dbe9062..23b7c42 100644 (file)
--- a/compiler/dex/mir_graph.h
+++ b/compiler/dex/mir_graph.h
@@ -173,7 +173,17 @@ enum OatMethodAttributes {
 
 typedef uint16_t BasicBlockId;
 static const BasicBlockId NullBasicBlockId = 0;
-static constexpr bool kLeafOptimization = false;
+
+// Leaf optimization is basically the removal of suspend checks from leaf methods.
+// This is incompatible with SuspendCheckElimination (SCE) which eliminates suspend
+// checks from loops that call any non-intrinsic method, since a loop that calls
+// only a leaf method would end up without any suspend checks at all. So turning
+// this on automatically disables the SCE in MIRGraph::EliminateSuspendChecksGate().
+//
+// Since the Optimizing compiler is actually applying the same optimization, Quick
+// must not run SCE anyway, so we enable this optimization as a way to disable SCE
+// while keeping a consistent behavior across the backends, b/22657404.
+static constexpr bool kLeafOptimization = true;
 
 /*
  * In general, vreg/sreg describe Dalvik registers that originated with dx.  However,

diff --git a/compiler/dex/mir_optimization.cc b/compiler/dex/mir_optimization.cc
index 5bb0ce3..80b7ac1 100644 (file)
--- a/compiler/dex/mir_optimization.cc
+++ b/compiler/dex/mir_optimization.cc
@@ -1724,7 +1724,8 @@ void MIRGraph::StringChange() {
 
 
 bool MIRGraph::EliminateSuspendChecksGate() {
-  if ((cu_->disable_opt & (1 << kSuspendCheckElimination)) != 0 ||  // Disabled.
+  if (kLeafOptimization ||           // Incompatible (could create loops without suspend checks).
+      (cu_->disable_opt & (1 << kSuspendCheckElimination)) != 0 ||  // Disabled.
       GetMaxNestedLoops() == 0u ||   // Nothing to do.
       GetMaxNestedLoops() >= 32u ||  // Only 32 bits in suspend_checks_in_loops_[.].
                                      // Exclude 32 as well to keep bit shifts well-defined.

diff --git a/compiler/dex/mir_optimization_test.cc b/compiler/dex/mir_optimization_test.cc
index 10a4337..47123ba 100644 (file)
--- a/compiler/dex/mir_optimization_test.cc
+++ b/compiler/dex/mir_optimization_test.cc
@@ -467,8 +467,17 @@ class SuspendCheckEliminationTest : public MirOptimizationTest {
     cu_.mir_graph->ComputeDominators();
     cu_.mir_graph->ComputeTopologicalSortOrder();
     cu_.mir_graph->SSATransformationEnd();
+
     bool gate_result = cu_.mir_graph->EliminateSuspendChecksGate();
-    ASSERT_TRUE(gate_result);
+    ASSERT_NE(gate_result, kLeafOptimization);
+    if (kLeafOptimization) {
+      // Even with kLeafOptimization on and Gate() refusing to allow SCE, we want
+      // to run the SCE test to avoid bitrot, so we need to initialize explicitly.
+      cu_.mir_graph->suspend_checks_in_loops_ =
+          cu_.mir_graph->arena_->AllocArray<uint32_t>(cu_.mir_graph->GetNumBlocks(),
+                                                      kArenaAllocMisc);
+    }
+
     TopologicalSortIterator iterator(cu_.mir_graph.get());
     bool change = false;
     for (BasicBlock* bb = iterator.Next(change); bb != nullptr; bb = iterator.Next(change)) {

diff --git a/test/109-suspend-check/src/Main.java b/test/109-suspend-check/src/Main.java
index 8046d75..3c3353b 100644 (file)
--- a/test/109-suspend-check/src/Main.java
+++ b/test/109-suspend-check/src/Main.java
@@ -32,6 +32,8 @@ public class Main {
                 new InfiniteWhileLoopWithSpecialPutOrNop(new SpecialMethods2()),
                 new InfiniteWhileLoopWithSpecialConstOrIGet(new SpecialMethods1()),
                 new InfiniteWhileLoopWithSpecialConstOrIGet(new SpecialMethods2()),
+                new InfiniteWhileLoopWithSpecialConstOrIGetInTryCatch(new SpecialMethods1()),
+                new InfiniteWhileLoopWithSpecialConstOrIGetInTryCatch(new SpecialMethods2()),
         };
         doWhileLoopWithLong.start();
         for (SimpleLoopThread loop : simpleLoops) {
@@ -135,6 +137,21 @@ class InfiniteWhileLoopWithSpecialConstOrIGet extends SimpleLoopThread {
   }
 }
 
+class InfiniteWhileLoopWithSpecialConstOrIGetInTryCatch extends SimpleLoopThread {
+  private SpecialMethodInterface smi;
+  public InfiniteWhileLoopWithSpecialConstOrIGetInTryCatch(SpecialMethodInterface smi) {
+    this.smi = smi;
+  }
+  public void run() {
+    try {
+      long i = 0L;
+      while (keepGoing) {
+        i += smi.ConstOrIGet();
+      }
+    } catch (Throwable ignored) { }
+  }
+}
+
 class InfiniteWhileLoopWithIntrinsic extends SimpleLoopThread {
   private String[] strings = { "a", "b", "c", "d" };
   private int sum = 0;