Fix access flags for merged virtual methods
Bug: 176819913
Change-Id: Iaa15d53aa93e2c72a8552f9ab722830dff52df58
diff --git a/src/main/java/com/android/tools/r8/graph/AccessFlags.java b/src/main/java/com/android/tools/r8/graph/AccessFlags.java
index 98806f6..70d7cdb 100644
--- a/src/main/java/com/android/tools/r8/graph/AccessFlags.java
+++ b/src/main/java/com/android/tools/r8/graph/AccessFlags.java
@@ -320,6 +320,33 @@
return self();
}
+ public B setPrivate(boolean value) {
+ if (value) {
+ flags.setPrivate();
+ } else {
+ flags.unsetPrivate();
+ }
+ return self();
+ }
+
+ public B setProtected(boolean value) {
+ if (value) {
+ flags.setProtected();
+ } else {
+ flags.unsetProtected();
+ }
+ return self();
+ }
+
+ public B setPublic(boolean value) {
+ if (value) {
+ flags.setPublic();
+ } else {
+ flags.unsetPublic();
+ }
+ return self();
+ }
+
public B setStatic() {
flags.setStatic();
return self();
diff --git a/src/main/java/com/android/tools/r8/graph/MethodAccessFlags.java b/src/main/java/com/android/tools/r8/graph/MethodAccessFlags.java
index 61e54e9..7d6eac0 100644
--- a/src/main/java/com/android/tools/r8/graph/MethodAccessFlags.java
+++ b/src/main/java/com/android/tools/r8/graph/MethodAccessFlags.java
@@ -151,6 +151,10 @@
set(Constants.ACC_VARARGS);
}
+ public void unsetVarargs() {
+ unset(Constants.ACC_VARARGS);
+ }
+
public boolean isNative() {
return isSet(Constants.ACC_NATIVE);
}
@@ -179,6 +183,10 @@
set(Constants.ACC_STRICT);
}
+ public void unsetStrict() {
+ unset(Constants.ACC_STRICT);
+ }
+
public boolean isConstructor() {
return isSet(Constants.ACC_CONSTRUCTOR);
}
@@ -207,7 +215,7 @@
set(Constants.ACC_DECLARED_SYNCHRONIZED);
}
- private void unsetDeclaredSynchronized() {
+ public void unsetDeclaredSynchronized() {
unset(Constants.ACC_DECLARED_SYNCHRONIZED);
}
@@ -222,6 +230,24 @@
return this;
}
+ public Builder setStrict(boolean value) {
+ if (value) {
+ flags.setStrict();
+ } else {
+ flags.unsetStrict();
+ }
+ return this;
+ }
+
+ public Builder setSynchronized(boolean value) {
+ if (value) {
+ flags.setSynchronized();
+ } else {
+ flags.unsetSynchronized();
+ }
+ return this;
+ }
+
@Override
public Builder self() {
return this;
diff --git a/src/main/java/com/android/tools/r8/horizontalclassmerging/VirtualMethodMerger.java b/src/main/java/com/android/tools/r8/horizontalclassmerging/VirtualMethodMerger.java
index 24a1b84..e8130ed 100644
--- a/src/main/java/com/android/tools/r8/horizontalclassmerging/VirtualMethodMerger.java
+++ b/src/main/java/com/android/tools/r8/horizontalclassmerging/VirtualMethodMerger.java
@@ -116,16 +116,29 @@
}
private MethodAccessFlags getAccessFlags() {
- // TODO(b/164998929): ensure this behaviour is correct, should probably calculate upper bound
- MethodAccessFlags flags = methods.iterator().next().getAccessFlags().copy();
- if (flags.isAbstract()
- && Iterables.any(methods, method -> !method.getAccessFlags().isAbstract())) {
- flags.unsetAbstract();
+ Iterable<MethodAccessFlags> allFlags =
+ Iterables.transform(methods, ProgramMethod::getAccessFlags);
+ MethodAccessFlags result = allFlags.iterator().next().copy();
+ assert Iterables.all(allFlags, flags -> !flags.isNative());
+ assert !result.isStrict() || Iterables.all(allFlags, MethodAccessFlags::isStrict);
+ assert !result.isSynchronized() || Iterables.all(allFlags, MethodAccessFlags::isSynchronized);
+ if (result.isAbstract() && Iterables.any(allFlags, flags -> !flags.isAbstract())) {
+ result.unsetAbstract();
}
- if (flags.isFinal() && Iterables.any(methods, method -> !method.getAccessFlags().isFinal())) {
- flags.unsetFinal();
+ if (result.isBridge() && Iterables.any(allFlags, flags -> !flags.isBridge())) {
+ result.unsetBridge();
}
- return flags;
+ if (result.isFinal() && Iterables.any(allFlags, flags -> !flags.isFinal())) {
+ result.unsetFinal();
+ }
+ if (result.isSynthetic() && Iterables.any(allFlags, flags -> !flags.isSynthetic())) {
+ result.unsetSynthetic();
+ }
+ if (result.isVarargs() && Iterables.any(allFlags, flags -> !flags.isVarargs())) {
+ result.unsetVarargs();
+ }
+ result.unsetDeclaredSynchronized();
+ return result;
}
private DexMethod getNewMethodReference() {
@@ -181,19 +194,24 @@
}
}
+ DexEncodedMethod newMethod;
if (representative.getHolder() == group.getTarget()) {
- classMethodsBuilder.addVirtualMethod(representative.getDefinition());
+ newMethod = representative.getDefinition();
} else {
// If the method is not in the target type, move it.
OptionalBool isLibraryMethodOverride =
representative.getDefinition().isLibraryMethodOverride();
- classMethodsBuilder.addVirtualMethod(
+ newMethod =
representative
.getDefinition()
.toTypeSubstitutedMethod(
newMethodReference,
- builder -> builder.setIsLibraryMethodOverrideIfKnown(isLibraryMethodOverride)));
+ builder -> builder.setIsLibraryMethodOverrideIfKnown(isLibraryMethodOverride));
}
+
+ newMethod.getAccessFlags().unsetFinal();
+
+ classMethodsBuilder.addVirtualMethod(newMethod);
}
public void merge(
diff --git a/src/main/java/com/android/tools/r8/horizontalclassmerging/policies/PreserveMethodCharacteristics.java b/src/main/java/com/android/tools/r8/horizontalclassmerging/policies/PreserveMethodCharacteristics.java
index 5165bd9..431fdb9 100644
--- a/src/main/java/com/android/tools/r8/horizontalclassmerging/policies/PreserveMethodCharacteristics.java
+++ b/src/main/java/com/android/tools/r8/horizontalclassmerging/policies/PreserveMethodCharacteristics.java
@@ -7,6 +7,7 @@
import com.android.tools.r8.graph.DexEncodedMethod;
import com.android.tools.r8.graph.DexMethodSignature;
import com.android.tools.r8.graph.DexProgramClass;
+import com.android.tools.r8.graph.MethodAccessFlags;
import com.android.tools.r8.horizontalclassmerging.MergeGroup;
import com.android.tools.r8.horizontalclassmerging.MultiClassPolicy;
import com.android.tools.r8.utils.OptionalBool;
@@ -26,17 +27,24 @@
static class MethodCharacteristics {
+ private final MethodAccessFlags accessFlags;
private final OptionalBool isLibraryMethodOverride;
- private final int visibilityOrdinal;
private MethodCharacteristics(DexEncodedMethod method) {
+ this.accessFlags =
+ MethodAccessFlags.builder()
+ .setPrivate(method.getAccessFlags().isPrivate())
+ .setProtected(method.getAccessFlags().isProtected())
+ .setPublic(method.getAccessFlags().isPublic())
+ .setStrict(method.getAccessFlags().isStrict())
+ .setSynchronized(method.getAccessFlags().isSynchronized())
+ .build();
this.isLibraryMethodOverride = method.isLibraryMethodOverride();
- this.visibilityOrdinal = method.getAccessFlags().getVisibilityOrdinal();
}
@Override
public int hashCode() {
- return (visibilityOrdinal << 2) | isLibraryMethodOverride.ordinal();
+ return (accessFlags.hashCode() << 2) | isLibraryMethodOverride.ordinal();
}
@Override
@@ -49,7 +57,7 @@
}
MethodCharacteristics characteristics = (MethodCharacteristics) obj;
return isLibraryMethodOverride == characteristics.isLibraryMethodOverride
- && visibilityOrdinal == characteristics.visibilityOrdinal;
+ && accessFlags.equals(characteristics.accessFlags);
}
}
diff --git a/src/test/java/com/android/tools/r8/classmerging/horizontal/NonFinalOverrideOfFinalMethodTest.java b/src/test/java/com/android/tools/r8/classmerging/horizontal/NonFinalOverrideOfFinalMethodTest.java
new file mode 100644
index 0000000..d73ffcf
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/classmerging/horizontal/NonFinalOverrideOfFinalMethodTest.java
@@ -0,0 +1,98 @@
+// Copyright (c) 2020, 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.classmerging.horizontal;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertFalse;
+
+import com.android.tools.r8.NeverClassInline;
+import com.android.tools.r8.NeverInline;
+import com.android.tools.r8.NoVerticalClassMerging;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.utils.BooleanUtils;
+import com.android.tools.r8.utils.codeinspector.ClassSubject;
+import com.android.tools.r8.utils.codeinspector.MethodSubject;
+import java.util.List;
+import org.junit.Test;
+import org.junit.runners.Parameterized;
+
+public class NonFinalOverrideOfFinalMethodTest extends HorizontalClassMergingTestBase {
+
+ @Parameterized.Parameters(name = "{0}, horizontalClassMerging:{1}")
+ public static List<Object[]> data() {
+ return buildParameters(
+ getTestParameters().withAllRuntimesAndApiLevels().build(), BooleanUtils.trueValues());
+ }
+
+ public NonFinalOverrideOfFinalMethodTest(
+ TestParameters parameters, boolean enableHorizontalClassMerging) {
+ super(parameters, enableHorizontalClassMerging);
+ }
+
+ @Test
+ public void testR8() throws Exception {
+ testForR8(parameters.getBackend())
+ .addInnerClasses(getClass())
+ .addKeepMainRule(Main.class)
+ .addOptionsModification(
+ options ->
+ options.horizontalClassMergerOptions().enableIf(enableHorizontalClassMerging))
+ .enableInliningAnnotations()
+ .enableNeverClassInliningAnnotations()
+ .enableNoVerticalClassMergingAnnotations()
+ .setMinApi(parameters.getApiLevel())
+ .addHorizontallyMergedClassesInspectorIf(
+ enableHorizontalClassMerging, inspector -> inspector.assertMergedInto(B.class, A.class))
+ .compile()
+ .inspect(
+ inspector -> {
+ ClassSubject aClassSubject = inspector.clazz(A.class);
+ assertThat(aClassSubject, isPresent());
+
+ MethodSubject synchronizedMethodSubject = aClassSubject.uniqueMethodWithName("foo");
+ assertThat(synchronizedMethodSubject, isPresent());
+ assertFalse(synchronizedMethodSubject.isFinal());
+ })
+ .run(parameters.getRuntime(), Main.class)
+ .assertSuccessWithOutputLines("A", "B", "BSub");
+ }
+
+ public static class Main {
+ public static void main(String[] args) {
+ new A().foo();
+ new B().bar();
+ new BSub().foo();
+ }
+ }
+
+ @NeverClassInline
+ public static class A {
+
+ @NeverInline
+ public final void foo() {
+ System.out.println("A");
+ }
+ }
+
+ @NeverClassInline
+ @NoVerticalClassMerging
+ public static class B {
+
+ @NeverInline
+ public void bar() {
+ System.out.println("B");
+ }
+ }
+
+ @NeverClassInline
+ public static class BSub extends B {
+
+ @NeverInline
+ public void foo() {
+ System.out.println("BSub");
+ }
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/classmerging/horizontal/StrictMethodMergingTest.java b/src/test/java/com/android/tools/r8/classmerging/horizontal/StrictMethodMergingTest.java
new file mode 100644
index 0000000..762824f
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/classmerging/horizontal/StrictMethodMergingTest.java
@@ -0,0 +1,116 @@
+// Copyright (c) 2020, 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.classmerging.horizontal;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import com.android.tools.r8.NeverClassInline;
+import com.android.tools.r8.NeverInline;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.utils.BooleanUtils;
+import com.android.tools.r8.utils.codeinspector.ClassSubject;
+import com.android.tools.r8.utils.codeinspector.MethodSubject;
+import java.util.List;
+import org.junit.Test;
+import org.junit.runners.Parameterized;
+
+public class StrictMethodMergingTest extends HorizontalClassMergingTestBase {
+
+ @Parameterized.Parameters(name = "{0}, horizontalClassMerging:{1}")
+ public static List<Object[]> data() {
+ return buildParameters(
+ getTestParameters().withAllRuntimesAndApiLevels().build(), BooleanUtils.trueValues());
+ }
+
+ public StrictMethodMergingTest(TestParameters parameters, boolean enableHorizontalClassMerging) {
+ super(parameters, enableHorizontalClassMerging);
+ }
+
+ @Test
+ public void testR8() throws Exception {
+ testForR8(parameters.getBackend())
+ .addInnerClasses(getClass())
+ .addKeepMainRule(Main.class)
+ .addOptionsModification(
+ options ->
+ options.horizontalClassMergerOptions().enableIf(enableHorizontalClassMerging))
+ .enableInliningAnnotations()
+ .enableNeverClassInliningAnnotations()
+ .setMinApi(parameters.getApiLevel())
+ .addHorizontallyMergedClassesInspectorIf(
+ enableHorizontalClassMerging,
+ inspector ->
+ inspector.assertMergedInto(B.class, A.class).assertMergedInto(D.class, C.class))
+ .compile()
+ .inspect(
+ inspector -> {
+ ClassSubject aClassSubject = inspector.clazz(A.class);
+ assertThat(aClassSubject, isPresent());
+
+ MethodSubject synchronizedMethodSubject =
+ aClassSubject.uniqueMethodWithName("m$bridge");
+ assertThat(synchronizedMethodSubject, isPresent());
+ assertTrue(synchronizedMethodSubject.getAccessFlags().isStrict());
+
+ ClassSubject cClassSubject = inspector.clazz(C.class);
+ assertThat(cClassSubject, isPresent());
+
+ MethodSubject unsynchronizedMethodSubject =
+ cClassSubject.uniqueMethodWithName("m$bridge");
+ assertThat(unsynchronizedMethodSubject, isPresent());
+ assertFalse(unsynchronizedMethodSubject.getAccessFlags().isStrict());
+ })
+ .run(parameters.getRuntime(), Main.class)
+ .assertSuccessWithOutputLines("A", "B", "C", "D");
+ }
+
+ public static class Main {
+ public static void main(String[] args) {
+ new A().m();
+ new B().m();
+ new C().m();
+ new D().m();
+ }
+ }
+
+ @NeverClassInline
+ public static class A {
+
+ @NeverInline
+ public strictfp void m() {
+ System.out.println("A");
+ }
+ }
+
+ @NeverClassInline
+ public static class B {
+
+ @NeverInline
+ public strictfp void m() {
+ System.out.println("B");
+ }
+ }
+
+ @NeverClassInline
+ public static class C {
+
+ @NeverInline
+ public void m() {
+ System.out.println("C");
+ }
+ }
+
+ @NeverClassInline
+ public static class D {
+
+ @NeverInline
+ public void m() {
+ System.out.println("D");
+ }
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/classmerging/horizontal/SynchronizedMethodMergingTest.java b/src/test/java/com/android/tools/r8/classmerging/horizontal/SynchronizedMethodMergingTest.java
new file mode 100644
index 0000000..8d802c4
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/classmerging/horizontal/SynchronizedMethodMergingTest.java
@@ -0,0 +1,117 @@
+// Copyright (c) 2020, 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.classmerging.horizontal;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import com.android.tools.r8.NeverClassInline;
+import com.android.tools.r8.NeverInline;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.utils.BooleanUtils;
+import com.android.tools.r8.utils.codeinspector.ClassSubject;
+import com.android.tools.r8.utils.codeinspector.MethodSubject;
+import java.util.List;
+import org.junit.Test;
+import org.junit.runners.Parameterized;
+
+public class SynchronizedMethodMergingTest extends HorizontalClassMergingTestBase {
+
+ @Parameterized.Parameters(name = "{0}, horizontalClassMerging:{1}")
+ public static List<Object[]> data() {
+ return buildParameters(
+ getTestParameters().withAllRuntimesAndApiLevels().build(), BooleanUtils.trueValues());
+ }
+
+ public SynchronizedMethodMergingTest(
+ TestParameters parameters, boolean enableHorizontalClassMerging) {
+ super(parameters, enableHorizontalClassMerging);
+ }
+
+ @Test
+ public void testR8() throws Exception {
+ testForR8(parameters.getBackend())
+ .addInnerClasses(getClass())
+ .addKeepMainRule(Main.class)
+ .addOptionsModification(
+ options ->
+ options.horizontalClassMergerOptions().enableIf(enableHorizontalClassMerging))
+ .enableInliningAnnotations()
+ .enableNeverClassInliningAnnotations()
+ .setMinApi(parameters.getApiLevel())
+ .addHorizontallyMergedClassesInspectorIf(
+ enableHorizontalClassMerging,
+ inspector ->
+ inspector.assertMergedInto(B.class, A.class).assertMergedInto(D.class, C.class))
+ .compile()
+ .inspect(
+ inspector -> {
+ ClassSubject aClassSubject = inspector.clazz(A.class);
+ assertThat(aClassSubject, isPresent());
+
+ MethodSubject synchronizedMethodSubject =
+ aClassSubject.uniqueMethodWithName("m$bridge");
+ assertThat(synchronizedMethodSubject, isPresent());
+ assertTrue(synchronizedMethodSubject.isSynchronized());
+
+ ClassSubject cClassSubject = inspector.clazz(C.class);
+ assertThat(cClassSubject, isPresent());
+
+ MethodSubject unsynchronizedMethodSubject =
+ cClassSubject.uniqueMethodWithName("m$bridge");
+ assertThat(unsynchronizedMethodSubject, isPresent());
+ assertFalse(unsynchronizedMethodSubject.isSynchronized());
+ })
+ .run(parameters.getRuntime(), Main.class)
+ .assertSuccessWithOutputLines("A", "B", "C", "D");
+ }
+
+ public static class Main {
+ public static void main(String[] args) {
+ new A().m();
+ new B().m();
+ new C().m();
+ new D().m();
+ }
+ }
+
+ @NeverClassInline
+ public static class A {
+
+ @NeverInline
+ public synchronized void m() {
+ System.out.println("A");
+ }
+ }
+
+ @NeverClassInline
+ public static class B {
+
+ @NeverInline
+ public synchronized void m() {
+ System.out.println("B");
+ }
+ }
+
+ @NeverClassInline
+ public static class C {
+
+ @NeverInline
+ public void m() {
+ System.out.println("C");
+ }
+ }
+
+ @NeverClassInline
+ public static class D {
+
+ @NeverInline
+ public void m() {
+ System.out.println("D");
+ }
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/utils/codeinspector/AbsentMethodSubject.java b/src/test/java/com/android/tools/r8/utils/codeinspector/AbsentMethodSubject.java
index f81dffd..37f26db 100644
--- a/src/test/java/com/android/tools/r8/utils/codeinspector/AbsentMethodSubject.java
+++ b/src/test/java/com/android/tools/r8/utils/codeinspector/AbsentMethodSubject.java
@@ -6,6 +6,7 @@
import com.android.tools.r8.errors.Unreachable;
import com.android.tools.r8.graph.DexEncodedMethod;
+import com.android.tools.r8.graph.MethodAccessFlags;
import com.android.tools.r8.graph.ProgramMethod;
import com.android.tools.r8.ir.code.IRCode;
import com.android.tools.r8.naming.MemberNaming.MethodSignature;
@@ -94,6 +95,11 @@
}
@Override
+ public MethodAccessFlags getAccessFlags() {
+ throw new Unreachable("Cannot get the access flags for an absent method");
+ }
+
+ @Override
public DexEncodedMethod getMethod() {
return null;
}
diff --git a/src/test/java/com/android/tools/r8/utils/codeinspector/FoundMethodSubject.java b/src/test/java/com/android/tools/r8/utils/codeinspector/FoundMethodSubject.java
index 4d5e0d3..c2ae3ab 100644
--- a/src/test/java/com/android/tools/r8/utils/codeinspector/FoundMethodSubject.java
+++ b/src/test/java/com/android/tools/r8/utils/codeinspector/FoundMethodSubject.java
@@ -26,6 +26,7 @@
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.MethodAccessFlags;
import com.android.tools.r8.graph.ProgramMethod;
import com.android.tools.r8.ir.code.IRCode;
import com.android.tools.r8.naming.MemberNaming;
@@ -142,6 +143,11 @@
}
@Override
+ public MethodAccessFlags getAccessFlags() {
+ return dexMethod.getAccessFlags();
+ }
+
+ @Override
public DexEncodedMethod getMethod() {
return dexMethod;
}
diff --git a/src/test/java/com/android/tools/r8/utils/codeinspector/MethodSubject.java b/src/test/java/com/android/tools/r8/utils/codeinspector/MethodSubject.java
index c6a63e2..4826876 100644
--- a/src/test/java/com/android/tools/r8/utils/codeinspector/MethodSubject.java
+++ b/src/test/java/com/android/tools/r8/utils/codeinspector/MethodSubject.java
@@ -5,6 +5,7 @@
package com.android.tools.r8.utils.codeinspector;
import com.android.tools.r8.graph.DexEncodedMethod;
+import com.android.tools.r8.graph.MethodAccessFlags;
import com.android.tools.r8.graph.ProgramMethod;
import com.android.tools.r8.ir.code.IRCode;
import com.android.tools.r8.naming.MemberNaming.MethodSignature;
@@ -37,6 +38,8 @@
return null;
}
+ public abstract MethodAccessFlags getAccessFlags();
+
@Override
public abstract MethodSignature getOriginalSignature();
