Access relaxation for static private methods. Bug:72108996 Change-Id: I6303417b11564b273b6abc687ff9a99332d6e5ae
diff --git a/src/main/java/com/android/tools/r8/R8.java b/src/main/java/com/android/tools/r8/R8.java index 9609280..901c490 100644 --- a/src/main/java/com/android/tools/r8/R8.java +++ b/src/main/java/com/android/tools/r8/R8.java
@@ -311,7 +311,7 @@ } if (options.proguardConfiguration.isAccessModificationAllowed()) { - ClassAndMemberPublicizer.run(application); + ClassAndMemberPublicizer.run(application, appInfo.dexItemFactory); // We can now remove visibility bridges. Note that we do not need to update the // invoke-targets here, as the existing invokes will simply dispatch to the now // visible super-method. MemberRebinding, if run, will then dispatch it correctly.
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 e2654aa..324362a 100644 --- a/src/main/java/com/android/tools/r8/graph/AccessFlags.java +++ b/src/main/java/com/android/tools/r8/graph/AccessFlags.java
@@ -139,13 +139,6 @@ unset(Constants.ACC_SYNTHETIC); } - public void promoteNonPrivateToPublic() { - if (!isPrivate()) { - unsetProtected(); - setPublic(); - } - } - public void promoteToPublic() { unsetProtected(); unsetPrivate();
diff --git a/src/main/java/com/android/tools/r8/graph/ClassAndMemberPublicizer.java b/src/main/java/com/android/tools/r8/graph/ClassAndMemberPublicizer.java index 6a6b3d8..7a13348 100644 --- a/src/main/java/com/android/tools/r8/graph/ClassAndMemberPublicizer.java +++ b/src/main/java/com/android/tools/r8/graph/ClassAndMemberPublicizer.java
@@ -3,18 +3,66 @@ // BSD-style license that can be found in the LICENSE file. package com.android.tools.r8.graph; -public abstract class ClassAndMemberPublicizer { +public final class ClassAndMemberPublicizer { + private final DexItemFactory factory; + private final DexApplication application; + + private ClassAndMemberPublicizer(DexApplication application, DexItemFactory factory) { + this.application = application; + this.factory = factory; + } + /** - * Marks all package private and protected methods and fields as public. + * Marks all package private and protected methods and fields as public. Also makes + * all private static methods public. * <p> * This will destructively update the DexApplication passed in as argument. */ - public static DexApplication run(DexApplication application) { + public static DexApplication run(DexApplication application, DexItemFactory factory) { + return new ClassAndMemberPublicizer(application, factory).run(); + } + + private DexApplication run() { for (DexClass clazz : application.classes()) { clazz.accessFlags.promoteToPublic(); - clazz.forEachMethod(method -> method.accessFlags.promoteNonPrivateToPublic()); + clazz.forEachMethod(this::publicizeMethod); clazz.forEachField(field -> field.accessFlags.promoteToPublic()); } return application; } + + private void publicizeMethod(DexEncodedMethod method) { + MethodAccessFlags accessFlags = method.accessFlags; + if (accessFlags.isPublic()) { + return; + } + + if (factory.isClassConstructor(method.method)) { + return; + } + + if (!accessFlags.isPrivate()) { + accessFlags.unsetProtected(); + accessFlags.setPublic(); + return; + } + assert accessFlags.isPrivate(); + + if (factory.isConstructor(method.method)) { + // TODO: b/72211928 + return; + } + + if (!accessFlags.isStatic()) { + // TODO: b/72109068 + return; + } + + // For private static methods we can just relax the access to private, since + // even though JLS prevents from declaring static method in derived class if + // an instance method with same signature exists in superclass, JVM actually + // does not take into account access of the static methods. + accessFlags.unsetPrivate(); + accessFlags.setPublic(); + } }
diff --git a/src/test/java/com/android/tools/r8/accessrelaxation/AccessRelaxationTest.java b/src/test/java/com/android/tools/r8/accessrelaxation/AccessRelaxationTest.java new file mode 100644 index 0000000..18e17f9 --- /dev/null +++ b/src/test/java/com/android/tools/r8/accessrelaxation/AccessRelaxationTest.java
@@ -0,0 +1,71 @@ +// 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.accessrelaxation; + +import static org.junit.Assert.assertEquals; + +import com.android.tools.r8.DexIndexedConsumer; +import com.android.tools.r8.R8Command; +import com.android.tools.r8.TestBase; +import com.android.tools.r8.ToolHelper; +import com.android.tools.r8.accessrelaxation.privatestatic.A; +import com.android.tools.r8.accessrelaxation.privatestatic.B; +import com.android.tools.r8.accessrelaxation.privatestatic.BB; +import com.android.tools.r8.accessrelaxation.privatestatic.C; +import com.android.tools.r8.origin.Origin; +import com.android.tools.r8.utils.AndroidApp; +import com.google.common.collect.ImmutableList; +import org.junit.Test; + +public class AccessRelaxationTest extends TestBase { + @Test + public void testStaticMethodRelaxation() throws Exception { + R8Command.Builder builder = R8Command.builder(); + builder.addProgramFiles(ToolHelper.getClassFilesForTestPackage(A.class.getPackage())); + builder.setProgramConsumer(DexIndexedConsumer.emptyConsumer()); + builder.setMinApiLevel(ToolHelper.getMinApiLevelForDexVm(ToolHelper.getDexVm())); + + // Note: we use '-checkdiscard' to indirectly check that the access relaxation is + // done which leads to inlining of all pB*** methods so they are removed. Without + // access relaxation inlining is not performed and method are kept. + builder.addProguardConfiguration( + ImmutableList.of( + "-keep class " + C.class.getCanonicalName() + "{", + " public static void main(java.lang.String[]);", + "}", + "", + "-checkdiscard class " + A.class.getCanonicalName() + "{", + " *** pBaz();", + " *** pBar();", + " *** pBar1();", + " *** pBlah1();", + "}", + "", + "-checkdiscard class " + B.class.getCanonicalName() + "{", + " *** pBlah1();", + "}", + "", + "-checkdiscard class " + BB.class.getCanonicalName() + "{", + " *** pBlah1();", + "}", + "", + "-dontobfuscate", + "-allowaccessmodification" + ), + Origin.unknown()); + + AndroidApp app = ToolHelper.runR8(builder.build()); + + // Run on Jvm. + String jvmOutput = runOnJava(C.class); + + // Run on Art to check generated code against verifier. + String artOutput = runOnArt(app, C.class); + + String adjustedArtOutput = artOutput.replace( + "java.lang.IncompatibleClassChangeError", "java.lang.IllegalAccessError"); + assertEquals(jvmOutput, adjustedArtOutput); + } +}
diff --git a/src/test/java/com/android/tools/r8/accessrelaxation/privatestatic/A.java b/src/test/java/com/android/tools/r8/accessrelaxation/privatestatic/A.java new file mode 100644 index 0000000..5337641 --- /dev/null +++ b/src/test/java/com/android/tools/r8/accessrelaxation/privatestatic/A.java
@@ -0,0 +1,49 @@ +// 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.accessrelaxation.privatestatic; + +public class A { + public String foo() { + return "A::foo()" + baz() + bar() + bar(0); + } + + // NOTE: here and below 'synchronized' is supposed to disable inlining of this method. + private synchronized static String baz() { + return "A::baz()"; + } + + public static String pBaz() { + return baz(); + } + + private synchronized static String bar() { + return "A::bar()"; + } + + public static String pBar() { + return bar(); + } + + private synchronized static String bar(int i) { + return "A::bar(int)"; + } + + public static String pBar1() { + return bar(1); + } + + private synchronized static String blah(int i) { + return "A::blah(int)"; + } + + public static String pBlah1() { + return blah(1); + } + + public void dump() { + System.out.println(foo()); + } +} +
diff --git a/src/test/java/com/android/tools/r8/accessrelaxation/privatestatic/B.java b/src/test/java/com/android/tools/r8/accessrelaxation/privatestatic/B.java new file mode 100644 index 0000000..b0518df --- /dev/null +++ b/src/test/java/com/android/tools/r8/accessrelaxation/privatestatic/B.java
@@ -0,0 +1,35 @@ +// 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.accessrelaxation.privatestatic; + +public class B extends A implements I { + public String bar() { + try { + return "B::bar()" + bar(0); + } catch (Throwable e) { + return "B::bar() >> " + e.getClass().getName(); + } + } + + private synchronized static String blah(int i) { + return "B::blah(int)"; + } + + public static String pBlah1() { + return blah(1); + } + + public void dump() { + System.out.println(this.bar()); + try { + System.out.println(this.bar(0)); + } catch (Throwable e) { + System.out.println(e.getClass().getName()); + } + System.out.println(this.foo()); + System.out.println(blah(0)); + } +} +
diff --git a/src/test/java/com/android/tools/r8/accessrelaxation/privatestatic/BB.java b/src/test/java/com/android/tools/r8/accessrelaxation/privatestatic/BB.java new file mode 100644 index 0000000..0887714 --- /dev/null +++ b/src/test/java/com/android/tools/r8/accessrelaxation/privatestatic/BB.java
@@ -0,0 +1,20 @@ +// 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.accessrelaxation.privatestatic; + +public class BB extends A { + private synchronized static String blah(int i) { + return "BB::blah(int)"; + } + + public static String pBlah1() { + return blah(1); + } + + public void dump() { + System.out.println(this.foo()); + } +} +
diff --git a/src/test/java/com/android/tools/r8/accessrelaxation/privatestatic/C.java b/src/test/java/com/android/tools/r8/accessrelaxation/privatestatic/C.java new file mode 100644 index 0000000..72ffccd --- /dev/null +++ b/src/test/java/com/android/tools/r8/accessrelaxation/privatestatic/C.java
@@ -0,0 +1,45 @@ +// 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.accessrelaxation.privatestatic; + +public class C extends B { + public String foo() { + return "B::foo()" + super.foo(); + } + + public String blah(int i) { + return "C::blah(int)"; + } + + public String bar(int i) { + try { + return "C::bar(int)" + super.bar(i) + super.bar() + super.bar(); + } catch (Throwable e) { + return "C::bar(int)" + e.getClass().getName() + super.bar() + super.bar(); + } + } + + public void dump() { + System.out.println(this.bar()); + System.out.println(this.bar(0)); + System.out.println(this.foo()); + System.out.println(this.blah(0)); + } + + public static void main(String[] args) { + System.out.println(A.pBaz()); + System.out.println(A.pBar()); + System.out.println(A.pBar1()); + System.out.println(A.pBlah1()); + + System.out.println(B.pBlah1()); + System.out.println(BB.pBlah1()); + + new A().dump(); + new B().dump(); + new BB().dump(); + new C().dump(); + } +}
diff --git a/src/test/java/com/android/tools/r8/accessrelaxation/privatestatic/I.java b/src/test/java/com/android/tools/r8/accessrelaxation/privatestatic/I.java new file mode 100644 index 0000000..bd42c58 --- /dev/null +++ b/src/test/java/com/android/tools/r8/accessrelaxation/privatestatic/I.java
@@ -0,0 +1,13 @@ +// 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.accessrelaxation.privatestatic; + +public interface I { + String bar(); + + default String bar(int i) { + return "I::bar(int)" + bar(); + } +}
diff --git a/src/test/java/com/android/tools/r8/naming/NamingTestBase.java b/src/test/java/com/android/tools/r8/naming/NamingTestBase.java index b55aea0..1c3a9e0 100644 --- a/src/test/java/com/android/tools/r8/naming/NamingTestBase.java +++ b/src/test/java/com/android/tools/r8/naming/NamingTestBase.java
@@ -71,7 +71,7 @@ new Reporter(new DefaultDiagnosticsHandler())); if (options.proguardConfiguration.isAccessModificationAllowed()) { - ClassAndMemberPublicizer.run(program); + ClassAndMemberPublicizer.run(program, dexItemFactory); } RootSet rootSet = new RootSetBuilder(program, appInfo, configuration.getRules(), options)