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")) {