Merge "Put min-sdk guard on 'this' liveness bug workaround."
diff --git a/.gitignore b/.gitignore
index f15c3c1..51e0c02 100644
--- a/.gitignore
+++ b/.gitignore
@@ -45,6 +45,8 @@
third_party/gradle/gradle
third_party/gradle-plugin
third_party/gradle-plugin.tar.gz
+third_party/jacoco/*
+third_party/jacoco.tar.gz
third_party/jasmin.tar.gz
third_party/jasmin
third_party/jdwp-tests.tar.gz
diff --git a/build.gradle b/build.gradle
index f5a3337..4231129 100644
--- a/build.gradle
+++ b/build.gradle
@@ -339,6 +339,7 @@
"gmscore/gmscore_v9",
"gmscore/gmscore_v10",
"gmscore/latest",
+ "jacoco",
"nest/nest_20180926_7c6cfb",
"photos/2017-06-06",
"youtube/youtube.android_12.10",
@@ -1310,17 +1311,23 @@
if (project.property('tool') == 'r8') {
exclude "com/android/tools/r8/art/*/d8/**"
exclude "com/android/tools/r8/jctf/d8/**"
- } else {
- assert(project.property('tool') == 'd8')
+ exclude "com/android/tools/r8/jctf/r8cf/**"
+ } else if (project.property('tool') == 'd8') {
exclude "com/android/tools/r8/art/*/r8/**"
exclude "com/android/tools/r8/jctf/r8/**"
+ exclude "com/android/tools/r8/jctf/r8cf/**"
+ } else {
+ assert(project.property('tool') == 'r8cf')
+ exclude "com/android/tools/r8/art/*/d8/**"
+ exclude "com/android/tools/r8/art/*/r8/**"
+ exclude "com/android/tools/r8/jctf/d8/**"
+ exclude "com/android/tools/r8/jctf/r8/**"
}
}
if (!project.hasProperty('all_tests')) {
exclude "com/android/tools/r8/art/dx/**"
exclude "com/android/tools/r8/art/jack/**"
}
- // TODO(tamaskenez) enable jctf on all_tests when consolidated
if (!project.hasProperty('jctf') && !project.hasProperty('only_jctf')) {
exclude "com/android/tools/r8/jctf/**"
}
diff --git a/src/main/java/com/android/tools/r8/graph/DexTypeList.java b/src/main/java/com/android/tools/r8/graph/DexTypeList.java
index 8df5030..45a1548 100644
--- a/src/main/java/com/android/tools/r8/graph/DexTypeList.java
+++ b/src/main/java/com/android/tools/r8/graph/DexTypeList.java
@@ -66,8 +66,11 @@
@Override
public String toString() {
StringBuilder builder = new StringBuilder();
- for (DexType type : values) {
- builder.append(' ').append(type);
+ if (values.length > 0) {
+ builder.append(values[0]);
+ for (int i = 1; i < values.length; i++) {
+ builder.append(' ').append(values[i]);
+ }
}
return builder.toString();
}
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 0c47df1..d194bb2 100644
--- a/src/main/java/com/android/tools/r8/shaking/AnnotationRemover.java
+++ b/src/main/java/com/android/tools/r8/shaking/AnnotationRemover.java
@@ -11,8 +11,10 @@
import com.android.tools.r8.graph.DexItemFactory;
import com.android.tools.r8.graph.DexProgramClass;
import com.android.tools.r8.graph.DexType;
+import com.android.tools.r8.graph.InnerClassAttribute;
import com.android.tools.r8.shaking.Enqueuer.AppInfoWithLiveness;
import com.android.tools.r8.utils.InternalOptions;
+import java.util.List;
public class AnnotationRemover {
@@ -135,11 +137,36 @@
field.annotations = field.annotations.keepIf(this::filterAnnotations);
}
+ private boolean enclosingMethodPinned(DexClass clazz) {
+ return clazz.getEnclosingMethod() != null
+ && clazz.getEnclosingMethod().getEnclosingClass() != null
+ && appInfo.isPinned(clazz.getEnclosingMethod().getEnclosingClass());
+ }
+
+ private boolean innerClassPinned(DexClass clazz) {
+ List<InnerClassAttribute> innerClasses = clazz.getInnerClasses();
+ for (InnerClassAttribute innerClass : innerClasses) {
+ if (appInfo.isPinned(innerClass.getInner())) {
+ return true;
+ }
+ if (appInfo.isPinned(innerClass.getOuter())) {
+ return true;
+ }
+ }
+ return false;
+ }
+
private void stripAttributes(DexProgramClass clazz) {
// If [clazz] is mentioned by a keep rule, it could be used for reflection, and we therefore
// need to keep the enclosing method and inner classes attributes, if requested. In Proguard
// compatibility mode we keep these attributes independent of whether the given class is kept.
- if (appInfo.isPinned(clazz.type) || options.forceProguardCompatibility) {
+ // To ensure reflection from both inner to outer and and outer to inner for kept classes - even
+ // if only one side is kept - keep the attributes is any class mentioned in these attributes
+ // is kept.
+ if (appInfo.isPinned(clazz.type)
+ || enclosingMethodPinned(clazz)
+ || innerClassPinned(clazz)
+ || options.forceProguardCompatibility) {
if (!keep.enclosingMethod) {
clazz.clearEnclosingMethod();
}
diff --git a/src/main/java/com/android/tools/r8/shaking/ProguardClassSpecification.java b/src/main/java/com/android/tools/r8/shaking/ProguardClassSpecification.java
index 2aff5df..1d970a3 100644
--- a/src/main/java/com/android/tools/r8/shaking/ProguardClassSpecification.java
+++ b/src/main/java/com/android/tools/r8/shaking/ProguardClassSpecification.java
@@ -230,6 +230,10 @@
return inheritanceIsExtends;
}
+ public boolean getInheritanceIsImplements() {
+ return !inheritanceIsExtends;
+ }
+
public boolean hasInheritanceClassName() {
return inheritanceClassName != null;
}
diff --git a/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java b/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java
index e117462..432ae10 100644
--- a/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java
+++ b/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java
@@ -47,7 +47,6 @@
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Future;
-import java.util.function.Function;
import java.util.function.Predicate;
import java.util.stream.Collectors;
@@ -80,7 +79,7 @@
public RootSetBuilder(
AppView<? extends AppInfo> appView,
DexApplication application,
- List<ProguardConfigurationRule> rules,
+ Collection<? extends ProguardConfigurationRule> rules,
InternalOptions options) {
this.appView = appView;
this.application = application.asDirect();
@@ -89,64 +88,10 @@
}
RootSetBuilder(
- AppView<? extends AppInfo> appView, Set<ProguardIfRule> ifRules, InternalOptions options) {
- this.appView = appView;
- this.application = appView.appInfo().app.asDirect();
- this.rules = Collections.unmodifiableCollection(ifRules);
- this.options = options;
- }
-
- private boolean anySuperTypeMatches(
- DexType type,
- Function<DexType, DexClass> definitionFor,
- ProguardTypeMatcher name,
- ProguardTypeMatcher annotation) {
- while (type != null) {
- DexClass clazz = definitionFor.apply(type);
- if (clazz == null) {
- // TODO(herhut): Warn about broken supertype chain?
- return false;
- }
- // TODO(b/110141157): Should the vertical class merger move annotations from the source to
- // the target class? If so, it is sufficient only to apply the annotation-matcher to the
- // annotations of `class`.
- if (name.matches(clazz.type, appView) && containsAnnotation(annotation, clazz.annotations)) {
- return true;
- }
- type = clazz.superType;
- }
- return false;
- }
-
- private boolean anyImplementedInterfaceMatches(
- DexClass clazz,
- Function<DexType, DexClass> definitionFor,
- ProguardTypeMatcher className,
- ProguardTypeMatcher annotation) {
- if (clazz == null) {
- return false;
- }
- for (DexType iface : clazz.interfaces.values) {
- DexClass ifaceClass = definitionFor.apply(iface);
- if (ifaceClass == null) {
- // TODO(herhut): Warn about broken supertype chain?
- return false;
- }
- // TODO(herhut): Maybe it would be better to do this breadth first.
- if ((className.matches(iface) && containsAnnotation(annotation, ifaceClass.annotations))
- || anyImplementedInterfaceMatches(ifaceClass, definitionFor, className, annotation)) {
- return true;
- }
- }
- if (clazz.superType == null) {
- return false;
- }
- DexClass superClass = definitionFor.apply(clazz.superType);
- if (superClass == null) {
- // TODO(herhut): Warn about broken supertype chain?
- return false;
- }
- return anyImplementedInterfaceMatches(superClass, definitionFor, className, annotation);
+ AppView<? extends AppInfo> appView,
+ Collection<ProguardIfRule> ifRules,
+ InternalOptions options) {
+ this(appView, appView.appInfo().app, ifRules, options);
}
// Process a class with the keep rule.
@@ -168,8 +113,7 @@
// seems not to care, so users have started to use this inconsistently. We are thus
// inconsistent, as well, but tell them.
// TODO(herhut): One day make this do what it says.
- if (rule.hasInheritanceClassName()
- && !satisfyInheritanceRule(clazz, application::definitionFor, rule)) {
+ if (rule.hasInheritanceClassName() && !satisfyInheritanceRule(clazz, rule)) {
return;
}
@@ -180,7 +124,7 @@
switch (((ProguardKeepRule) rule).getType()) {
case KEEP_CLASS_MEMBERS: {
// Members mentioned at -keepclassmembers always depend on their holder.
- preconditionSupplier = ImmutableMap.of((definition -> true), clazz);
+ preconditionSupplier = ImmutableMap.of(definition -> true, clazz);
markMatchingVisibleMethods(clazz, memberKeepRules, rule, preconditionSupplier);
markMatchingFields(clazz, memberKeepRules, rule, preconditionSupplier);
break;
@@ -395,7 +339,7 @@
if (rule.hasInheritanceClassName()) {
// Note that, in presence of vertical class merging, we check if the resulting class
// (i.e., the target class) satisfies the implements/extends-matcher.
- if (!satisfyInheritanceRule(targetClass, this::definitionForWithLiveTypes, rule)) {
+ if (!satisfyInheritanceRule(targetClass, rule)) {
// Try another live type since the current one doesn't satisfy the inheritance rule.
return;
}
@@ -458,12 +402,6 @@
ProguardIfRule materializedRule = rule.materialize();
runPerRule(executorService, futures, materializedRule.subsequentRule, materializedRule);
}
-
- private DexClass definitionForWithLiveTypes(DexType type) {
- assert appView.verticallyMergedClasses() == null
- || !appView.verticallyMergedClasses().hasBeenMergedIntoSubtype(type);
- return liveTypes.contains(type) ? appView.appInfo().definitionFor(type) : null;
- }
}
private static DexDefinition testAndGetPrecondition(
@@ -593,27 +531,23 @@
return containsAnnotation(rule.getClassAnnotation(), clazz.annotations);
}
- private boolean satisfyInheritanceRule(
- DexClass clazz, Function<DexType, DexClass> definitionFor, ProguardConfigurationRule rule) {
- ProguardTypeMatcher inheritanceClassName = rule.getInheritanceClassName();
- ProguardTypeMatcher inheritanceAnnotation = rule.getInheritanceAnnotation();
- boolean extendsExpected =
- satisfyExtendsRule(clazz, definitionFor, inheritanceClassName, inheritanceAnnotation);
+ private boolean satisfyInheritanceRule(DexClass clazz, ProguardConfigurationRule rule) {
+ boolean extendsExpected = satisfyExtendsRule(clazz, rule);
boolean implementsExpected = false;
if (!extendsExpected) {
- implementsExpected =
- anyImplementedInterfaceMatches(
- clazz, definitionFor, inheritanceClassName, inheritanceAnnotation);
+ implementsExpected = satisfyImplementsRule(clazz, rule);
}
if (extendsExpected || implementsExpected) {
// Warn if users got it wrong, but only warn once.
if (rule.getInheritanceIsExtends()) {
if (implementsExpected && rulesThatUseExtendsOrImplementsWrong.add(rule)) {
+ assert options.testing.allowProguardRulesThatUseExtendsOrImplementsWrong;
options.reporter.warning(
new StringDiagnostic(
"The rule `" + rule + "` uses extends but actually matches implements."));
}
} else if (extendsExpected && rulesThatUseExtendsOrImplementsWrong.add(rule)) {
+ assert options.testing.allowProguardRulesThatUseExtendsOrImplementsWrong;
options.reporter.warning(
new StringDiagnostic(
"The rule `" + rule + "` uses implements but actually matches extends."));
@@ -623,27 +557,90 @@
return false;
}
- private boolean satisfyExtendsRule(
- DexClass clazz,
- Function<DexType, DexClass> definitionFor,
- ProguardTypeMatcher inheritanceClassName,
- ProguardTypeMatcher inheritanceAnnotation) {
- if (anySuperTypeMatches(
- clazz.superType, definitionFor, inheritanceClassName, inheritanceAnnotation)) {
+ private boolean satisfyExtendsRule(DexClass clazz, ProguardConfigurationRule rule) {
+ if (anySuperTypeMatchesExtendsRule(clazz.superType, rule)) {
return true;
}
-
// It is possible that this class used to inherit from another class X, but no longer does it,
// because X has been merged into `clazz`.
- if (appView.verticallyMergedClasses() != null) {
- // TODO(b/110141157): Figure out what to do with annotations. Should the annotations of
- // the DexClass corresponding to `sourceType` satisfy the `annotation`-matcher?
- return appView.verticallyMergedClasses().getSourcesFor(clazz.type).stream()
- .anyMatch(inheritanceClassName::matches);
+ return anySourceMatchesInheritanceRuleDirectly(clazz, rule, false);
+ }
+
+ private boolean anySuperTypeMatchesExtendsRule(DexType type, ProguardConfigurationRule rule) {
+ while (type != null) {
+ DexClass clazz = application.definitionFor(type);
+ if (clazz == null) {
+ // TODO(herhut): Warn about broken supertype chain?
+ return false;
+ }
+ // TODO(b/110141157): Should the vertical class merger move annotations from the source to
+ // the target class? If so, it is sufficient only to apply the annotation-matcher to the
+ // annotations of `class`.
+ if (rule.getInheritanceClassName().matches(clazz.type, appView)
+ && containsAnnotation(rule.getInheritanceAnnotation(), clazz.annotations)) {
+ return true;
+ }
+ type = clazz.superType;
}
return false;
}
+ private boolean satisfyImplementsRule(DexClass clazz, ProguardConfigurationRule rule) {
+ if (anyImplementedInterfaceMatchesImplementsRule(clazz, rule)) {
+ return true;
+ }
+ // It is possible that this class used to implement an interface I, but no longer does it,
+ // because I has been merged into `clazz`.
+ return anySourceMatchesInheritanceRuleDirectly(clazz, rule, true);
+ }
+
+ private boolean anyImplementedInterfaceMatchesImplementsRule(
+ DexClass clazz, ProguardConfigurationRule rule) {
+ // TODO(herhut): Maybe it would be better to do this breadth first.
+ if (clazz == null) {
+ return false;
+ }
+ for (DexType iface : clazz.interfaces.values) {
+ DexClass ifaceClass = application.definitionFor(iface);
+ if (ifaceClass == null) {
+ // TODO(herhut): Warn about broken supertype chain?
+ return false;
+ }
+ // TODO(b/110141157): Should the vertical class merger move annotations from the source to
+ // the target class? If so, it is sufficient only to apply the annotation-matcher to the
+ // annotations of `ifaceClass`.
+ if (rule.getInheritanceClassName().matches(iface, appView)
+ && containsAnnotation(rule.getInheritanceAnnotation(), ifaceClass.annotations)) {
+ return true;
+ }
+ if (anyImplementedInterfaceMatchesImplementsRule(ifaceClass, rule)) {
+ return true;
+ }
+ }
+ if (clazz.superType == null) {
+ return false;
+ }
+ DexClass superClass = application.definitionFor(clazz.superType);
+ if (superClass == null) {
+ // TODO(herhut): Warn about broken supertype chain?
+ return false;
+ }
+ return anyImplementedInterfaceMatchesImplementsRule(superClass, rule);
+ }
+
+ private boolean anySourceMatchesInheritanceRuleDirectly(
+ DexClass clazz, ProguardConfigurationRule rule, boolean isInterface) {
+ // TODO(b/110141157): Figure out what to do with annotations. Should the annotations of
+ // the DexClass corresponding to `sourceType` satisfy the `annotation`-matcher?
+ return appView.verticallyMergedClasses() != null
+ && appView.verticallyMergedClasses().getSourcesFor(clazz.type).stream()
+ .filter(
+ sourceType ->
+ appView.appInfo().definitionFor(sourceType).accessFlags.isInterface()
+ == isInterface)
+ .anyMatch(rule.getInheritanceClassName()::matches);
+ }
+
private boolean allRulesSatisfied(Collection<ProguardMemberRule> memberKeepRules,
DexClass clazz) {
for (ProguardMemberRule rule : memberKeepRules) {
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 555bfc2..65d1e3a 100644
--- a/src/main/java/com/android/tools/r8/utils/InternalOptions.java
+++ b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
@@ -453,6 +453,7 @@
? NondeterministicIROrdering.getInstance()
: IdentityIROrdering.getInstance();
+ public boolean allowProguardRulesThatUseExtendsOrImplementsWrong = true;
public boolean alwaysUsePessimisticRegisterAllocation = false;
public boolean invertConditionals = false;
public boolean placeExceptionalBlocksLast = false;
diff --git a/src/test/java/com/android/tools/r8/JacocoRegressionTest.java b/src/test/java/com/android/tools/r8/JacocoRegressionTest.java
new file mode 100644
index 0000000..235b7c6
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/JacocoRegressionTest.java
@@ -0,0 +1,115 @@
+// Copyright (c) 2018, 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;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+import com.android.tools.r8.ClassFileConsumer.ArchiveConsumer;
+import com.android.tools.r8.ToolHelper.ProcessResult;
+import com.android.tools.r8.utils.DescriptorUtils;
+import java.nio.file.Path;
+import org.junit.Test;
+import org.objectweb.asm.AnnotationVisitor;
+import org.objectweb.asm.ClassWriter;
+import org.objectweb.asm.FieldVisitor;
+import org.objectweb.asm.Label;
+import org.objectweb.asm.MethodVisitor;
+import org.objectweb.asm.Opcodes;
+
+/** Jacoco does invalid instrumentation when R8 clobber locals of arguments: TODO(b/117589870) */
+public class JacocoRegressionTest extends TestBase implements Opcodes {
+
+ @Test
+ public void test() throws Exception {
+ Path output = temp.newFolder().toPath();
+ String name = "Test";
+ String desc = DescriptorUtils.javaTypeToDescriptor(name);
+ byte[] bytes = dump();
+ Path path = output.resolve("out.jar");
+ ArchiveConsumer archiveConsumer = new ArchiveConsumer(path);
+ archiveConsumer.accept(ByteDataView.of(bytes), desc, null);
+ archiveConsumer.finished(null);
+
+ String expected = "15" + System.lineSeparator();
+ ProcessResult result = ToolHelper.runJava(path, name);
+ assertEquals(expected, result.stdout);
+
+ Path agentOutput = output.resolve("agent.out");
+ ProcessResult result1 =
+ ToolHelper.runJava(
+ path,
+ String.format(
+ "-javaagent:%s=destfile=%s,dumponexit=true,output=file",
+ ToolHelper.JACOCO_AGENT, agentOutput),
+ name);
+ assertEquals(1, result1.exitCode);
+ assertTrue(result1.toString().contains("java.lang.VerifyError: Bad local variable type"));
+ }
+
+ public static byte[] dump() throws Exception {
+
+ ClassWriter classWriter = new ClassWriter(0);
+ FieldVisitor fieldVisitor;
+ MethodVisitor methodVisitor;
+ AnnotationVisitor annotationVisitor0;
+
+ classWriter.visit(
+ V1_8, ACC_FINAL | ACC_SUPER | ACC_PUBLIC, "Test", null, "java/lang/Object", null);
+ classWriter.visitSource("Test.java", null);
+
+ {
+ methodVisitor = classWriter.visitMethod(ACC_PRIVATE, "<init>", "()V", null, null);
+ methodVisitor.visitCode();
+ Label label0 = new Label();
+ methodVisitor.visitLabel(label0);
+ methodVisitor.visitLineNumber(32, label0);
+ methodVisitor.visitVarInsn(ALOAD, 0);
+ methodVisitor.visitMethodInsn(INVOKESPECIAL, "java/lang/Object", "<init>", "()V", false);
+ methodVisitor.visitInsn(RETURN);
+ Label label1 = new Label();
+ methodVisitor.visitLabel(label1);
+ methodVisitor.visitLocalVariable("this", "LTest;", null, label0, label1, 0);
+ methodVisitor.visitMaxs(1, 1);
+ methodVisitor.visitEnd();
+ }
+
+ // This is the problematic part for Jacoco, where we clobber local 1 with long_2nd
+ {
+ methodVisitor = classWriter.visitMethod(ACC_STATIC, "foo", "(I)I", null, null);
+ methodVisitor.visitCode();
+ methodVisitor.visitVarInsn(ILOAD, 0);
+ methodVisitor.visitInsn(I2L);
+ methodVisitor.visitVarInsn(LSTORE, 0);
+ methodVisitor.visitIntInsn(BIPUSH, 15);
+ methodVisitor.visitInsn(IRETURN);
+ methodVisitor.visitMaxs(4, 4);
+ methodVisitor.visitEnd();
+ }
+
+ {
+ methodVisitor =
+ classWriter.visitMethod(
+ ACC_PUBLIC | ACC_STATIC, "main", "([Ljava/lang/String;)V", null, null);
+ methodVisitor.visitCode();
+ Label label0 = new Label();
+ methodVisitor.visitLabel(label0);
+ methodVisitor.visitLineNumber(6, label0);
+ methodVisitor.visitFieldInsn(GETSTATIC, "java/lang/System", "out", "Ljava/io/PrintStream;");
+ methodVisitor.visitIntInsn(BIPUSH, 42);
+ methodVisitor.visitMethodInsn(INVOKESTATIC, "Test", "foo", "(I)I", false);
+ methodVisitor.visitMethodInsn(INVOKEVIRTUAL, "java/io/PrintStream", "println", "(I)V", false);
+ Label label1 = new Label();
+ methodVisitor.visitLabel(label1);
+ methodVisitor.visitLineNumber(7, label1);
+ methodVisitor.visitInsn(RETURN);
+ methodVisitor.visitMaxs(2, 1);
+ methodVisitor.visitEnd();
+ }
+
+ classWriter.visitEnd();
+
+ return classWriter.toByteArray();
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/TestRunResult.java b/src/test/java/com/android/tools/r8/TestRunResult.java
index 462ca1f..91c01c4 100644
--- a/src/test/java/com/android/tools/r8/TestRunResult.java
+++ b/src/test/java/com/android/tools/r8/TestRunResult.java
@@ -11,6 +11,7 @@
import com.android.tools.r8.utils.AndroidApp;
import com.android.tools.r8.utils.codeinspector.CodeInspector;
import java.io.IOException;
+import java.io.PrintStream;
import java.util.concurrent.ExecutionException;
public class TestRunResult {
@@ -47,22 +48,43 @@
private String errorMessage(String message) {
StringBuilder builder = new StringBuilder(message).append('\n');
- printInfo(builder);
+ appendInfo(builder);
return builder.toString();
}
- private void printInfo(StringBuilder builder) {
+ private void appendInfo(StringBuilder builder) {
builder.append("APPLICATION: ");
- printApplication(builder);
+ appendApplication(builder);
builder.append('\n');
- printProcessResult(builder);
+ appendProcessResult(builder);
}
- private void printApplication(StringBuilder builder) {
+ private void appendApplication(StringBuilder builder) {
builder.append(app == null ? "<default>" : app.toString());
}
- private void printProcessResult(StringBuilder builder) {
+ private void appendProcessResult(StringBuilder builder) {
builder.append("COMMAND: ").append(result.command).append('\n').append(result);
}
+
+ public TestRunResult writeInfo(PrintStream ps) {
+ StringBuilder sb = new StringBuilder();
+ appendInfo(sb);
+ ps.println(sb.toString());
+ return this;
+ }
+
+ public TestRunResult writeApplicaion(PrintStream ps) {
+ StringBuilder sb = new StringBuilder();
+ appendApplication(sb);
+ ps.println(sb.toString());
+ return this;
+ }
+
+ public TestRunResult writeProcessResult(PrintStream ps) {
+ StringBuilder sb = new StringBuilder();
+ appendProcessResult(sb);
+ ps.println(sb.toString());
+ return this;
+ }
}
diff --git a/src/test/java/com/android/tools/r8/ToolHelper.java b/src/test/java/com/android/tools/r8/ToolHelper.java
index 8ae0dad..fa8ca13 100644
--- a/src/test/java/com/android/tools/r8/ToolHelper.java
+++ b/src/test/java/com/android/tools/r8/ToolHelper.java
@@ -104,6 +104,7 @@
private static final String PROGUARD5_2_1 = "third_party/proguard/proguard5.2.1/bin/proguard";
private static final String PROGUARD6_0_1 = "third_party/proguard/proguard6.0.1/bin/proguard";
private static final String PROGUARD = PROGUARD5_2_1;
+ public static final String JACOCO_AGENT = "third_party/jacoco/org.jacoco.agent-0.8.2-runtime.jar";
private static final String RETRACE6_0_1 = "third_party/proguard/proguard6.0.1/bin/retrace";
private static final String RETRACE = RETRACE6_0_1;
diff --git a/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/IfRuleWithVerticalClassMerging.java b/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/IfRuleWithVerticalClassMerging.java
index 18783750..b026c31 100644
--- a/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/IfRuleWithVerticalClassMerging.java
+++ b/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/IfRuleWithVerticalClassMerging.java
@@ -60,7 +60,6 @@
// TODO(b/110141157):
// - Add tests where fields and methods get renamed due to naming conflicts.
-// - Add tests where the type in a implements clause has changed.
// - Add tests where the then-clause of an -if rule keeps a class that has been merged into another.
@RunWith(Parameterized.class)
public class IfRuleWithVerticalClassMerging extends TestBase {
diff --git a/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/ImplementsMergedTypeDirectlyTest.java b/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/ImplementsMergedTypeDirectlyTest.java
new file mode 100644
index 0000000..6e8b394
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/ImplementsMergedTypeDirectlyTest.java
@@ -0,0 +1,58 @@
+// Copyright (c) 2018, 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.ifrule.verticalclassmerging;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.core.IsNot.not;
+import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
+
+import com.android.tools.r8.utils.codeinspector.ClassSubject;
+import com.android.tools.r8.utils.codeinspector.CodeInspector;
+
+public class ImplementsMergedTypeDirectlyTest extends MergedTypeBaseTest {
+
+ static class TestClass implements K {
+
+ public static void main(String[] args) {
+ System.out.print("Hello world");
+ }
+ }
+
+ public ImplementsMergedTypeDirectlyTest(Backend backend, boolean enableVerticalClassMerging) {
+ super(backend, enableVerticalClassMerging);
+ }
+
+ @Override
+ public Class<?> getTestClass() {
+ return TestClass.class;
+ }
+
+ @Override
+ public String getConditionForProguardIfRule() {
+ // After class merging, TestClass will no longer implement K, but we should still keep the
+ // class Unused in the output.
+ return "-if class **$TestClass implements **$K";
+ }
+
+ @Override
+ public String getExpectedStdout() {
+ return "Hello world";
+ }
+
+ public void inspect(CodeInspector inspector) {
+ super.inspect(inspector);
+
+ if (enableVerticalClassMerging) {
+ // Check that TestClass no longer implements K.
+ ClassSubject testClassSubject = inspector.clazz(TestClass.class);
+ assertThat(testClassSubject, isPresent());
+ assertTrue(testClassSubject.getDexClass().interfaces.isEmpty());
+
+ // Check that K is no longer present.
+ assertThat(inspector.clazz(K.class), not(isPresent()));
+ }
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/ImplementsMergedTypeIndirectlyTest.java b/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/ImplementsMergedTypeIndirectlyTest.java
new file mode 100644
index 0000000..4583978
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/ImplementsMergedTypeIndirectlyTest.java
@@ -0,0 +1,55 @@
+// Copyright (c) 2018, 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.ifrule.verticalclassmerging;
+
+import static org.junit.Assert.assertEquals;
+
+import com.android.tools.r8.utils.codeinspector.ClassSubject;
+import com.android.tools.r8.utils.codeinspector.CodeInspector;
+
+public class ImplementsMergedTypeIndirectlyTest extends MergedTypeBaseTest {
+
+ static class TestClass implements J {
+
+ public static void main(String[] args) {
+ System.out.print("Hello world");
+ }
+ }
+
+ public ImplementsMergedTypeIndirectlyTest(Backend backend, boolean enableVerticalClassMerging) {
+ super(backend, enableVerticalClassMerging);
+ }
+
+ @Override
+ public Class<?> getTestClass() {
+ return TestClass.class;
+ }
+
+ @Override
+ public String getAdditionalKeepRules() {
+ // Keep interface J to prevent it from being merged into TestClass.
+ return "-keep class **$J";
+ }
+
+ @Override
+ public String getConditionForProguardIfRule() {
+ // After class merging, J will no longer extend I (and therefore, TestClass will no longer
+ // implement I indirectly), but we should still keep the class Unused in the output.
+ return "-if class **$TestClass implements **$I";
+ }
+
+ @Override
+ public String getExpectedStdout() {
+ return "Hello world";
+ }
+
+ public void inspect(CodeInspector inspector) {
+ super.inspect(inspector);
+
+ // Verify that TestClass still implements J.
+ ClassSubject testClassSubject = inspector.clazz(TestClass.class);
+ assertEquals(J.class.getTypeName(), testClassSubject.getDexClass().interfaces.toSourceString());
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/MergedTypeBaseTest.java b/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/MergedTypeBaseTest.java
index d2ce210..ac341eb 100644
--- a/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/MergedTypeBaseTest.java
+++ b/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/MergedTypeBaseTest.java
@@ -28,7 +28,8 @@
public abstract class MergedTypeBaseTest extends TestBase {
private final List<Class> CLASSES =
- ImmutableList.of(A.class, B.class, C.class, Unused.class, getTestClass());
+ ImmutableList.of(
+ A.class, B.class, C.class, I.class, J.class, K.class, Unused.class, getTestClass());
static class A {}
@@ -36,6 +37,12 @@
static class C {}
+ interface I {}
+
+ interface J extends I {}
+
+ interface K {}
+
static class Unused {}
final Backend backend;
@@ -58,6 +65,10 @@
public abstract Class<?> getTestClass();
+ public String getAdditionalKeepRules() {
+ return "";
+ }
+
public abstract String getConditionForProguardIfRule();
public abstract String getExpectedStdout();
@@ -65,9 +76,10 @@
public void inspect(CodeInspector inspector) {
assertThat(inspector.clazz(Unused.class), isPresent());
- // Verify that A is no longer present when vertical class merging is enabled.
+ // Verify that A and I are no longer present when vertical class merging is enabled.
if (enableVerticalClassMerging) {
assertThat(inspector.clazz(A.class), not(isPresent()));
+ assertThat(inspector.clazz(I.class), not(isPresent()));
}
}
@@ -82,7 +94,8 @@
" public static void main(java.lang.String[]);",
"}",
getConditionForProguardIfRule(),
- "-keep class " + Unused.class.getTypeName());
+ "-keep class " + Unused.class.getTypeName(),
+ getAdditionalKeepRules());
AndroidApp output = compileWithR8(readClasses(CLASSES), config, this::configure, backend);
assertEquals(expected, runOnVM(output, getTestClass(), backend));
inspect(new CodeInspector(output));
@@ -92,6 +105,10 @@
options.enableMinification = false;
options.enableVerticalClassMerging = enableVerticalClassMerging;
+ // To ensure that the handling of extends and implements rules work as intended,
+ // and that we don't end up keeping `Unused` only because one of the two implementations work.
+ options.testing.allowProguardRulesThatUseExtendsOrImplementsWrong = false;
+
// TODO(b/110148109): Allow ordinary method inlining when -if rules work with inlining.
options.testing.validInliningReasons = ImmutableSet.of(Reason.FORCE);
}
diff --git a/src/test/java/com/android/tools/r8/shaking/innerclassesenclosingmethod/KeepInnerClassesEnclosingMethodAnnotationsTest.java b/src/test/java/com/android/tools/r8/shaking/innerclassesenclosingmethod/KeepInnerClassesEnclosingMethodAnnotationsTest.java
new file mode 100644
index 0000000..474194e
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/shaking/innerclassesenclosingmethod/KeepInnerClassesEnclosingMethodAnnotationsTest.java
@@ -0,0 +1,141 @@
+// Copyright (c) 2018, 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.innerclassesenclosingmethod;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isMemberClass;
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static com.android.tools.r8.utils.codeinspector.Matchers.isRenamed;
+import static org.hamcrest.CoreMatchers.not;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThat;
+
+import com.android.tools.r8.R8Command;
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.ToolHelper;
+import com.android.tools.r8.origin.Origin;
+import com.android.tools.r8.shaking.innerclassesenclosingmethod.testclasses.Outer;
+import com.android.tools.r8.utils.AndroidApp;
+import com.android.tools.r8.utils.StringUtils;
+import com.android.tools.r8.utils.codeinspector.ClassSubject;
+import com.android.tools.r8.utils.codeinspector.CodeInspector;
+import com.google.common.collect.ImmutableList;
+import java.util.List;
+import org.junit.Test;
+
+class Main {
+
+ public static void main(String[] args) {
+ Class<?>[] declaredClasses = Outer.class.getDeclaredClasses();
+ if (declaredClasses.length == 0) {
+ System.out.println("No declared classes");
+ } else {
+ for (int i = 0; i < declaredClasses.length; i++) {
+ System.out.println("Declared class: " + declaredClasses[i].getName());
+ }
+ }
+ if (Outer.Inner.class.getDeclaringClass() == null) {
+ System.out.println("No declaring classes");
+ } else {
+ System.out.println("Declaring class: " + Outer.Inner.class.getDeclaringClass().getName());
+ }
+ }
+}
+
+public class KeepInnerClassesEnclosingMethodAnnotationsTest extends TestBase {
+ private static class TestResult {
+
+ public final AndroidApp app;
+ public final CodeInspector inspector;
+ public final ClassSubject outer;
+ public final ClassSubject inner;
+
+ TestResult(AndroidApp app) throws Throwable {
+ this.app = app;
+ this.inspector = new CodeInspector(app);
+ this.outer = inspector.clazz(Outer.class);
+ this.inner = inspector.clazz(Outer.Inner.class);
+ assertThat(outer, isPresent());
+ assertThat(inner, isPresent());
+ }
+ }
+
+ private TestResult runTest(List<String> proguardConfiguration) throws Throwable {
+ return new TestResult(
+ ToolHelper.runR8(
+ R8Command.builder()
+ .addProgramFiles(ToolHelper.getClassFilesForTestPackage(Outer.class.getPackage()))
+ .addClassProgramData(ToolHelper.getClassAsBytes(Main.class), Origin.unknown())
+ .addProguardConfiguration(proguardConfiguration, Origin.unknown())
+ .setProgramConsumer(emptyConsumer(Backend.DEX))
+ .build()));
+ }
+
+ private void noInnerClassesEnclosingMethodInformation(TestResult result) throws Throwable {
+ List<String> lines = StringUtils.splitLines(runOnArt(result.app, Main.class));
+ assertEquals(2, lines.size());
+ assertEquals("No declared classes", lines.get(0));
+ assertEquals("No declaring classes", lines.get(1));
+ }
+
+ private void fullInnerClassesEnclosingMethodInformation(TestResult result) throws Throwable {
+ List<String> lines = StringUtils.splitLines(runOnArt(result.app, Main.class));
+ assertEquals(2, lines.size());
+ assertEquals("Declared class: " + result.inner.getFinalName(), lines.get(0));
+ assertEquals("Declaring class: " + result.outer.getFinalName(), lines.get(1));
+ }
+
+ @Test
+ public void testKeepAll() throws Throwable {
+ TestResult result =
+ runTest(
+ ImmutableList.of(
+ "-keepattributes InnerClasses,EnclosingMethod",
+ "-keep class **Outer*",
+ keepMainProguardConfiguration(Main.class)));
+ assertThat(result.outer, not(isRenamed()));
+ assertThat(result.inner, not(isRenamed()));
+ assertThat(result.inner, isMemberClass());
+ fullInnerClassesEnclosingMethodInformation(result);
+ }
+
+ @Test
+ public void testKeepAllWithoutAttributes() throws Throwable {
+ TestResult result =
+ runTest(
+ ImmutableList.of("-keep class **Outer*", keepMainProguardConfiguration(Main.class)));
+ assertThat(result.outer, not(isRenamed()));
+ assertThat(result.inner, not(isRenamed()));
+ assertThat(result.inner, not(isMemberClass()));
+ noInnerClassesEnclosingMethodInformation(result);
+ }
+
+ @Test
+ public void testKeepInner() throws Throwable {
+ TestResult result =
+ runTest(
+ ImmutableList.of(
+ "-keepattributes InnerClasses,EnclosingMethod",
+ "-keep class **Outer$Inner",
+ keepMainProguardConfiguration(Main.class)));
+ assertThat(result.outer, isRenamed());
+ assertThat(result.inner, not(isRenamed()));
+ assertThat(result.inner, isMemberClass());
+ fullInnerClassesEnclosingMethodInformation(result);
+ }
+
+ @Test
+ public void testKeepOuter() throws Throwable {
+ TestResult result =
+ runTest(
+ ImmutableList.of(
+ "-keepattributes InnerClasses,EnclosingMethod",
+ "-keep class **Outer",
+ keepMainProguardConfiguration(Main.class)));
+ assertThat(result.outer, not(isRenamed()));
+ assertThat(result.inner, isRenamed());
+ assertThat(result.inner, isMemberClass());
+ fullInnerClassesEnclosingMethodInformation(result);
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/shaking/innerclassesenclosingmethod/testclasses/Outer.java b/src/test/java/com/android/tools/r8/shaking/innerclassesenclosingmethod/testclasses/Outer.java
new file mode 100644
index 0000000..064583f
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/shaking/innerclassesenclosingmethod/testclasses/Outer.java
@@ -0,0 +1,10 @@
+// Copyright (c) 2018, 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.innerclassesenclosingmethod.testclasses;
+
+public class Outer {
+
+ public static class Inner {}
+}
diff --git a/src/test/java/com/android/tools/r8/utils/codeinspector/Matchers.java b/src/test/java/com/android/tools/r8/utils/codeinspector/Matchers.java
index 3b718a4..7f310ed 100644
--- a/src/test/java/com/android/tools/r8/utils/codeinspector/Matchers.java
+++ b/src/test/java/com/android/tools/r8/utils/codeinspector/Matchers.java
@@ -115,6 +115,25 @@
};
}
+ public static Matcher<ClassSubject> isMemberClass() {
+ return new TypeSafeMatcher<ClassSubject>() {
+ @Override
+ public boolean matchesSafely(final ClassSubject clazz) {
+ return clazz.isMemberClass();
+ }
+
+ @Override
+ public void describeTo(final Description description) {
+ description.appendText("is member class");
+ }
+
+ @Override
+ public void describeMismatchSafely(final ClassSubject clazz, Description description) {
+ description.appendText("class ").appendValue(clazz.getOriginalName()).appendText(" is not");
+ }
+ };
+ }
+
public static Matcher<MethodSubject> isAbstract() {
return new TypeSafeMatcher<MethodSubject>() {
@Override
diff --git a/third_party/jacoco.tar.gz.sha1 b/third_party/jacoco.tar.gz.sha1
new file mode 100644
index 0000000..8a0462f
--- /dev/null
+++ b/third_party/jacoco.tar.gz.sha1
@@ -0,0 +1 @@
+b968df99f88434e2a2a35445ac860ee6a2fa16e3
\ No newline at end of file
diff --git a/tools/disasm.py b/tools/disasm.py
index 0d2599a..a9a5a0a 100755
--- a/tools/disasm.py
+++ b/tools/disasm.py
@@ -7,4 +7,4 @@
import toolhelper
if __name__ == '__main__':
- sys.exit(toolhelper.run('disasm', sys.argv[1:]))
+ sys.exit(toolhelper.run('disasm', sys.argv[1:]), debug=False)
diff --git a/tools/test.py b/tools/test.py
index 72e7e01..cd9b70f 100755
--- a/tools/test.py
+++ b/tools/test.py
@@ -52,14 +52,14 @@
help='Print a line before a tests starts and after it ends to stdout.',
default=False, action='store_true')
result.add_option('--tool',
- help='Tool to run ART tests with: "r8" (default) or "d8". Ignored if'
- ' "--all_tests" enabled.',
- default=None, choices=["r8", "d8"])
+ help='Tool to run ART tests with: "r8" (default) or "d8" or "r8cf"'
+ ' (r8 w/CF-backend). Ignored if "--all_tests" enabled.',
+ default=None, choices=["r8", "d8", "r8cf"])
result.add_option('--jctf',
- help='Run JCTF tests with: "r8" (default) or "d8".',
+ help='Run JCTF tests with: "r8" (default) or "d8" or "r8cf".',
default=False, action='store_true')
result.add_option('--only-jctf', '--only_jctf',
- help='Run only JCTF tests with: "r8" (default) or "d8".',
+ help='Run only JCTF tests with: "r8" (default) or "d8" or "r8cf".',
default=False, action='store_true')
result.add_option('--jctf-compile-only', '--jctf_compile_only',
help="Don't run, only compile JCTF tests.",