Reland "Include handler stack and full stack when compilation fails."

This reverts commit f71f8e201e76deaa8c2e2e29b189e70765098f83.

Change-Id: Ib4cc2470ea33dbab8921f888cadcaf8bfe470af1
diff --git a/src/main/java/com/android/tools/r8/BaseCommand.java b/src/main/java/com/android/tools/r8/BaseCommand.java
index 50e5be1..9bfdfcd 100644
--- a/src/main/java/com/android/tools/r8/BaseCommand.java
+++ b/src/main/java/com/android/tools/r8/BaseCommand.java
@@ -8,7 +8,9 @@
 import com.android.tools.r8.origin.PathOrigin;
 import com.android.tools.r8.utils.AbortException;
 import com.android.tools.r8.utils.AndroidApp;
+import com.android.tools.r8.utils.Box;
 import com.android.tools.r8.utils.ExceptionDiagnostic;
+import com.android.tools.r8.utils.ExceptionUtils;
 import com.android.tools.r8.utils.InternalOptions;
 import com.android.tools.r8.utils.Reporter;
 import com.android.tools.r8.utils.StringDiagnostic;
@@ -134,14 +136,15 @@
      * associated diagnostics handler and a {@link CompilationFailedException} exception is thrown.
      */
     public final C build() throws CompilationFailedException {
-      try {
-        validate();
-        C c = makeCommand();
-        reporter.failIfPendingErrors();
-        return c;
-      } catch (AbortException e) {
-        throw new CompilationFailedException(e);
-      }
+      Box<C> box = new Box<>(null);
+      ExceptionUtils.withCompilationHandler(
+          reporter,
+          () -> {
+            validate();
+            box.set(makeCommand());
+            reporter.failIfPendingErrors();
+          });
+      return box.get();
     }
 
     // Helper to construct the actual command. Called as part of {@link build()}.
diff --git a/src/main/java/com/android/tools/r8/utils/ExceptionUtils.java b/src/main/java/com/android/tools/r8/utils/ExceptionUtils.java
index e5ba3f1..28d1ed5 100644
--- a/src/main/java/com/android/tools/r8/utils/ExceptionUtils.java
+++ b/src/main/java/com/android/tools/r8/utils/ExceptionUtils.java
@@ -12,6 +12,7 @@
 import com.android.tools.r8.origin.Origin;
 import com.android.tools.r8.origin.PathOrigin;
 import com.android.tools.r8.position.Position;
+import com.google.common.collect.ObjectArrays;
 import java.io.IOException;
 import java.nio.file.FileSystemException;
 import java.nio.file.Paths;
@@ -97,6 +98,10 @@
       suppressed.add(innerMostCause);
       innerMostCause = innerMostCause.getCause();
     }
+    // Add the full stack as a suppressed stack on the inner cause.
+    if (topMostException != innerMostCause) {
+      innerMostCause.addSuppressed(topMostException);
+    }
 
     // If no abort is seen, the exception is not reported, so report it now.
     if (!hasBeenReported) {
@@ -116,10 +121,9 @@
         new CompilationFailedException(message.toString(), innerMostCause);
     // Replace its stack by the cause stack and insert version info at the top.
     String filename = "Version_" + Version.LABEL + ".java";
-    rethrow.setStackTrace(
-        new StackTraceElement[] {
-          new StackTraceElement(Version.class.getSimpleName(), "fakeStackEntry", filename, 0)
-        });
+    StackTraceElement versionElement =
+        new StackTraceElement(Version.class.getSimpleName(), "fakeStackEntry", filename, 0);
+    rethrow.setStackTrace(ObjectArrays.concat(versionElement, rethrow.getStackTrace()));
     return rethrow;
   }
 
diff --git a/src/main/java/com/android/tools/r8/utils/Reporter.java b/src/main/java/com/android/tools/r8/utils/Reporter.java
index f7f9d1f..f251b0c 100644
--- a/src/main/java/com/android/tools/r8/utils/Reporter.java
+++ b/src/main/java/com/android/tools/r8/utils/Reporter.java
@@ -5,7 +5,6 @@
 
 import com.android.tools.r8.Diagnostic;
 import com.android.tools.r8.DiagnosticsHandler;
