OSDN Git Service

Fix SimplifyIfs() trying to redirect exception handler edges.
authorVladimir Marko <vmarko@google.com>
Thu, 3 Nov 2016 13:01:28 +0000 (13:01 +0000)
committerVladimir Marko <vmarko@google.com>
Tue, 8 Nov 2016 10:24:52 +0000 (10:24 +0000)
Test: 622-simplifyifs-exception-edges
Test: m test-art-host
Bug: 32545860
Bug: 32546110
Bug: 18051191
Change-Id: I641ed73baace840411fc5651937bbc80e79b0c46

compiler/optimizing/dead_code_elimination.cc
test/622-simplifyifs-exception-edges/expected.txt [new file with mode: 0644]
test/622-simplifyifs-exception-edges/info.txt [new file with mode: 0644]
test/622-simplifyifs-exception-edges/smali/Test.smali [new file with mode: 0644]
test/622-simplifyifs-exception-edges/src/Main.java [new file with mode: 0644]

index 9de521a..c31c66a 100644 (file)
@@ -161,8 +161,21 @@ static HConstant* Evaluate(HCondition* condition, HInstruction* left, HInstructi
 //        |      |      |
 //       B4      B5    B?
 //
-// This simplification cannot be applied for loop headers, as they
-// contain a suspend check.
+// Note that individual edges can be redirected (for example B2->B3
+// can be redirected as B2->B5) without applying this optimization
+// to other incoming edges.
+//
+// This simplification cannot be applied to catch blocks, because
+// exception handler edges do not represent normal control flow.
+// Though in theory this could still apply to normal control flow
+// going directly to a catch block, we cannot support it at the
+// moment because the catch Phi's inputs do not correspond to the
+// catch block's predecessors, so we cannot identify which
+// predecessor corresponds to a given statically evaluated input.
+//
+// We do not apply this optimization to loop headers as this could
+// create irreducible loops. We rely on the suspend check in the
+// loop header to prevent the pattern match.
 //
 // Note that we rely on the dead code elimination to get rid of B3.
 bool HDeadCodeElimination::SimplifyIfs() {
@@ -172,7 +185,8 @@ bool HDeadCodeElimination::SimplifyIfs() {
   for (HBasicBlock* block : graph_->GetReversePostOrder()) {
     HInstruction* last = block->GetLastInstruction();
     HInstruction* first = block->GetFirstInstruction();
-    if (last->IsIf() &&
+    if (!block->IsCatchBlock() &&
+        last->IsIf() &&
         block->HasSinglePhi() &&
         block->GetFirstPhi()->HasOnlyOneNonEnvironmentUse()) {
       bool has_only_phi_and_if = (last == first) && (last->InputAt(0) == block->GetFirstPhi());
diff --git a/test/622-simplifyifs-exception-edges/expected.txt b/test/622-simplifyifs-exception-edges/expected.txt
new file mode 100644 (file)
index 0000000..e69de29
diff --git a/test/622-simplifyifs-exception-edges/info.txt b/test/622-simplifyifs-exception-edges/info.txt
new file mode 100644 (file)
index 0000000..58c4bfb
--- /dev/null
@@ -0,0 +1,2 @@
+Regression test for the SimplifyIfs() graph simplification erroneously trying
+to redirect exception handler edges.
\ No newline at end of file
diff --git a/test/622-simplifyifs-exception-edges/smali/Test.smali b/test/622-simplifyifs-exception-edges/smali/Test.smali
new file mode 100644 (file)
index 0000000..5e91258
--- /dev/null
@@ -0,0 +1,76 @@
+# Copyright (C) 2016 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 LTest;
+
+.super Ljava/lang/Object;
+
+.method public static test([I)I
+    .locals 2
+    const/4 v0, 0
+    :try1_begin
+    array-length v1, p0
+    :try1_end
+    add-int/lit8 v0, v1, -1
+    :try2_begin
+    aget v0, p0, v0
+    :try2_end
+    :end
+    return v0
+
+    :catch_all
+    # Regression test for bug 32545860:
+    #     SimplifyIfs() would have redirected exception handler edges leading here.
+    # Note: There is no move-exception here to prevent matching the SimplifyIfs() pattern.
+    if-eqz v0, :is_zero
+    const/4 v0, -1
+    goto :end
+    :is_zero
+    const/4 v0, -2
+    goto :end
+
+    .catchall {:try1_begin .. :try1_end } :catch_all
+    .catchall {:try2_begin .. :try2_end } :catch_all
+.end method
+
+.method public static test2([II)I
+    .locals 3
+    move v0, p1
+    :try_begin
+    array-length v1, p0
+    add-int/lit8 v1, v1, -1
+    add-int/lit8 v0, v0, 1
+    aget v1, p0, v1
+    const/4 v0, 2
+    aget v2, p0, p1
+    const/4 v0, 3
+    :try_end
+    :end
+    return v0
+
+    :catch_all
+    # Regression test for bug 32546110:
+    #     SimplifyIfs() would have looked at predecessors of this block based on the indexes
+    #     of the catch Phi's inputs. For catch blocks these two arrays are unrelated, so
+    #     this caused out-of-range access triggering a DCHECK() in dchecked_vector<>.
+    # Note: There is no move-exception here to prevent matching the SimplifyIfs() pattern.
+    if-eqz v0, :is_zero
+    const/4 v0, -1
+    goto :end
+    :is_zero
+    const/4 v0, -2
+    goto :end
+
+    .catchall {:try_begin .. :try_end } :catch_all
+.end method
diff --git a/test/622-simplifyifs-exception-edges/src/Main.java b/test/622-simplifyifs-exception-edges/src/Main.java
new file mode 100644 (file)
index 0000000..636f047
--- /dev/null
@@ -0,0 +1,43 @@
+/*
+ * Copyright (C) 2016 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.lang.reflect.Method;
+import java.lang.reflect.InvocationTargetException;
+
+public class Main {
+    public static void main(String[] args) throws Exception {
+        Class<?> c = Class.forName("Test");
+        Method test = c.getDeclaredMethod("test", int[].class);
+        assertIntEquals(-2, (int)test.invoke(null, new Object[] { null }));
+        assertIntEquals(-1, (int)test.invoke(null, new Object[] { new int[0] }));
+        assertIntEquals(42, (int)test.invoke(null, new Object[] { new int[] { 42 } }));
+
+        Method test2 = c.getDeclaredMethod("test2", int[].class, int.class);
+        assertIntEquals(-2, (int)test2.invoke(null, new Object[] { null, 0 }));
+        assertIntEquals(-1, (int)test2.invoke(null, new Object[] { new int[0], 0 }));
+        assertIntEquals(-1, (int)test2.invoke(null, new Object[] { new int[0], 1 }));
+        assertIntEquals(3, (int)test2.invoke(null, new Object[] { new int[] { 42 }, 0 }));
+    }
+
+    public static void assertIntEquals(int expected, int result) {
+        if (expected != result) {
+            throw new Error("Expected: " + expected + ", found: " + result);
+        }
+    }
+
+    // Workaround for non-zero field ids offset in dex file with no fields. Bug: 18051191
+    static final boolean dummy = false;
+}