Check for try catch ranges when verifying CF frames
Bug: b/227982754
Bug: b/228141844
Change-Id: Ic871e255a04a05c32e9025010a316d84b442fe74
diff --git a/src/main/java/com/android/tools/r8/cf/code/CfFrameVerificationHelper.java b/src/main/java/com/android/tools/r8/cf/code/CfFrameVerificationHelper.java
index 0d466f0..c81abe4 100644
--- a/src/main/java/com/android/tools/r8/cf/code/CfFrameVerificationHelper.java
+++ b/src/main/java/com/android/tools/r8/cf/code/CfFrameVerificationHelper.java
@@ -174,16 +174,6 @@
for (CfTryCatch tryCatchRange : tryCatchRanges) {
if (tryCatchRange.start == label) {
currentCatchRanges.add(tryCatchRange);
- // We can have fall-through into this range requiring the current frame being
- // assignable to the handler frame. This is handled for locals when we see a throwing
- // instruction, but we can validate here that the stack will be a single element stack
- // [throwable].
- CfFrame destinationFrame = stateMap.get(tryCatchRange.start);
- if (destinationFrame == null) {
- throw CfCodeStackMapValidatingException.error("No frame for target catch range target");
- }
- checkStackIsAssignable(
- destinationFrame.getStack(), throwStack, factory, isJavaAssignable);
}
}
currentCatchRanges.removeIf(currentRange -> currentRange.end == label);
@@ -191,6 +181,35 @@
return this;
}
+ public void checkTryCatchRange(CfTryCatch tryCatchRange) {
+ // According to the spec:
+ // https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.10.1
+ // saying ` and the handler's target (the initial instruction of the handler code) is type
+ // safe assuming an incoming type state T. The type state T is derived from ExcStackFrame
+ // by replacing the operand stack with a stack whose sole element is the handler's
+ // exception class.
+ tryCatchRange.targets.forEach(
+ target -> {
+ CfFrame destinationFrame = stateMap.get(target);
+ if (destinationFrame == null) {
+ throw CfCodeStackMapValidatingException.error("No frame for target catch range target");
+ }
+ // From the spec: the handler's exception class is assignable to the class Throwable.
+ tryCatchRange.guards.forEach(
+ guard -> {
+ if (!isJavaAssignable.test(guard, factory.throwableType)) {
+ throw CfCodeStackMapValidatingException.error(
+ "Could not assign '" + guard.toSourceString() + "' to throwable.");
+ }
+ checkStackIsAssignable(
+ ImmutableDeque.of(FrameType.initialized(guard)),
+ destinationFrame.getStack(),
+ factory,
+ isJavaAssignable);
+ });
+ });
+ }
+
private void checkFrameIsSet() {
if (currentFrame == NO_FRAME) {
throw CfCodeStackMapValidatingException.error("Unexpected state change");
@@ -218,9 +237,6 @@
if (destinationFrame == null) {
throw CfCodeStackMapValidatingException.error("No frame for target catch range target");
}
- // We have to check all current handler targets have assignable locals and a 1-element
- // stack assignable to throwable. It is not required that the the thrown error is
- // handled.
checkLocalsIsAssignable(
currentFrame.getLocals(), destinationFrame.getLocals(), factory, isJavaAssignable);
}
diff --git a/src/main/java/com/android/tools/r8/cf/code/CfLabel.java b/src/main/java/com/android/tools/r8/cf/code/CfLabel.java
index 9e3b1ee..3df1eba 100644
--- a/src/main/java/com/android/tools/r8/cf/code/CfLabel.java
+++ b/src/main/java/com/android/tools/r8/cf/code/CfLabel.java
@@ -100,6 +100,6 @@
DexMethod context,
AppView<?> appView,
DexItemFactory dexItemFactory) {
- // This is a no-op.
+ frameBuilder.seenLabel(this);
}
}
diff --git a/src/main/java/com/android/tools/r8/graph/CfCode.java b/src/main/java/com/android/tools/r8/graph/CfCode.java
index 910d5f5..4974a5d 100644
--- a/src/main/java/com/android/tools/r8/graph/CfCode.java
+++ b/src/main/java/com/android/tools/r8/graph/CfCode.java
@@ -963,6 +963,16 @@
isAssignablePredicate(appView),
appView.dexItemFactory(),
maxStack);
+ for (CfTryCatch tryCatchRange : tryCatchRanges) {
+ try {
+ builder.checkTryCatchRange(tryCatchRange);
+ } catch (CfCodeStackMapValidatingException ex) {
+ return reportStackMapError(
+ CfCodeStackMapValidatingException.invalidTryCatchRange(
+ method, tryCatchRange, ex.getMessage(), appView),
+ appView);
+ }
+ }
if (stateMap.containsKey(null)) {
assert !shouldComputeInitialFrame();
builder.checkFrameAndSet(stateMap.get(null));
@@ -989,7 +999,7 @@
instruction.evaluate(builder, previousMethodSignature, appView, appView.dexItemFactory());
} catch (CfCodeStackMapValidatingException ex) {
return reportStackMapError(
- CfCodeStackMapValidatingException.toDiagnostics(
+ CfCodeStackMapValidatingException.invalidStackMapForInstruction(
method, i, instruction, ex.getMessage(), appView),
appView);
}
diff --git a/src/main/java/com/android/tools/r8/graph/CfCodeStackMapValidatingException.java b/src/main/java/com/android/tools/r8/graph/CfCodeStackMapValidatingException.java
index 1a25ce6..4b417bf 100644
--- a/src/main/java/com/android/tools/r8/graph/CfCodeStackMapValidatingException.java
+++ b/src/main/java/com/android/tools/r8/graph/CfCodeStackMapValidatingException.java
@@ -5,6 +5,8 @@
package com.android.tools.r8.graph;
import com.android.tools.r8.cf.code.CfInstruction;
+import com.android.tools.r8.cf.code.CfTryCatch;
+import com.android.tools.r8.utils.StringUtils;
public class CfCodeStackMapValidatingException extends RuntimeException {
@@ -52,7 +54,24 @@
sb.toString());
}
- public static CfCodeDiagnostics toDiagnostics(
+ public static CfCodeDiagnostics invalidTryCatchRange(
+ ProgramMethod method, CfTryCatch tryCatch, String detailMessage, AppView<?> appView) {
+ StringBuilder sb =
+ new StringBuilder("Invalid try catch range for ")
+ .append(StringUtils.join(", ", tryCatch.guards, DexType::getTypeName))
+ .append(": ")
+ .append(detailMessage)
+ .append(".");
+ if (appView.enableWholeProgramOptimizations()) {
+ sb.append(" In later version of R8, the method may be assumed not reachable.");
+ }
+ return new CfCodeDiagnostics(
+ method.getOrigin(),
+ appView.graphLens().getOriginalMethodSignature(method.getReference()),
+ sb.toString());
+ }
+
+ public static CfCodeDiagnostics invalidStackMapForInstruction(
ProgramMethod method,
int instructionIndex,
CfInstruction instruction,
diff --git a/src/test/java/com/android/tools/r8/cf/stackmap/SharedExceptionTypeTest.java b/src/test/java/com/android/tools/r8/cf/stackmap/SharedExceptionTypeTest.java
new file mode 100644
index 0000000..1f30f40
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/cf/stackmap/SharedExceptionTypeTest.java
@@ -0,0 +1,84 @@
+// Copyright (c) 2022, the R8 project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+package com.android.tools.r8.cf.stackmap;
+
+import static org.junit.Assume.assumeTrue;
+
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.TestParametersCollection;
+import com.android.tools.r8.utils.AndroidApiLevel;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+@RunWith(Parameterized.class)
+public class SharedExceptionTypeTest extends TestBase {
+
+ private final TestParameters parameters;
+ private final String EXPECTED = "Hello World!";
+
+ @Parameters(name = "{0}")
+ public static TestParametersCollection data() {
+ return getTestParameters().withAllRuntimes().build();
+ }
+
+ public SharedExceptionTypeTest(TestParameters parameters) {
+ this.parameters = parameters;
+ }
+
+ @Test
+ public void testJvm() throws Exception {
+ assumeTrue(parameters.isCfRuntime());
+ testForJvm()
+ .addInnerClasses(SharedExceptionTypeTest.class)
+ .run(parameters.getRuntime(), Main.class)
+ .assertSuccessWithOutputLines(EXPECTED);
+ }
+
+ @Test
+ public void testD8() throws Exception {
+ assumeTrue(parameters.isDexRuntime());
+ testForD8(parameters.getBackend())
+ .addInnerClasses(SharedExceptionTypeTest.class)
+ .setMinApi(AndroidApiLevel.B)
+ .addOptionsModification(options -> options.testing.readInputStackMaps = true)
+ .run(parameters.getRuntime(), Main.class)
+ .assertSuccessWithOutputLines(EXPECTED);
+ }
+
+ public static class ExceptionSuper extends RuntimeException {
+
+ public void printError() {
+ System.out.println("Hello World!");
+ }
+ }
+
+ public static class ExceptionA extends ExceptionSuper {}
+
+ public static class ExceptionB extends ExceptionSuper {}
+
+ public static class Main {
+
+ public static void foo(String[] args) {
+ if (args.length == 0) {
+ throw new ExceptionA();
+ } else if (args.length == 1) {
+ throw new ExceptionB();
+ } else {
+ throw new RuntimeException("FOO BAR");
+ }
+ }
+
+ public static void main(String[] args) {
+ try {
+ foo(args);
+ } catch (ExceptionA | ExceptionB e) {
+ e.printError();
+ }
+ }
+ }
+}