-import com.android.tools.r8.errors.Unreachable;
 
 public class Reporter implements DiagnosticsHandler {
 
@@ -56,14 +55,13 @@
    */
   public RuntimeException fatalError(Diagnostic error) {
     error(error);
-    failIfPendingErrors();
-    throw new Unreachable();
+    throw abort;
   }
 
   /** @throws AbortException if any error was reported. */
   public synchronized void failIfPendingErrors() {
     if (abort != null) {
-      throw abort;
+      throw new RuntimeException(abort);
     }
   }
 }
diff --git a/src/test/java/com/android/tools/r8/diagnostics/ErrorDuringIrConversionTest.java b/src/test/java/com/android/tools/r8/diagnostics/ErrorDuringIrConversionTest.java
index 1f19d89..b9eabe6 100644
--- a/src/test/java/com/android/tools/r8/diagnostics/ErrorDuringIrConversionTest.java
+++ b/src/test/java/com/android/tools/r8/diagnostics/ErrorDuringIrConversionTest.java
@@ -28,6 +28,7 @@
 import java.io.StringWriter;
 import java.util.concurrent.atomic.AtomicBoolean;
 import org.hamcrest.Matcher;
+import org.hamcrest.core.IsAnything;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
@@ -35,8 +36,6 @@
 @RunWith(Parameterized.class)
 public class ErrorDuringIrConversionTest extends TestBase {
 
-  static final String EXPECTED = StringUtils.lines("Hello, world");
-
   static final Origin ORIGIN =
       new Origin(Origin.root()) {
         @Override
@@ -61,10 +60,21 @@
       CompilationFailedException e, Matcher<String> messageMatcher, Matcher<String> stackMatcher) {
     // Check that the failure exception exiting the compiler contains origin info in the message.
     assertThat(e.getMessage(), messageMatcher);
-    // Check that the stack trace has the version marker.
     StringWriter writer = new StringWriter();
     e.printStackTrace(new PrintWriter(writer));
-    assertThat(writer.toString(), stackMatcher);
+    String fullStackTrace = writer.toString();
+    // Extract the top cause stack.
+    int topStackTraceEnd = fullStackTrace.indexOf("Caused by:");
+    String topStackTrace = fullStackTrace.substring(0, topStackTraceEnd);
+    String restStackTrace = fullStackTrace.substring(topStackTraceEnd);
+    // Check that top stack trace always has the version marker.
+    assertThat(topStackTrace, containsString("fakeStackEntry"));
+    // Check that top stack has the D8 entry (from tests the non-renamed entry is ToolHelper.runD8).
+    assertThat(topStackTrace, containsString("com.android.tools.r8.ToolHelper.runD8("));
+    // Check that the stack trace always has the suppressed info.
+    assertThat(restStackTrace, containsString(StringUtils.LINE_SEPARATOR + "\tSuppressed:"));
+    // Custom test checks.
+    assertThat(restStackTrace, stackMatcher);
   }
 
   private static void throwNPE() {
@@ -90,9 +100,7 @@
               });
     } catch (CompilationFailedException e) {
       checkCompilationFailedException(
-          e,
-          containsString(ORIGIN.toString()),
-          allOf(containsString("fakeStackEntry"), containsString("throwNPE")));
+          e, containsString(ORIGIN.toString()), containsString("throwNPE"));
       return;
     }
     fail("Expected compilation to fail");
@@ -122,8 +130,7 @@
                         diagnosticOrigin(Origin.unknown())));
               });
     } catch (CompilationFailedException e) {
-      checkCompilationFailedException(
-          e, containsString(ORIGIN.toString()), containsString("fakeStackEntry"));
+      checkCompilationFailedException(e, containsString(ORIGIN.toString()), new IsAnything<>());
       return;
     }
     fail("Expected compilation to fail");
@@ -173,10 +180,9 @@
           // There may be no fail-if-error barrier inside any origin association, thus only the
           // top level message can be expected here.
           containsString("Compilation failed to complete"),
