From: Vladimir Marko Date: Thu, 3 Nov 2016 13:01:28 +0000 (+0000) Subject: Fix SimplifyIfs() trying to redirect exception handler edges. X-Git-Url: http://git.osdn.net/view?a=commitdiff_plain;h=606c8f04d45df8f09f56cbce9881a57a4ae43174;p=android-x86%2Fart.git Fix SimplifyIfs() trying to redirect exception handler edges. Test: 622-simplifyifs-exception-edges Test: m test-art-host Bug: 32545860 Bug: 32546110 Bug: 18051191 Change-Id: I641ed73baace840411fc5651937bbc80e79b0c46 --- diff --git a/compiler/optimizing/dead_code_elimination.cc b/compiler/optimizing/dead_code_elimination.cc index 9de521ad8..c31c66a05 100644 --- a/compiler/optimizing/dead_code_elimination.cc +++ b/compiler/optimizing/dead_code_elimination.cc @@ -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 index 000000000..e69de29bb diff --git a/test/622-simplifyifs-exception-edges/info.txt b/test/622-simplifyifs-exception-edges/info.txt new file mode 100644 index 000000000..58c4bfbbc --- /dev/null +++ b/test/622-simplifyifs-exception-edges/info.txt @@ -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 index 000000000..5e91258ed --- /dev/null +++ b/test/622-simplifyifs-exception-edges/smali/Test.smali @@ -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 index 000000000..636f04719 --- /dev/null +++ b/test/622-simplifyifs-exception-edges/src/Main.java @@ -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; +}