Fix marking methods as always throws

Bug: 373867314
Fixes: 373867314
Test: art/test/testrunner/testrunner.py --host --64 -b --optimizing
Change-Id: I18b5e03021e6eec841050bd39c79480c0378dda2

5 files changed, 77 insertions, 1 deletions

diff --git a/compiler/optimizing/inliner.cc b/compiler/optimizing/inliner.cc
index c90308d39c..1375c36643 100644
--- a/compiler/optimizing/inliner.cc
+++ b/compiler/optimizing/inliner.cc
@@ -422,7 +422,13 @@ static bool AlwaysThrows(ArtMethod* method)
   if (!method->IsCompilable() || !IsMethodVerified(method)) {
     return false;
   }
+
   // Skip native methods, methods with try blocks, and methods that are too large.
+  // TODO(solanes): We could correctly mark methods with try/catch blocks as always throwing as long
+  // as we can get rid of the infinite loop cases. These cases (e.g. `void foo() {while (true) {}}`)
+  // are the only ones that can have no return instruction and still not be an "always throwing
+  // method". Unfortunately, we need to construct the graph to know there's an infinite loop and
+  // therefore not worth the trouble.
   CodeItemDataAccessor accessor(method->DexInstructionData());
   if (!accessor.HasCodeItem() ||
       accessor.TriesSize() != 0 ||
@@ -1993,11 +1999,13 @@ bool HInliner::CanInlineBody(const HGraph* callee_graph,
   }
 
   bool has_one_return = false;
+  bool has_try_catch = false;
   for (HBasicBlock* predecessor : exit_block->GetPredecessors()) {
     const HInstruction* last_instruction = predecessor->GetLastInstruction();
     // On inlinees, we can have Return/ReturnVoid/Throw -> TryBoundary -> Exit. To check for the
     // actual last instruction, we have to skip the TryBoundary instruction.
     if (last_instruction->IsTryBoundary()) {
+      has_try_catch = true;
       predecessor = predecessor->GetSinglePredecessor();
       last_instruction = predecessor->GetLastInstruction();
 
@@ -2039,7 +2047,9 @@ bool HInliner::CanInlineBody(const HGraph* callee_graph,
   }
 
   if (!has_one_return) {
-    if (!is_speculative) {
+    // If a method has a try catch, all throws are potentially caught. We are conservative and
+    // don't assume a method always throws unless we can guarantee that.
+    if (!is_speculative && !has_try_catch) {
       // If we know that the method always throws with the particular parameters, set it as such.
       // This is better than using the dex instructions as we have more information about this
       // particular call. We don't mark speculative inlines (e.g. the ones from the inline cache) as
@@ -2048,6 +2058,13 @@ bool HInliner::CanInlineBody(const HGraph* callee_graph,
       graph_->SetHasAlwaysThrowingInvokes(/* value= */ true);
     }
 
+    // Methods that contain infinite loops with try catches fall into this line too as we construct
+    // an Exit block for them. This will mean that the stat `kNotInlinedAlwaysThrows` might not be
+    // 100% correct but:
+    // 1) This is a very small fraction of methods, and
+    // 2) It is not easy to disambiguate between those.
+    // Since we want to avoid inlining methods with infinite loops anyway, we return false for these
+    // cases too.
     LOG_FAIL(stats_, MethodCompilationStat::kNotInlinedAlwaysThrows)
         << "Method " << resolved_method->PrettyMethod()
         << " could not be inlined because it always throws";
diff --git a/test/2282-checker-always-throws-try-catch/expected-stderr.txt b/test/2282-checker-always-throws-try-catch/expected-stderr.txt
new file mode 100644
index 0000000000..e69de29bb2
--- /dev/null
+++ b/test/2282-checker-always-throws-try-catch/expected-stderr.txt
diff --git a/test/2282-checker-always-throws-try-catch/expected-stdout.txt b/test/2282-checker-always-throws-try-catch/expected-stdout.txt
new file mode 100644
index 0000000000..e69de29bb2
--- /dev/null
+++ b/test/2282-checker-always-throws-try-catch/expected-stdout.txt
diff --git a/test/2282-checker-always-throws-try-catch/info.txt b/test/2282-checker-always-throws-try-catch/info.txt
new file mode 100644
index 0000000000..49168b8c0a
--- /dev/null
+++ b/test/2282-checker-always-throws-try-catch/info.txt
@@ -0,0 +1,2 @@
+Tests we don't mark an always throwing method with a try/catch. Otherwise,
+we might incorrectly mark other methods as "always throws" methods.
diff --git a/test/2282-checker-always-throws-try-catch/src/Main.java b/test/2282-checker-always-throws-try-catch/src/Main.java
new file mode 100644
index 0000000000..4c5357d592
--- /dev/null
+++ b/test/2282-checker-always-throws-try-catch/src/Main.java
@@ -0,0 +1,57 @@
+/*
+ * Copyright (C) 2024 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.
+ */
+
+public class Main {
+    public static void main(String[] args) {
+        $noinline$testAlwaysThrows();
+    }
+
+    // This never visibily throws as all throws are caught within the same method.
+    private static void alwaysThrowsInfiniteLoop() {
+        while (true) {
+            try {
+                throw new Error();
+            } catch (Error expected) {
+            }
+        }
+    }
+
+    private static void alwaysThrows() throws Exception {
+        try {
+            throw new Error();
+        } catch (Error expected) {
+            throw new Exception();
+        }
+    }
+
+    /// CHECK-START: void Main.$noinline$testAlwaysThrows() inliner (after)
+    /// CHECK: InvokeStaticOrDirect method_name:Main.alwaysThrows always_throws:false
+    private static void $noinline$testAlwaysThrows() {
+        try {
+            alwaysThrows();
+            System.out.println("alwaysThrows didn't throw");
+        } catch (Exception expected) {
+        }
+    }
+
+    // Don't call this method as it has an infinite loop
+
+    /// CHECK-START: void Main.$noinline$doNotCallInfiniteLoop() inliner (after)
+    /// CHECK: InvokeStaticOrDirect method_name:Main.alwaysThrowsInfiniteLoop always_throws:false
+    private static void $noinline$doNotCallInfiniteLoop() {
+        alwaysThrowsInfiniteLoop();
+    }
+}