-          // The stack trace must contain both the version, the frame for the hook above, and one
+          // The stack trace must contain the reportErrors frame for the hook above, and one
           // of the error messages.
           allOf(
-              containsString("fakeStackEntry"),
               containsString("reportErrors"),
               anyOf(containsString("FOO!"), containsString("BAR!"), containsString("BAZ!"))));
       return;
diff --git a/src/test/java/com/android/tools/r8/naming/SeedMapperTests.java b/src/test/java/com/android/tools/r8/naming/SeedMapperTests.java
index eb670d6..d420618 100644
--- a/src/test/java/com/android/tools/r8/naming/SeedMapperTests.java
+++ b/src/test/java/com/android/tools/r8/naming/SeedMapperTests.java
@@ -52,7 +52,7 @@
     try {
       SeedMapper.seedMapperFromFile(reporter, applyMappingFile);
       fail("Should have thrown an error");
-    } catch (AbortException e) {
+    } catch (RuntimeException e) {
       assertEquals(1, testDiagnosticMessages.getErrors().size());
       Diagnostic diagnostic = testDiagnosticMessages.getErrors().get(0);
       assertEquals(
@@ -75,7 +75,7 @@
     try {
       SeedMapper.seedMapperFromFile(reporter, applyMappingFile);
       fail("Should have thrown an error");
-    } catch (AbortException e) {
+    } catch (RuntimeException e) {
       assertEquals(1, testDiagnosticMessages.getErrors().size());
       Diagnostic diagnostic = testDiagnosticMessages.getErrors().get(0);
       assertEquals(
@@ -98,7 +98,7 @@
     try {
       SeedMapper.seedMapperFromFile(reporter, applyMappingFile);
       fail("Should have thrown an error");
-    } catch (AbortException e) {
+    } catch (RuntimeException e) {
       assertEquals(1, testDiagnosticMessages.getErrors().size());
       Diagnostic diagnostic = testDiagnosticMessages.getErrors().get(0);
       assertEquals(
@@ -116,7 +116,7 @@
     try {
       SeedMapper.seedMapperFromFile(reporter, applyMappingFile);
       fail("Should have thrown an error");
-    } catch (AbortException e) {
+    } catch (RuntimeException e) {
       assertEquals(1, testDiagnosticMessages.getErrors().size());
       Diagnostic diagnostic = testDiagnosticMessages.getErrors().get(0);
       assertEquals(
diff --git a/src/test/java/com/android/tools/r8/shaking/ProguardConfigurationParserTest.java b/src/test/java/com/android/tools/r8/shaking/ProguardConfigurationParserTest.java
index 3b39b815..4cada28 100644
--- a/src/test/java/com/android/tools/r8/shaking/ProguardConfigurationParserTest.java
+++ b/src/test/java/com/android/tools/r8/shaking/ProguardConfigurationParserTest.java
@@ -766,7 +766,7 @@
           new ProguardConfigurationParser(new DexItemFactory(), reporter);
       parser.parse(path);
       fail("Expect to fail due to the lack of file name.");
-    } catch (AbortException e) {
+    } catch (RuntimeException e) {
       checkDiagnostics(handler.errors, path, 6, 14, "File name expected");
     }
   }
@@ -795,7 +795,7 @@
       new ProguardConfigurationParser(new DexItemFactory(), reporter)
           .parse(path);
       fail();
-    } catch (AbortException e) {
+    } catch (RuntimeException e) {
       checkDiagnostics(handler.errors, path, 6, 10,"does-not-exist.flags");
     }
   }
@@ -807,7 +807,7 @@
       new ProguardConfigurationParser(new DexItemFactory(), reporter)
           .parse(path);
       fail();
-    } catch (AbortException e) {
+    } catch (RuntimeException e) {
       checkDiagnostics(handler.errors, path, 6,2, "does-not-exist.flags");
     }
   }
@@ -832,7 +832,7 @@
       parser.parse(createConfigurationForTesting(
           Collections.singletonList("-injars abc.jar(*.zip;*.class)")));
       fail();
-    } catch (AbortException e) {
+    } catch (RuntimeException e) {
       assertEquals(1, handler.errors.size());
     }
   }
