Only keep throws clauses on kept methods
Bug: b/343448072
Bug: b/343907109
Change-Id: Id084959bfa879d0ccb5543c7856a8f5fe01b3872
diff --git a/src/main/java/com/android/tools/r8/graph/DexAnnotation.java b/src/main/java/com/android/tools/r8/graph/DexAnnotation.java
index efe3ead..ec6b70a 100644
--- a/src/main/java/com/android/tools/r8/graph/DexAnnotation.java
+++ b/src/main/java/com/android/tools/r8/graph/DexAnnotation.java
@@ -676,6 +676,10 @@
}
+ public static boolean isThrowsAnnotation(DexAnnotation annotation, DexItemFactory factory) {
+ return factory.annotationThrows.isIdenticalTo(annotation.annotation.type);
+ }
+
@SuppressWarnings("ReferenceEquality")
public static boolean isAnnotationDefaultAnnotation(
DexAnnotation annotation, DexItemFactory factory) {
diff --git a/src/main/java/com/android/tools/r8/shaking/AnnotationRemover.java b/src/main/java/com/android/tools/r8/shaking/AnnotationRemover.java
index 1fc0e25..f58aab9 100644
--- a/src/main/java/com/android/tools/r8/shaking/AnnotationRemover.java
+++ b/src/main/java/com/android/tools/r8/shaking/AnnotationRemover.java
@@ -103,22 +103,26 @@
assert !DexAnnotation.isEnclosingClassAnnotation(annotation, dexItemFactory);
assert options.passthroughDexCode
|| !DexAnnotation.isSignatureAnnotation(annotation, dexItemFactory);
- if (config.exceptions && DexAnnotation.isThrowingAnnotation(annotation, dexItemFactory)) {
- return true;
+ if (DexAnnotation.isThrowingAnnotation(annotation, dexItemFactory)) {
+ KeepMethodInfo methodInfo = keepInfo.asMethodInfo();
+ return methodInfo != null && !methodInfo.isThrowsRemovalAllowed(options);
}
if (DexAnnotation.isSourceDebugExtension(annotation, dexItemFactory)) {
assert holder.isProgramClass();
appView.setSourceDebugExtensionForType(
holder.asProgramClass(), annotation.annotation.elements[0].value.asDexValueString());
+ // TODO(b/343909250): Is this supposed to be kept on all live items?
return config.sourceDebugExtension;
}
if (config.methodParameters
&& DexAnnotation.isParameterNameAnnotation(annotation, dexItemFactory)) {
+ // TODO(b/343907109): This should be conditional on its own keep info bit.
return true;
}
if (isAnnotationOnAnnotationClass
&& DexAnnotation.isAnnotationDefaultAnnotation(annotation, dexItemFactory)
&& shouldRetainAnnotationDefaultAnnotationOnAnnotationClass(annotation)) {
+ // TODO(b/343909250): Is this supposed to be kept on all live items?
return true;
}
return false;
diff --git a/src/main/java/com/android/tools/r8/shaking/GlobalKeepInfoConfiguration.java b/src/main/java/com/android/tools/r8/shaking/GlobalKeepInfoConfiguration.java
index a2ab3d3..c924664 100644
--- a/src/main/java/com/android/tools/r8/shaking/GlobalKeepInfoConfiguration.java
+++ b/src/main/java/com/android/tools/r8/shaking/GlobalKeepInfoConfiguration.java
@@ -22,6 +22,8 @@
boolean isForceKeepSignatureAttributeEnabled();
+ boolean isForceKeepExceptionsAttributeEnabled();
+
boolean isKeepEnclosingMethodAttributeEnabled();
boolean isKeepInnerClassesAttributeEnabled();
diff --git a/src/main/java/com/android/tools/r8/shaking/KeepMethodInfo.java b/src/main/java/com/android/tools/r8/shaking/KeepMethodInfo.java
index 6b5a8a2..caaba95 100644
--- a/src/main/java/com/android/tools/r8/shaking/KeepMethodInfo.java
+++ b/src/main/java/com/android/tools/r8/shaking/KeepMethodInfo.java
@@ -28,6 +28,7 @@
return bottom().joiner();
}
+ private final boolean allowThrowsRemoval;
private final boolean allowClassInlining;
private final boolean allowClosedWorldReasoning;
private final boolean allowConstantArgumentOptimization;
@@ -45,6 +46,7 @@
protected KeepMethodInfo(Builder builder) {
super(builder);
+ this.allowThrowsRemoval = builder.isThrowsRemovalAllowed();
this.allowClassInlining = builder.isClassInliningAllowed();
this.allowClosedWorldReasoning = builder.isClosedWorldReasoningAllowed();
this.allowConstantArgumentOptimization = builder.isConstantArgumentOptimizationAllowed();
@@ -86,6 +88,21 @@
configuration.isKeepRuntimeInvisibleParameterAnnotationsEnabled());
}
+ /**
+ * True if an item may have its exception throws clause removed.
+ *
+ * <p>This method requires knowledge of the global configuration as that can override the concrete
+ * value on a given item.
+ */
+ public boolean isThrowsRemovalAllowed(GlobalKeepInfoConfiguration configuration) {
+ return !configuration.isForceKeepExceptionsAttributeEnabled()
+ && internalIsThrowsRemovalAllowed();
+ }
+
+ boolean internalIsThrowsRemovalAllowed() {
+ return allowThrowsRemoval;
+ }
+
KeepAnnotationCollectionInfo internalParameterAnnotationsInfo() {
return parameterAnnotationsInfo;
}
@@ -242,6 +259,7 @@
public static class Builder extends KeepMemberInfo.Builder<Builder, KeepMethodInfo> {
+ private boolean allowThrowsRemoval;
private boolean allowClassInlining;
private boolean allowClosedWorldReasoning;
private boolean allowConstantArgumentOptimization;
@@ -263,6 +281,7 @@
protected Builder(KeepMethodInfo original) {
super(original);
+ allowThrowsRemoval = original.internalIsThrowsRemovalAllowed();
allowClassInlining = original.internalIsClassInliningAllowed();
allowClosedWorldReasoning = original.internalIsClosedWorldReasoningAllowed();
allowConstantArgumentOptimization = original.internalIsConstantArgumentOptimizationAllowed();
@@ -280,6 +299,23 @@
parameterAnnotationsInfo = original.internalParameterAnnotationsInfo().toBuilder();
}
+ public boolean isThrowsRemovalAllowed() {
+ return allowThrowsRemoval;
+ }
+
+ private Builder setAllowThrowsRemoval(boolean allowThrowsRemoval) {
+ this.allowThrowsRemoval = allowThrowsRemoval;
+ return self();
+ }
+
+ public Builder allowThrowsRemoval() {
+ return setAllowThrowsRemoval(true);
+ }
+
+ public Builder disallowThrowsRemoval() {
+ return setAllowThrowsRemoval(false);
+ }
+
// Class inlining.
public boolean isClassInliningAllowed() {
@@ -572,6 +608,7 @@
@Override
boolean internalIsEqualTo(KeepMethodInfo other) {
return super.internalIsEqualTo(other)
+ && isThrowsRemovalAllowed() == other.internalIsThrowsRemovalAllowed()
&& isClassInliningAllowed() == other.internalIsClassInliningAllowed()
&& isClosedWorldReasoningAllowed() == other.internalIsClosedWorldReasoningAllowed()
&& isConstantArgumentOptimizationAllowed()
@@ -600,6 +637,7 @@
@Override
public Builder makeTop() {
return super.makeTop()
+ .disallowThrowsRemoval()
.disallowClassInlining()
.disallowClosedWorldReasoning()
.disallowConstantArgumentOptimization()
@@ -619,6 +657,7 @@
@Override
public Builder makeBottom() {
return super.makeBottom()
+ .allowThrowsRemoval()
.allowClassInlining()
.allowClosedWorldReasoning()
.allowConstantArgumentOptimization()
@@ -646,6 +685,11 @@
super(builder);
}
+ public Joiner disallowThrowsRemoval() {
+ builder.disallowThrowsRemoval();
+ return self();
+ }
+
public Joiner disallowClassInlining() {
builder.disallowClassInlining();
return self();
@@ -733,7 +777,8 @@
builder
.getParameterAnnotationsInfo()
.destructiveJoin(joiner.builder.getParameterAnnotationsInfo());
- return applyIf(!joiner.builder.isClassInliningAllowed(), Joiner::disallowClassInlining)
+ return applyIf(!joiner.builder.isThrowsRemovalAllowed(), Joiner::disallowThrowsRemoval)
+ .applyIf(!joiner.builder.isClassInliningAllowed(), Joiner::disallowClassInlining)
.applyIf(
!joiner.builder.isClosedWorldReasoningAllowed(), Joiner::disallowClosedWorldReasoning)
.applyIf(
diff --git a/src/main/java/com/android/tools/r8/shaking/RootSetUtils.java b/src/main/java/com/android/tools/r8/shaking/RootSetUtils.java
index 626dc9a..f6ff126 100644
--- a/src/main/java/com/android/tools/r8/shaking/RootSetUtils.java
+++ b/src/main/java/com/android/tools/r8/shaking/RootSetUtils.java
@@ -1673,6 +1673,11 @@
context.markAsUsed();
}
+ if (attributesConfig.exceptions && item.isMethod()) {
+ itemJoiner.computeIfAbsent().asMethodJoiner().disallowThrowsRemoval();
+ context.markAsUsed();
+ }
+
if (appView.options().isMinificationEnabled() && !modifiers.allowsObfuscation) {
itemJoiner.computeIfAbsent().disallowMinification();
context.markAsUsed();
diff --git a/src/main/java/com/android/tools/r8/utils/InternalOptions.java b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
index f565875..bbfe77c 100644
--- a/src/main/java/com/android/tools/r8/utils/InternalOptions.java
+++ b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
@@ -825,6 +825,13 @@
}
@Override
+ public boolean isForceKeepExceptionsAttributeEnabled() {
+ return proguardConfiguration == null
+ || (isForceProguardCompatibilityEnabled()
+ && proguardConfiguration.getKeepAttributes().exceptions);
+ }
+
+ @Override
public boolean isKeepEnclosingMethodAttributeEnabled() {
return proguardConfiguration.getKeepAttributes().enclosingMethod;
}
diff --git a/src/test/java/com/android/tools/r8/shaking/attributes/KeepExceptionsAttributeTest.java b/src/test/java/com/android/tools/r8/shaking/attributes/KeepExceptionsAttributeTest.java
new file mode 100644
index 0000000..43213c0
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/shaking/attributes/KeepExceptionsAttributeTest.java
@@ -0,0 +1,152 @@
+// Copyright (c) 2024, 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.shaking.attributes;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isAbsent;
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import com.android.tools.r8.SingleTestRunResult;
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.TestBuilder;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.TestParametersCollection;
+import com.android.tools.r8.shaking.ProguardKeepAttributes;
+import com.android.tools.r8.utils.StringUtils;
+import com.android.tools.r8.utils.codeinspector.CodeInspector;
+import com.android.tools.r8.utils.codeinspector.MethodSubject;
+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 KeepExceptionsAttributeTest extends TestBase {
+
+ private final TestParameters parameters;
+ private final String[] EXPECTED = new String[] {"A.foo", "A.bar", "A.foo", "A.bar"};
+
+ @Parameters(name = "{0}")
+ public static TestParametersCollection data() {
+ return getTestParameters().withAllRuntimesAndApiLevels().build();
+ }
+
+ public KeepExceptionsAttributeTest(TestParameters parameters) {
+ this.parameters = parameters;
+ }
+
+ @Test
+ public void testRuntime() throws Exception {
+ testForRuntime(parameters)
+ .apply(this::addInputs)
+ .run(parameters.getRuntime(), TestClass.class)
+ .assertSuccessWithOutputLines(EXPECTED);
+ }
+
+ @Test
+ public void testR8() throws Exception {
+ testForR8(parameters.getBackend())
+ .apply(this::addInputs)
+ .addKeepMainRule(TestClass.class)
+ .addKeepRules(
+ StringUtils.lines("-keep class " + typeName(A.class) + " {", " *** foo(...);", "}"))
+ .addKeepAttributes(ProguardKeepAttributes.EXCEPTIONS)
+ .setMinApi(parameters)
+ .run(parameters.getRuntime(), TestClass.class)
+ .assertSuccessWithOutputLines(EXPECTED)
+ .inspect(this::inspect);
+ }
+
+ private void addInputs(TestBuilder<? extends SingleTestRunResult<?>, ?> builder) {
+ builder.addProgramClasses(E1.class, E2.class, I.class, A.class, B.class, TestClass.class);
+ }
+
+ private void inspect(CodeInspector inspector) throws NoSuchMethodException {
+ assertThat(inspector.clazz(E1.class), isPresent());
+ assertThat(inspector.clazz(E2.class), isAbsent());
+ assertThat(inspector.clazz(I.class), isPresent());
+ assertThat(inspector.clazz(A.class), isPresent());
+ assertThat(inspector.clazz(B.class), isPresent());
+ {
+ MethodSubject aFoo = inspector.method(A.class.getMethod("foo"));
+ assertThat(aFoo.getThrowsAnnotation(E1.class), isPresent());
+ }
+ {
+ MethodSubject aBar = inspector.method(A.class.getMethod("bar"));
+ assertThat(aBar.getThrowsAnnotation(E2.class), isAbsent());
+ }
+ {
+ MethodSubject aFoobar = inspector.method(A.class.getMethod("foobar"));
+ assertThat(aFoobar.getThrowsAnnotation(E1.class), isAbsent());
+ assertThat(aFoobar.getThrowsAnnotation(E2.class), isAbsent());
+ }
+ }
+
+ static class E1 extends Exception {}
+
+ static class E2 extends Exception {}
+
+ interface I {
+ void foo() throws E1;
+
+ void bar() throws E2;
+
+ void foobar() throws E1, E2;
+ }
+
+ static class A implements I {
+
+ public void foo() throws E1 {
+ if (System.nanoTime() < 0) {
+ throw new E1();
+ }
+ System.out.println("A.foo");
+ }
+
+ public void bar() throws E2 {
+ if (System.nanoTime() < 0) {
+ throw new E2();
+ }
+ System.out.println("A.bar");
+ }
+
+ public void foobar() throws E1, E2 {
+ foo();
+ bar();
+ }
+ }
+
+ static class B implements I {
+
+ public void foo() throws E1 {
+ if (System.nanoTime() < 0) {
+ throw new E1();
+ }
+ System.out.println("B.foo");
+ }
+
+ public void bar() throws E2 {
+ if (System.nanoTime() < 0) {
+ throw new E2();
+ }
+ System.out.println("B.bar");
+ }
+
+ public void foobar() throws E1, E2 {
+ foo();
+ bar();
+ }
+ }
+
+ static class TestClass {
+
+ public static void main(String[] args) throws Exception {
+ I i = System.nanoTime() > 0 ? new A() : new B();
+ i.foo();
+ i.bar();
+ i.foobar();
+ }
+ }
+}
diff --git a/src/test/testbase/java/com/android/tools/r8/utils/codeinspector/AbsentMethodSubject.java b/src/test/testbase/java/com/android/tools/r8/utils/codeinspector/AbsentMethodSubject.java
index aeeb8f4..a9ce82e 100644
--- a/src/test/testbase/java/com/android/tools/r8/utils/codeinspector/AbsentMethodSubject.java
+++ b/src/test/testbase/java/com/android/tools/r8/utils/codeinspector/AbsentMethodSubject.java
@@ -108,6 +108,11 @@
}
@Override
+ public AnnotationSubject getThrowsAnnotation(Class<?> clazz) {
+ throw new Unreachable("Cannot get the exceptions attribute/annotation for an absent method");
+ }
+
+ @Override
public ProgramMethod getProgramMethod() {
return null;
}
diff --git a/src/test/testbase/java/com/android/tools/r8/utils/codeinspector/FoundAnnotationSubject.java b/src/test/testbase/java/com/android/tools/r8/utils/codeinspector/FoundAnnotationSubject.java
index 4d67e5d..aec24b6 100644
--- a/src/test/testbase/java/com/android/tools/r8/utils/codeinspector/FoundAnnotationSubject.java
+++ b/src/test/testbase/java/com/android/tools/r8/utils/codeinspector/FoundAnnotationSubject.java
@@ -67,4 +67,9 @@
public DexTypeAnnotation asDexTypeAnnotation() {
return annotation.asTypeAnnotation();
}
+
+ @Override
+ public String toString() {
+ return annotation.toString();
+ }
}
diff --git a/src/test/testbase/java/com/android/tools/r8/utils/codeinspector/FoundMethodSubject.java b/src/test/testbase/java/com/android/tools/r8/utils/codeinspector/FoundMethodSubject.java
index 8cc6ab0..6b28889 100644
--- a/src/test/testbase/java/com/android/tools/r8/utils/codeinspector/FoundMethodSubject.java
+++ b/src/test/testbase/java/com/android/tools/r8/utils/codeinspector/FoundMethodSubject.java
@@ -17,6 +17,7 @@
import com.android.tools.r8.graph.CfCode.LocalVariableInfo;
import com.android.tools.r8.graph.Code;
import com.android.tools.r8.graph.DexAnnotation;
+import com.android.tools.r8.graph.DexAnnotationElement;
import com.android.tools.r8.graph.DexCode;
import com.android.tools.r8.graph.DexDebugEvent;
import com.android.tools.r8.graph.DexDebugInfo;
@@ -27,6 +28,8 @@
import com.android.tools.r8.graph.DexMethod;
import com.android.tools.r8.graph.DexString;
import com.android.tools.r8.graph.DexType;
+import com.android.tools.r8.graph.DexValue;
+import com.android.tools.r8.graph.DexValue.DexValueType;
import com.android.tools.r8.graph.MethodAccessFlags;
import com.android.tools.r8.graph.ProgramMethod;
import com.android.tools.r8.ir.code.IRCode;
@@ -175,6 +178,29 @@
}
@Override
+ public AnnotationSubject getThrowsAnnotation(Class<?> clazz) {
+ ClassSubject exceptionClass = codeInspector.clazz(clazz);
+ if (exceptionClass.isAbsent() || !getMethod().hasAnyAnnotations()) {
+ return new AbsentAnnotationSubject();
+ }
+ for (DexAnnotation annotation : getMethod().annotations().getAnnotations()) {
+ if (DexAnnotation.isThrowsAnnotation(annotation, codeInspector.dexItemFactory)) {
+ for (DexAnnotationElement element : annotation.annotation.elements) {
+ for (DexValue value : element.value.asDexValueArray().getValues()) {
+ DexValueType type = value.asDexValueType();
+ String desc = type.value.toDescriptorString();
+ if (desc.equals(exceptionClass.getOriginalDescriptor())
+ || desc.equals(exceptionClass.getFinalDescriptor())) {
+ return new FoundAnnotationSubject(annotation, codeInspector);
+ }
+ }
+ }
+ }
+ }
+ return new AbsentAnnotationSubject();
+ }
+
+ @Override
public ProgramMethod getProgramMethod() {
return new ProgramMethod(clazz.getDexProgramClass(), getMethod());
}
diff --git a/src/test/testbase/java/com/android/tools/r8/utils/codeinspector/MethodSubject.java b/src/test/testbase/java/com/android/tools/r8/utils/codeinspector/MethodSubject.java
index 02ecc3a..439a306 100644
--- a/src/test/testbase/java/com/android/tools/r8/utils/codeinspector/MethodSubject.java
+++ b/src/test/testbase/java/com/android/tools/r8/utils/codeinspector/MethodSubject.java
@@ -65,6 +65,8 @@
public abstract List<FoundAnnotationSubject> getParameterAnnotations(int index);
+ public abstract AnnotationSubject getThrowsAnnotation(Class<?> clazz);
+
public abstract ProgramMethod getProgramMethod();
public Iterator<InstructionSubject> iterateInstructions() {
