Version 1.4.25 Cherry pick: Correctly mark and treat FillArrayData as a throwing instruction. CL: https://r8-review.googlesource.com/c/r8/+/33037 Bug: 122887884 Cherry pick: Allow referencing graph nodes that are removed during tree shaking. CL: https://r8-review.googlesource.com/c/r8/+/32980 Bug: 120959039 Change-Id: Ic06dcf651f49f2c3fa7860155e1948ca43e7c95f
diff --git a/src/main/java/com/android/tools/r8/Version.java b/src/main/java/com/android/tools/r8/Version.java index 4336ca5..6c27fb1 100644 --- a/src/main/java/com/android/tools/r8/Version.java +++ b/src/main/java/com/android/tools/r8/Version.java
@@ -11,7 +11,7 @@ // This field is accessed from release scripts using simple pattern matching. // Therefore, changing this field could break our release scripts. - public static final String LABEL = "1.4.24"; + public static final String LABEL = "1.4.25"; private Version() { }
diff --git a/src/main/java/com/android/tools/r8/code/FillArrayData.java b/src/main/java/com/android/tools/r8/code/FillArrayData.java index 4569897..a49a31d 100644 --- a/src/main/java/com/android/tools/r8/code/FillArrayData.java +++ b/src/main/java/com/android/tools/r8/code/FillArrayData.java
@@ -44,4 +44,9 @@ public String toSmaliString(ClassNameMapper naming) { return formatSmaliString("v" + AA + ", :label_" + (getOffset() + BBBBBBBB)); } + + @Override + public boolean canThrow() { + return true; + } }
diff --git a/src/main/java/com/android/tools/r8/ir/code/NewArrayFilledData.java b/src/main/java/com/android/tools/r8/ir/code/NewArrayFilledData.java index de0f48c..ed51913 100644 --- a/src/main/java/com/android/tools/r8/ir/code/NewArrayFilledData.java +++ b/src/main/java/com/android/tools/r8/ir/code/NewArrayFilledData.java
@@ -8,7 +8,6 @@ import com.android.tools.r8.code.FillArrayDataPayload; import com.android.tools.r8.dex.Constants; import com.android.tools.r8.errors.Unreachable; -import com.android.tools.r8.graph.AppInfo; import com.android.tools.r8.graph.DexType; import com.android.tools.r8.ir.conversion.CfBuilder; import com.android.tools.r8.ir.conversion.DexBuilder; @@ -74,9 +73,8 @@ } @Override - public boolean canBeDeadCode(AppInfo appInfo, IRCode code) { - // Side-effects its input values. - return false; + public boolean instructionTypeCanThrow() { + return true; } @Override
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/DexSourceCode.java b/src/main/java/com/android/tools/r8/ir/conversion/DexSourceCode.java index 24487aa..6acd9b1 100644 --- a/src/main/java/com/android/tools/r8/ir/conversion/DexSourceCode.java +++ b/src/main/java/com/android/tools/r8/ir/conversion/DexSourceCode.java
@@ -355,6 +355,10 @@ return index; } if (dex.canThrow()) { + // TODO(zerny): Remove this from block computation. + if (dex.hasPayload()) { + arrayFilledDataPayloadResolver.addPayloadUser((FillArrayData) dex); + } // If the instruction can throw and is in a try block, add edges to its catch successors. Try tryRange = getTryForOffset(offset); if (tryRange != null) { @@ -397,10 +401,6 @@ builder.ensureNormalSuccessorBlock(offset, offset + dex.getSize()); return index; } - // TODO(zerny): Remove this from block computation. - if (dex.hasPayload()) { - arrayFilledDataPayloadResolver.addPayloadUser((FillArrayData) dex); - } // This instruction does not close the block. return -1; }
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/IRBuilder.java b/src/main/java/com/android/tools/r8/ir/conversion/IRBuilder.java index 835399c..4c9b15e 100644 --- a/src/main/java/com/android/tools/r8/ir/conversion/IRBuilder.java +++ b/src/main/java/com/android/tools/r8/ir/conversion/IRBuilder.java
@@ -1646,9 +1646,11 @@ } public void addNewArrayFilledData(int arrayRef, int elementWidth, long size, short[] data) { - add( + NewArrayFilledData instruction = new NewArrayFilledData( - readRegister(arrayRef, ValueTypeConstraint.OBJECT), elementWidth, size, data)); + readRegister(arrayRef, ValueTypeConstraint.OBJECT), elementWidth, size, data); + assert instruction.instructionTypeCanThrow(); + addInstruction(instruction); } public void addNewInstance(int dest, DexType type) {
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java b/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java index b1598bc..6a7e6ba 100644 --- a/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java +++ b/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java
@@ -2697,6 +2697,9 @@ if (contents == null) { continue; } + if (block.hasCatchHandlers()) { + continue; + } int arraySize = newArray.size().getConstInstruction().asConstNumber().getIntValue(); NewArrayFilledData fillArray = new NewArrayFilledData(newArray.outValue(), elementSize, arraySize, contents);
diff --git a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java index f10d000..3376854 100644 --- a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java +++ b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
@@ -2838,23 +2838,27 @@ } ClassGraphNode getClassGraphNode(DexType type) { - return classNodes.computeIfAbsent(type, - t -> new ClassGraphNode( - appInfo.definitionFor(type).isLibraryClass(), - Reference.classFromDescriptor(t.toDescriptorString()))); + return classNodes.computeIfAbsent( + type, + t -> { + DexClass definition = appInfo.definitionFor(t); + return new ClassGraphNode( + definition != null && definition.isLibraryClass(), + Reference.classFromDescriptor(t.toDescriptorString())); + }); } MethodGraphNode getMethodGraphNode(DexMethod context) { return methodNodes.computeIfAbsent( context, m -> { - boolean isLibraryNode = appInfo.definitionFor(context.holder).isLibraryClass(); + DexClass holderDefinition = appInfo.definitionFor(context.holder); Builder<TypeReference> builder = ImmutableList.builder(); for (DexType param : m.proto.parameters.values) { builder.add(Reference.typeFromDescriptor(param.toDescriptorString())); } return new MethodGraphNode( - isLibraryNode, + holderDefinition != null && holderDefinition.isLibraryClass(), Reference.method( Reference.classFromDescriptor(m.holder.toDescriptorString()), m.name.toString(), @@ -2868,13 +2872,15 @@ FieldGraphNode getFieldGraphNode(DexField context) { return fieldNodes.computeIfAbsent( context, - f -> - new FieldGraphNode( - appInfo.definitionFor(context.getHolder()).isLibraryClass(), - Reference.field( - Reference.classFromDescriptor(f.getHolder().toDescriptorString()), - f.name.toString(), - Reference.typeFromDescriptor(f.type.toDescriptorString())))); + f -> { + DexClass holderDefinition = appInfo.definitionFor(context.getHolder()); + return new FieldGraphNode( + holderDefinition != null && holderDefinition.isLibraryClass(), + Reference.field( + Reference.classFromDescriptor(f.getHolder().toDescriptorString()), + f.name.toString(), + Reference.typeFromDescriptor(f.type.toDescriptorString()))); + }); } KeepRuleGraphNode getKeepRuleGraphNode(ProguardKeepRule rule) {
diff --git a/src/test/java/com/android/tools/r8/regress/b122887884/Regress122887884.java b/src/test/java/com/android/tools/r8/regress/b122887884/Regress122887884.java new file mode 100644 index 0000000..23650c9 --- /dev/null +++ b/src/test/java/com/android/tools/r8/regress/b122887884/Regress122887884.java
@@ -0,0 +1,28 @@ +// Copyright (c) 2019, 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.regress.b122887884; + +public class Regress122887884 { + + public static int[] foo(String[] args) { + if (args.length == 0) { + return new int[] {0, 0}; + } + String first = args[0]; + try { + String[] split = first.split(""); + if (split.length != 2) { + // This results in a new-array v2 v2 int[] at which point the exception handler must split. + return new int[] {0, 0}; + } + return new int[] {1, 1}; + } catch (Throwable t) { + return new int[] {0, 0}; + } + } + + public static void main(String[] args) { + System.out.println(foo(args)[0]); + } +}
diff --git a/src/test/java/com/android/tools/r8/regress/b122887884/Regress122887884Runner.java b/src/test/java/com/android/tools/r8/regress/b122887884/Regress122887884Runner.java new file mode 100644 index 0000000..54f32fd --- /dev/null +++ b/src/test/java/com/android/tools/r8/regress/b122887884/Regress122887884Runner.java
@@ -0,0 +1,19 @@ +// Copyright (c) 2019, 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.regress.b122887884; + +import com.android.tools.r8.TestBase; +import com.android.tools.r8.utils.StringUtils; +import org.junit.Test; + +public class Regress122887884Runner extends TestBase { + + private final Class<?> CLASS = Regress122887884.class; + private final String EXPECTED = StringUtils.lines("0"); + + @Test + public void test() throws Exception { + testForD8().addProgramClasses(CLASS).run(CLASS).assertSuccessWithOutput(EXPECTED); + } +}
diff --git a/src/test/java/com/android/tools/r8/shaking/keptgraph/RemovedClassTest.java b/src/test/java/com/android/tools/r8/shaking/keptgraph/RemovedClassTest.java new file mode 100644 index 0000000..5992eff --- /dev/null +++ b/src/test/java/com/android/tools/r8/shaking/keptgraph/RemovedClassTest.java
@@ -0,0 +1,25 @@ +// Copyright (c) 2019, 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.keptgraph; + +import com.android.tools.r8.NeverInline; + +public class RemovedClassTest { + + public static class RemovedInnerClass {} + + public static void main(String[] args) { + bar(); + } + + @NeverInline + public static void bar() { + System.out.println("called bar"); + } + + @NeverInline + public static void baz() { + System.out.println("called baz, created " + new RemovedClassTest().getClass().getSimpleName()); + } +}
diff --git a/src/test/java/com/android/tools/r8/shaking/keptgraph/RemovedClassTestRunner.java b/src/test/java/com/android/tools/r8/shaking/keptgraph/RemovedClassTestRunner.java new file mode 100644 index 0000000..c555b7e --- /dev/null +++ b/src/test/java/com/android/tools/r8/shaking/keptgraph/RemovedClassTestRunner.java
@@ -0,0 +1,84 @@ +// Copyright (c) 2019, 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.keptgraph; + +import static com.android.tools.r8.references.Reference.classFromClass; +import static com.android.tools.r8.references.Reference.methodFromMethod; +import static org.junit.Assert.assertEquals; + +import com.android.tools.r8.R8TestCompileResult; +import com.android.tools.r8.TestBase; +import com.android.tools.r8.origin.Origin; +import com.android.tools.r8.references.MethodReference; +import com.android.tools.r8.utils.StringUtils; +import com.android.tools.r8.utils.graphinspector.GraphInspector; +import com.android.tools.r8.utils.graphinspector.GraphInspector.QueryNode; +import com.google.common.collect.ImmutableList; +import java.io.ByteArrayOutputStream; +import java.io.PrintStream; +import java.nio.charset.StandardCharsets; +import java.util.List; +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 RemovedClassTestRunner extends TestBase { + + private static final Class<?> CLASS = RemovedClassTest.class; + private static final Class<?> REMOVED_CLASS = RemovedClassTest.RemovedInnerClass.class; + private static final List<Class<?>> CLASSES = ImmutableList.of(CLASS, REMOVED_CLASS); + + private static final String EXPECTED = StringUtils.lines("called bar"); + + private final Backend backend; + + @Parameters(name = "{0}") + public static Backend[] data() { + return Backend.values(); + } + + public RemovedClassTestRunner(Backend backend) { + this.backend = backend; + } + + @Test + public void testRemovedClass() throws Exception { + MethodReference mainMethod = methodFromMethod(CLASS.getDeclaredMethod("main", String[].class)); + MethodReference barMethod = methodFromMethod(CLASS.getDeclaredMethod("bar")); + MethodReference bazMethod = methodFromMethod(CLASS.getDeclaredMethod("baz")); + + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + R8TestCompileResult compileResult = + testForR8(backend) + .enableGraphInspector() + .enableInliningAnnotations() + .addProgramClasses(CLASSES) + .addKeepMethodRules(mainMethod) + .addKeepRules("-whyareyoukeeping class " + REMOVED_CLASS.getTypeName()) + .redirectStdOut(new PrintStream(baos)) + .compile(); + String expectedOutput = StringUtils.lines("Nothing is keeping " + REMOVED_CLASS.getTypeName()); + String compileOutput = new String(baos.toByteArray(), StandardCharsets.UTF_8); + assertEquals(expectedOutput, compileOutput); + + GraphInspector inspector = + compileResult.run(CLASS).assertSuccessWithOutput(EXPECTED).graphInspector(); + + // The only root should be the keep main-method rule. + assertEquals(1, inspector.getRoots().size()); + QueryNode root = inspector.rule(Origin.unknown(), 1, 1).assertRoot(); + + // Check that the call chain goes from root -> main(unchanged) -> bar(renamed). + inspector.method(barMethod).assertRenamed().assertInvokedFrom(mainMethod); + inspector.method(mainMethod).assertNotRenamed().assertKeptBy(root); + + // Check baz is removed. + inspector.method(bazMethod).assertAbsent(); + + // Check that the inner class is removed. + inspector.clazz(classFromClass(REMOVED_CLASS)).assertAbsent(); + } +}