@@ -1026,7 +1026,7 @@
       reset();
       parser.parse(createConfigurationForTesting(ImmutableList.of(option)));
       fail("Expect to fail due to unsupported option.");
-    } catch (AbortException e) {
+    } catch (RuntimeException e) {
       checkDiagnostics(handler.errors, null, 1, 1, "Unsupported option", option);
     }
   }
@@ -1427,7 +1427,7 @@
           new ProguardConfigurationParser(new DexItemFactory(), reporter);
       parser.parse(createConfigurationForTesting(ImmutableList.of("-keepattributes xxx,")));
       fail();
-    } catch (AbortException e) {
+    } catch (RuntimeException e) {
       assertTrue(
           handler.errors.get(0).getDiagnosticMessage().contains("Expected list element at "));
     }
@@ -1613,7 +1613,7 @@
         reset();
         parser.parse(createConfigurationForTesting(ImmutableList.of(option + " ,")));
         fail("Expect to fail due to the lack of path filter.");
-      } catch (AbortException e) {
+      } catch (RuntimeException e) {
         checkDiagnostics(handler.errors, null, 1, option.length() + 2, "Path filter expected");
       }
     }
@@ -1628,7 +1628,7 @@
         reset();
         parser.parse(createConfigurationForTesting(ImmutableList.of(option + " xxx,,yyy")));
         fail("Expect to fail due to the lack of path filter.");
-      } catch (AbortException e) {
+      } catch (RuntimeException e) {
         checkDiagnostics(handler.errors, null, 1, option.length() + 6, "Path filter expected");
       }
     }
@@ -1643,7 +1643,7 @@
         reset();
         parser.parse(createConfigurationForTesting(ImmutableList.of(option + " xxx,")));
         fail("Expect to fail due to the lack of path filter.");
-      } catch (AbortException e) {
+      } catch (RuntimeException e) {
         checkDiagnostics(handler.errors, null, 1, option.length() + 6, "Path filter expected");
       }
     }
@@ -1819,7 +1819,7 @@
     try {
       parser.parse(proguardConfig);
       fail("Expect to fail due to unsupported constructor name pattern.");
-    } catch (AbortException e) {
+    } catch (RuntimeException e) {
       checkDiagnostics(
           handler.errors, proguardConfig, 5, 3, "Unexpected character", "method name");
     }
@@ -2501,7 +2501,7 @@
     try {
       parser.parse(createConfigurationForTesting(ImmutableList.of("-printusage <>")));
       fail("Expect to fail due to the lack of file name.");
-    } catch (AbortException e) {
+    } catch (RuntimeException e) {
       checkDiagnostics(handler.errors, null, 1, 15, "Value of system property '' not found");
     }
   }
@@ -2518,7 +2518,7 @@
       parser.parse(
           createConfigurationForTesting(ImmutableList.of("-printusage <" + property + ">")));
       fail("Expect to fail due to the lack of file name.");
-    } catch (AbortException e) {
+    } catch (RuntimeException e) {
       checkDiagnostics(
           handler.errors, null, 1, 16, "Value of system property '" + property + "' not found");
     }
@@ -2539,7 +2539,7 @@
       reset();
       parser.parse(createConfigurationForTesting(ImmutableList.of(flag + " " + value)));
       fail("Expect to fail due to un-closed quote.");
-    } catch (AbortException e) {
+    } catch (RuntimeException e) {
       checker.get();
     }
   }
@@ -2759,7 +2759,7 @@
       new ProguardConfigurationParser(new DexItemFactory(), reporter)
           .parse(proguardConfigurationFile);
       fail("Expected to fail since the type name cannot be negated.");
-    } catch (AbortException e) {
+    } catch (RuntimeException e) {
       checkDiagnostics(
           handler.errors,
           proguardConfigurationFile,
@@ -2794,7 +2794,7 @@
       try {
         parser.parse(proguardConfig);
         fail("Expect to fail due to unsupported constructor name pattern.");
-      } catch (AbortException e) {
+      } catch (RuntimeException e) {
         int column = initName.contains("void") ? initName.indexOf("void") + 8
             : (initName.contains("XYZ") ? initName.indexOf(">") + 4 : 3);
         if (initName.contains("XYZ")) {