Version 1.3.46 Merge: Minor cleanup of class inliner CL: https://r8-review.googlesource.com/c/r8/+/32221 Merge: Check for inline-ability before force-inlining in class inliner CL: https://r8-review.googlesource.com/c/r8/+/32138 Merge: Add class inliner test for invoke-super inlining CL: https://r8-review.googlesource.com/c/r8/+/32133 Merge: Abort gracefully from class inlining in case of ineligible users CL: https://r8-review.googlesource.com/c/r8/+/32104 Merge: Add failing test for class inliner CL: https://r8-review.googlesource.com/c/r8/+/32103 Merge: Output abort reason in class inliner CL: https://r8-review.googlesource.com/c/r8/+/31960 Merge: Attempt to make test independent of Java version CL: https://r8-review.googlesource.com/c/r8/+/31961 Merge: Extend class inliner's escape analysis CL: https://r8-review.googlesource.com/c/r8/+/31920 Merge: Partial fix for class inliner's escape analysis CL: https://r8-review.googlesource.com/c/r8/+/31901 Merge: Add failing test for class inliner bug CL: https://r8-review.googlesource.com/c/r8/+/31808 Merge: Extend error messages in class inliner CL: https://r8-review.googlesource.com/c/r8/+/31508 Merge: Introduce -neverclassinline config option CL: https://r8-review.googlesource.com/c/r8/+/30891 Bug: 120182628, 121119666 Change-Id: I1cab5c66c3f066960f82163d5287600e4959cbf6
diff --git a/src/main/java/com/android/tools/r8/Version.java b/src/main/java/com/android/tools/r8/Version.java index 8b14556..3d5acb9 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.3.45"; + public static final String LABEL = "1.3.46"; private Version() { }
diff --git a/src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java b/src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java index c3db796..481b6c1 100644 --- a/src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java +++ b/src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java
@@ -390,12 +390,6 @@ } } - public boolean hasDebugPositions() { - checkIfObsolete(); - assert code != null && code.isDexCode(); - return code.asDexCode().hasDebugPositions(); - } - public int getClassFileVersion() { checkIfObsolete(); assert classFileVersion >= 0;
diff --git a/src/main/java/com/android/tools/r8/graph/ParameterUsagesInfo.java b/src/main/java/com/android/tools/r8/graph/ParameterUsagesInfo.java index 84cf331..aac25ba 100644 --- a/src/main/java/com/android/tools/r8/graph/ParameterUsagesInfo.java +++ b/src/main/java/com/android/tools/r8/graph/ParameterUsagesInfo.java
@@ -4,9 +4,14 @@ package com.android.tools.r8.graph; +import com.android.tools.r8.ir.code.If; import com.android.tools.r8.ir.code.If.Type; +import com.android.tools.r8.ir.code.InstanceGet; +import com.android.tools.r8.ir.code.InstancePut; import com.android.tools.r8.ir.code.Instruction; import com.android.tools.r8.ir.code.Invoke; +import com.android.tools.r8.ir.code.InvokeMethodWithReceiver; +import com.android.tools.r8.ir.code.Return; import com.android.tools.r8.ir.code.Value; import com.android.tools.r8.utils.Pair; import com.google.common.collect.ImmutableList; @@ -38,32 +43,63 @@ } public final static class ParameterUsage { + public final int index; public final Set<Type> ifZeroTest; public final List<Pair<Invoke.Type, DexMethod>> callsReceiver; - public final boolean returnValue; - ParameterUsage(int index, Set<Type> ifZeroTest, - List<Pair<Invoke.Type, DexMethod>> callsReceiver, boolean returnValue) { + // If a field of this argument is assigned: arg.f = x. + public final boolean hasFieldAssignment; + + // If a field of this argument is read: x = arg.f. + public final boolean hasFieldRead; + + // If this argument is assigned to a field: x.f = arg. + public final boolean isAssignedToField; + + // If this argument is returned: return arg. + public final boolean isReturned; + + ParameterUsage( + int index, + Set<Type> ifZeroTest, + List<Pair<Invoke.Type, DexMethod>> callsReceiver, + boolean hasFieldAssignment, + boolean hasFieldRead, + boolean isAssignedToField, + boolean isReturned) { this.index = index; - this.ifZeroTest = ifZeroTest.isEmpty() - ? Collections.emptySet() : ImmutableSet.copyOf(ifZeroTest); - this.callsReceiver = callsReceiver.isEmpty() - ? Collections.emptyList() : ImmutableList.copyOf(callsReceiver); - this.returnValue = returnValue; + this.ifZeroTest = + ifZeroTest.isEmpty() ? Collections.emptySet() : ImmutableSet.copyOf(ifZeroTest); + this.callsReceiver = + callsReceiver.isEmpty() ? Collections.emptyList() : ImmutableList.copyOf(callsReceiver); + this.hasFieldAssignment = hasFieldAssignment; + this.hasFieldRead = hasFieldRead; + this.isAssignedToField = isAssignedToField; + this.isReturned = isReturned; } public boolean notUsed() { - return ifZeroTest == null && callsReceiver == null && !returnValue; + return ifZeroTest == null + && callsReceiver == null + && !hasFieldAssignment + && !hasFieldRead + && !isAssignedToField + && !isReturned; } } public static class ParameterUsageBuilder { + private final int index; private final Value arg; private final Set<Type> ifZeroTestTypes = new HashSet<>(); private final List<Pair<Invoke.Type, DexMethod>> callsOnReceiver = new ArrayList<>(); - private boolean returnValue = false; + + private boolean hasFieldAssignment = false; + private boolean hasFieldRead = false; + private boolean isAssignedToField = false; + private boolean isReturned = false; public ParameterUsageBuilder(Value arg, int index) { this.arg = arg; @@ -72,31 +108,82 @@ // Returns false if the instruction is not supported. public boolean note(Instruction instruction) { - if (instruction.isInvokeMethodWithReceiver() && - instruction.inValues().lastIndexOf(arg) == 0) { - callsOnReceiver.add(new Pair<>( - instruction.asInvokeMethodWithReceiver().getType(), - instruction.asInvokeMethodWithReceiver().getInvokedMethod())); - return true; + if (instruction.isIf()) { + return note(instruction.asIf()); } - - if (instruction.isIf() && instruction.asIf().isZeroTest()) { - assert instruction.inValues().size() == 1 && instruction.inValues().get(0) == arg; - ifZeroTestTypes.add(instruction.asIf().getType()); - return true; + if (instruction.isInstanceGet()) { + return note(instruction.asInstanceGet()); } - + if (instruction.isInstancePut()) { + return note(instruction.asInstancePut()); + } + if (instruction.isInvokeMethodWithReceiver()) { + return note(instruction.asInvokeMethodWithReceiver()); + } if (instruction.isReturn()) { - assert instruction.inValues().size() == 1 && instruction.inValues().get(0) == arg; - returnValue = true; - return true; + return note(instruction.asReturn()); } - return false; } public ParameterUsage build() { - return new ParameterUsage(index, ifZeroTestTypes, callsOnReceiver, returnValue); + return new ParameterUsage( + index, + ifZeroTestTypes, + callsOnReceiver, + hasFieldAssignment, + hasFieldRead, + isAssignedToField, + isReturned); + } + + private boolean note(If ifInstruction) { + if (ifInstruction.asIf().isZeroTest()) { + assert ifInstruction.inValues().size() == 1 && ifInstruction.inValues().get(0) == arg; + ifZeroTestTypes.add(ifInstruction.asIf().getType()); + return true; + } + return false; + } + + private boolean note(InstanceGet instanceGetInstruction) { + assert arg != instanceGetInstruction.outValue(); + if (instanceGetInstruction.object() == arg) { + hasFieldRead = true; + return true; + } + return false; + } + + private boolean note(InstancePut instancePutInstruction) { + assert arg != instancePutInstruction.outValue(); + if (instancePutInstruction.object() == arg) { + hasFieldAssignment = true; + isAssignedToField |= instancePutInstruction.value() == arg; + return true; + } + if (instancePutInstruction.value() == arg) { + isAssignedToField = true; + return true; + } + return false; + } + + private boolean note(InvokeMethodWithReceiver invokeInstruction) { + if (invokeInstruction.inValues().lastIndexOf(arg) == 0) { + callsOnReceiver.add( + new Pair<>( + invokeInstruction.asInvokeMethodWithReceiver().getType(), + invokeInstruction.asInvokeMethodWithReceiver().getInvokedMethod())); + return true; + } + return false; + } + + private boolean note(Return returnInstruction) { + assert returnInstruction.inValues().size() == 1 && returnInstruction.inValues().get(0) == arg; + isReturned = true; + return true; } } }
diff --git a/src/main/java/com/android/tools/r8/ir/code/Instruction.java b/src/main/java/com/android/tools/r8/ir/code/Instruction.java index 6f62d21..8c5f51b 100644 --- a/src/main/java/com/android/tools/r8/ir/code/Instruction.java +++ b/src/main/java/com/android/tools/r8/ir/code/Instruction.java
@@ -31,7 +31,7 @@ import java.util.Set; import java.util.function.Function; -public abstract class Instruction { +public abstract class Instruction implements InstructionOrPhi { protected Value outValue = null; protected final List<Value> inValues = new ArrayList<>(); @@ -499,6 +499,11 @@ return debugValues != null ? debugValues : ImmutableSet.of(); } + @Override + public boolean isInstruction() { + return true; + } + public boolean isArrayGet() { return false; }
diff --git a/src/main/java/com/android/tools/r8/ir/code/InstructionOrPhi.java b/src/main/java/com/android/tools/r8/ir/code/InstructionOrPhi.java new file mode 100644 index 0000000..10110a0 --- /dev/null +++ b/src/main/java/com/android/tools/r8/ir/code/InstructionOrPhi.java
@@ -0,0 +1,16 @@ +// 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.ir.code; + +public interface InstructionOrPhi { + + default boolean isInstruction() { + return false; + } + + default boolean isPhi() { + return false; + } +}
diff --git a/src/main/java/com/android/tools/r8/ir/code/Phi.java b/src/main/java/com/android/tools/r8/ir/code/Phi.java index f3301e7..61d3883 100644 --- a/src/main/java/com/android/tools/r8/ir/code/Phi.java +++ b/src/main/java/com/android/tools/r8/ir/code/Phi.java
@@ -20,7 +20,7 @@ import java.util.Map.Entry; import java.util.Set; -public class Phi extends Value { +public class Phi extends Value implements InstructionOrPhi { public enum RegisterReadType { NORMAL,
diff --git a/src/main/java/com/android/tools/r8/ir/code/Value.java b/src/main/java/com/android/tools/r8/ir/code/Value.java index 96f765f..1d8e88b 100644 --- a/src/main/java/com/android/tools/r8/ir/code/Value.java +++ b/src/main/java/com/android/tools/r8/ir/code/Value.java
@@ -277,6 +277,11 @@ return users.getFirst(); } + public Phi firstPhiUser() { + assert !phiUsers.isEmpty(); + return phiUsers.getFirst(); + } + public Set<Phi> uniquePhiUsers() { if (uniquePhiUsers != null) { return uniquePhiUsers;
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java b/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java index 1aad1b9..ac5dd28 100644 --- a/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java +++ b/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
@@ -487,10 +487,9 @@ BiConsumer<IRCode, DexEncodedMethod> outlineHandler = outliner == null ? Outliner::noProcessing : outliner.identifyCandidateMethods(); callGraph.forEachMethod( - (method, isProcessedConcurrently) -> { - processMethod( - method, directFeedback, isProcessedConcurrently, callGraph, outlineHandler); - }, + (method, isProcessedConcurrently) -> + processMethod( + method, directFeedback, isProcessedConcurrently, callGraph, outlineHandler), executorService); timing.end(); } @@ -841,14 +840,21 @@ // lambda, it is not get collected by merger. assert options.enableInlining && inliner != null; classInliner.processMethodCode( - appInfo.withLiveness(), codeRewriter, method, code, isProcessedConcurrently, - methodsToInline -> inliner.performForcedInlining(method, code, methodsToInline), - Suppliers.memoize(() -> inliner.createDefaultOracle( - method, code, - isProcessedConcurrently, callSiteInformation, - Integer.MAX_VALUE / 2, Integer.MAX_VALUE / 2) - ) - ); + appInfo.withLiveness(), + codeRewriter, + method, + code, + isProcessedConcurrently, + inliner, + Suppliers.memoize( + () -> + inliner.createDefaultOracle( + method, + code, + isProcessedConcurrently, + callSiteInformation, + Integer.MAX_VALUE / 2, + Integer.MAX_VALUE / 2))); assert code.isConsistentSSA(); }
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 d11946c..809ef70 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
@@ -818,8 +818,18 @@ continue; } - if (insn.isInstanceGet() || - (insn.isInstancePut() && insn.asInstancePut().object() == receiver)) { + if (insn.isInstanceGet() || insn.isInstancePut()) { + if (insn.isInstancePut()) { + InstancePut instancePutInstruction = insn.asInstancePut(); + // Only allow field writes to the receiver. + if (instancePutInstruction.object() != receiver) { + return; + } + // Do not allow the receiver to escape via a field write. + if (instancePutInstruction.value() == receiver) { + return; + } + } DexField field = insn.asFieldInstruction().getField(); if (field.clazz == clazz.type && clazz.lookupInstanceField(field) != null) { // Require only accessing instance fields of the *current* class. @@ -880,7 +890,7 @@ public void identifyParameterUsages( DexEncodedMethod method, IRCode code, OptimizationFeedback feedback) { List<ParameterUsage> usages = new ArrayList<>(); - List<Value> values = code.collectArguments(true); + List<Value> values = code.collectArguments(); for (int i = 0; i < values.size(); i++) { Value value = values.get(i); if (value.numberOfPhiUsers() > 0) {
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/classinliner/ClassInliner.java b/src/main/java/com/android/tools/r8/ir/optimize/classinliner/ClassInliner.java index 0ef4597..ec6a2fe 100644 --- a/src/main/java/com/android/tools/r8/ir/optimize/classinliner/ClassInliner.java +++ b/src/main/java/com/android/tools/r8/ir/optimize/classinliner/ClassInliner.java
@@ -4,22 +4,20 @@ package com.android.tools.r8.ir.optimize.classinliner; -import com.android.tools.r8.graph.AppInfo; import com.android.tools.r8.graph.DexClass; import com.android.tools.r8.graph.DexEncodedMethod; import com.android.tools.r8.graph.DexItemFactory; import com.android.tools.r8.ir.code.IRCode; import com.android.tools.r8.ir.code.Instruction; -import com.android.tools.r8.ir.code.InvokeMethod; +import com.android.tools.r8.ir.code.InstructionOrPhi; import com.android.tools.r8.ir.desugar.LambdaRewriter; import com.android.tools.r8.ir.optimize.CodeRewriter; -import com.android.tools.r8.ir.optimize.Inliner.InliningInfo; +import com.android.tools.r8.ir.optimize.Inliner; import com.android.tools.r8.ir.optimize.InliningOracle; import com.android.tools.r8.shaking.Enqueuer.AppInfoWithLiveness; import com.google.common.collect.Streams; import java.util.Iterator; import java.util.List; -import java.util.Map; import java.util.concurrent.ConcurrentHashMap; import java.util.function.Predicate; import java.util.function.Supplier; @@ -31,10 +29,6 @@ private final int totalMethodInstructionLimit; private final ConcurrentHashMap<DexClass, Boolean> knownClasses = new ConcurrentHashMap<>(); - public interface InlinerAction { - void inline(Map<InvokeMethod, InliningInfo> methods); - } - public ClassInliner(DexItemFactory factory, LambdaRewriter lambdaRewriter, int totalMethodInstructionLimit) { this.factory = factory; @@ -125,13 +119,14 @@ DexEncodedMethod method, IRCode code, Predicate<DexEncodedMethod> isProcessedConcurrently, - InlinerAction inliner, + Inliner inliner, Supplier<InliningOracle> defaultOracle) { // Collect all the new-instance and static-get instructions in the code before inlining. - List<Instruction> roots = Streams.stream(code.instructionIterator()) - .filter(insn -> insn.isNewInstance() || insn.isStaticGet()) - .collect(Collectors.toList()); + List<Instruction> roots = + Streams.stream(code.instructionIterator()) + .filter(insn -> insn.isNewInstance() || insn.isStaticGet()) + .collect(Collectors.toList()); // We loop inlining iterations until there was no inlining, but still use same set // of roots to avoid infinite inlining. Looping makes possible for some roots to @@ -145,20 +140,26 @@ while (rootsIterator.hasNext()) { Instruction root = rootsIterator.next(); InlineCandidateProcessor processor = - new InlineCandidateProcessor(factory, appInfo, lambdaRewriter, + new InlineCandidateProcessor( + factory, + appInfo, + lambdaRewriter, + inliner, clazz -> isClassEligible(appInfo, clazz), - isProcessedConcurrently, method, root); + isProcessedConcurrently, + method, + root); // Assess eligibility of instance and class. if (!processor.isInstanceEligible() || !processor.isClassAndUsageEligible()) { // This root will never be inlined. - rootsIterator.remove(); continue; } // Assess users eligibility and compute inlining of direct calls and extra methods needed. - if (!processor.areInstanceUsersEligible(method.method.getHolder(), defaultOracle)) { + InstructionOrPhi ineligibleUser = processor.areInstanceUsersEligible(defaultOracle); + if (ineligibleUser != null) { // This root may succeed if users change in future. continue; } @@ -169,7 +170,7 @@ } // Inline the class instance. - boolean anyInlinedMethods = processor.processInlining(code, inliner); + boolean anyInlinedMethods = processor.processInlining(code, defaultOracle); // Restore normality. code.removeAllTrivialPhis(); @@ -183,7 +184,7 @@ } while (repeat); } - private boolean isClassEligible(AppInfo appInfo, DexClass clazz) { + private boolean isClassEligible(AppInfoWithLiveness appInfo, DexClass clazz) { Boolean eligible = knownClasses.get(clazz); if (eligible == null) { Boolean computed = computeClassEligible(appInfo, clazz); @@ -198,9 +199,12 @@ // - is not an abstract class or interface // - does not declare finalizer // - does not trigger any static initializers except for its own - private boolean computeClassEligible(AppInfo appInfo, DexClass clazz) { - if (clazz == null || clazz.isLibraryClass() || - clazz.accessFlags.isAbstract() || clazz.accessFlags.isInterface()) { + private boolean computeClassEligible(AppInfoWithLiveness appInfo, DexClass clazz) { + if (clazz == null + || clazz.isLibraryClass() + || clazz.accessFlags.isAbstract() + || clazz.accessFlags.isInterface() + || appInfo.neverClassInline.contains(clazz.type)) { return false; }
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/classinliner/InlineCandidateProcessor.java b/src/main/java/com/android/tools/r8/ir/optimize/classinliner/InlineCandidateProcessor.java index 9baa647..c669507 100644 --- a/src/main/java/com/android/tools/r8/ir/optimize/classinliner/InlineCandidateProcessor.java +++ b/src/main/java/com/android/tools/r8/ir/optimize/classinliner/InlineCandidateProcessor.java
@@ -24,17 +24,18 @@ import com.android.tools.r8.ir.code.If; import com.android.tools.r8.ir.code.InstanceGet; import com.android.tools.r8.ir.code.Instruction; +import com.android.tools.r8.ir.code.InstructionOrPhi; import com.android.tools.r8.ir.code.Invoke.Type; import com.android.tools.r8.ir.code.InvokeDirect; import com.android.tools.r8.ir.code.InvokeMethod; import com.android.tools.r8.ir.code.InvokeMethodWithReceiver; import com.android.tools.r8.ir.code.Value; import com.android.tools.r8.ir.desugar.LambdaRewriter; +import com.android.tools.r8.ir.optimize.Inliner; import com.android.tools.r8.ir.optimize.Inliner.InlineAction; import com.android.tools.r8.ir.optimize.Inliner.InliningInfo; import com.android.tools.r8.ir.optimize.Inliner.Reason; import com.android.tools.r8.ir.optimize.InliningOracle; -import com.android.tools.r8.ir.optimize.classinliner.ClassInliner.InlinerAction; import com.android.tools.r8.kotlin.KotlinInfo; import com.android.tools.r8.shaking.Enqueuer.AppInfoWithLiveness; import com.android.tools.r8.utils.Pair; @@ -56,6 +57,7 @@ private final DexItemFactory factory; private final AppInfoWithLiveness appInfo; private final LambdaRewriter lambdaRewriter; + private final Inliner inliner; private final Predicate<DexClass> isClassEligible; private final Predicate<DexEncodedMethod> isProcessedConcurrently; private final DexEncodedMethod method; @@ -76,12 +78,17 @@ private int estimatedCombinedSizeForInlining = 0; InlineCandidateProcessor( - DexItemFactory factory, AppInfoWithLiveness appInfo, - LambdaRewriter lambdaRewriter, Predicate<DexClass> isClassEligible, + DexItemFactory factory, + AppInfoWithLiveness appInfo, + LambdaRewriter lambdaRewriter, + Inliner inliner, + Predicate<DexClass> isClassEligible, Predicate<DexEncodedMethod> isProcessedConcurrently, - DexEncodedMethod method, Instruction root) { + DexEncodedMethod method, + Instruction root) { this.factory = factory; this.lambdaRewriter = lambdaRewriter; + this.inliner = inliner; this.isClassEligible = isClassEligible; this.method = method; this.root = root; @@ -101,8 +108,8 @@ return false; } - eligibleClass = isNewInstance() ? - root.asNewInstance().clazz : root.asStaticGet().getField().type; + eligibleClass = + root.isNewInstance() ? root.asNewInstance().clazz : root.asStaticGet().getField().type; eligibleClassDefinition = appInfo.definitionFor(eligibleClass); if (eligibleClassDefinition == null && lambdaRewriter != null) { // Check if the class is synthesized for a desugared lambda @@ -128,7 +135,7 @@ return false; } - if (isNewInstance()) { + if (root.isNewInstance()) { // NOTE: if the eligible class does not directly extend java.lang.Object, // we also have to guarantee that it is initialized with initializer classified as // TrivialInstanceInitializer. This will be checked in areInstanceUsersEligible(...). @@ -215,53 +222,78 @@ !appInfo.isPinned(eligibleClassDefinition.lookupStaticField(instanceField).field); } - // Checks if the inlining candidate instance users are eligible, - // see comment on processMethodCode(...). - boolean areInstanceUsersEligible( - DexType invocationContext, Supplier<InliningOracle> defaultOracle) { + /** + * Checks if the inlining candidate instance users are eligible, see comment on {@link + * ClassInliner#processMethodCode}. + * + * @return null if all users are eligible, or the first ineligible user. + */ + protected InstructionOrPhi areInstanceUsersEligible(Supplier<InliningOracle> defaultOracle) { // No Phi users. if (eligibleInstance.numberOfPhiUsers() > 0) { - return false; // Not eligible. + return eligibleInstance.firstPhiUser(); // Not eligible. } for (Instruction user : eligibleInstance.uniqueUsers()) { // Field read/write. - if (user.isInstanceGet() || - (user.isInstancePut() && user.asInstancePut().value() != eligibleInstance)) { + if (user.isInstanceGet() + || (user.isInstancePut() && user.asInstancePut().value() != eligibleInstance)) { DexField field = user.asFieldInstruction().getField(); - if (field.clazz == eligibleClass && - eligibleClassDefinition.lookupInstanceField(field) != null) { + if (field.clazz == eligibleClass + && eligibleClassDefinition.lookupInstanceField(field) != null) { // Since class inliner currently only supports classes directly extending // java.lang.Object, we don't need to worry about fields defined in superclasses. continue; } - return false; // Not eligible. + return user; // Not eligible. } - // Eligible constructor call (for new instance roots only). - if (user.isInvokeDirect() && root.isNewInstance()) { - InliningInfo inliningInfo = isEligibleConstructorCall(user.asInvokeDirect()); - if (inliningInfo != null) { - methodCallsOnInstance.put(user.asInvokeDirect(), inliningInfo); - continue; - } - } - - // Eligible virtual method call on the instance as a receiver. - if (user.isInvokeVirtual() || user.isInvokeInterface()) { - InliningInfo inliningInfo = - isEligibleDirectVirtualMethodCall(user.asInvokeMethodWithReceiver()); - if (inliningInfo != null) { - methodCallsOnInstance.put(user.asInvokeMethodWithReceiver(), inliningInfo); - continue; - } - } - - // Eligible usage as an invocation argument. if (user.isInvokeMethod()) { - if (isExtraMethodCallEligible(defaultOracle, user.asInvokeMethod(), invocationContext)) { - continue; + InvokeMethod invokeMethod = user.asInvokeMethod(); + DexEncodedMethod singleTarget = findSingleTarget(invokeMethod); + if (!isEligibleSingleTarget(singleTarget)) { + return user; // Not eligible. } + + // Eligible constructor call (for new instance roots only). + if (user.isInvokeDirect()) { + InvokeDirect invoke = user.asInvokeDirect(); + if (factory.isConstructor(invoke.getInvokedMethod())) { + boolean isCorrespondingConstructorCall = + root.isNewInstance() + && !invoke.inValues().isEmpty() + && root.outValue() == invoke.inValues().get(0); + if (isCorrespondingConstructorCall) { + InliningInfo inliningInfo = + isEligibleConstructorCall(user.asInvokeDirect(), singleTarget, defaultOracle); + if (inliningInfo != null) { + methodCallsOnInstance.put(user.asInvokeDirect(), inliningInfo); + continue; + } + } + } + } + + // Eligible virtual method call on the instance as a receiver. + if (user.isInvokeVirtual() || user.isInvokeInterface()) { + InvokeMethodWithReceiver invoke = user.asInvokeMethodWithReceiver(); + InliningInfo inliningInfo = isEligibleDirectVirtualMethodCall(invoke, singleTarget); + if (inliningInfo != null) { + methodCallsOnInstance.put(invoke, inliningInfo); + continue; + } + } + + // Eligible usage as an invocation argument. + if (isExtraMethodCall(invokeMethod)) { + assert !invokeMethod.isInvokeSuper(); + assert !invokeMethod.isInvokePolymorphic(); + if (isExtraMethodCallEligible(invokeMethod, singleTarget, defaultOracle)) { + continue; + } + } + + return user; // Not eligible. } // Allow some IF instructions. @@ -274,9 +306,9 @@ } } - return false; // Not eligible. + return user; // Not eligible. } - return true; + return null; // Eligible. } // Process inlining, includes the following steps: @@ -289,10 +321,34 @@ // * remove root instruction // // Returns `true` if at least one method was inlined. - boolean processInlining(IRCode code, InlinerAction inliner) { + boolean processInlining(IRCode code, Supplier<InliningOracle> defaultOracle) { replaceUsagesAsUnusedArgument(code); - boolean anyInlinedMethods = forceInlineExtraMethodInvocations(inliner); - anyInlinedMethods |= forceInlineDirectMethodInvocations(inliner); + + boolean anyInlinedMethods = forceInlineExtraMethodInvocations(code); + if (anyInlinedMethods) { + // Reset the collections. + methodCallsOnInstance.clear(); + extraMethodCalls.clear(); + unusedArguments.clear(); + estimatedCombinedSizeForInlining = 0; + + // Repeat user analysis + InstructionOrPhi ineligibleUser = areInstanceUsersEligible(defaultOracle); + if (ineligibleUser != null) { + // We introduced a user that we cannot handle in the class inliner as a result of force + // inlining. Abort gracefully from class inlining without removing the instance. + // + // Alternatively we would need to collect additional information about the behavior of + // methods (which is bad for memory), or we would need to analyze the called methods before + // inlining them. The latter could be good solution, since we are going to build IR for the + // methods that need to be inlined anyway. + return true; + } + assert extraMethodCalls.isEmpty(); + assert unusedArguments.isEmpty(); + } + + anyInlinedMethods |= forceInlineDirectMethodInvocations(code); removeMiscUsages(code); removeFieldReads(code); removeFieldWrites(); @@ -317,36 +373,20 @@ unusedArguments.clear(); } - private boolean forceInlineExtraMethodInvocations(InlinerAction inliner) { + private boolean forceInlineExtraMethodInvocations(IRCode code) { if (extraMethodCalls.isEmpty()) { return false; } - // Inline extra methods. - inliner.inline(extraMethodCalls); - - // Reset the collections. - methodCallsOnInstance.clear(); - extraMethodCalls.clear(); - unusedArguments.clear(); - estimatedCombinedSizeForInlining = 0; - - // Repeat user analysis - if (!areInstanceUsersEligible(null, () -> { - throw new Unreachable("Inlining oracle is expected to be needed"); - })) { - throw new Unreachable("Analysis must succeed after inlining of extra methods"); - } - assert extraMethodCalls.isEmpty(); - assert unusedArguments.isEmpty(); + inliner.performForcedInlining(method, code, extraMethodCalls); return true; } - private boolean forceInlineDirectMethodInvocations(InlinerAction inliner) { + private boolean forceInlineDirectMethodInvocations(IRCode code) { if (methodCallsOnInstance.isEmpty()) { return false; } - inliner.inline(methodCallsOnInstance); + inliner.performForcedInlining(method, code, methodCallsOnInstance); return true; } @@ -390,7 +430,11 @@ continue; } - throw new Unreachable("Unexpected usage left after method inlining: " + user); + throw new Unreachable( + "Unexpected usage left in method `" + + method.method.toSourceString() + + "` after inlining: " + + user); } if (needToRemoveUnreachableBlocks) { @@ -414,7 +458,11 @@ continue; } - throw new Unreachable("Unexpected usage left after method inlining: " + user); + throw new Unreachable( + "Unexpected usage left in method `" + + method.method.toSourceString() + + "` after inlining: " + + user); } } @@ -437,45 +485,57 @@ private void removeFieldWrites() { for (Instruction user : eligibleInstance.uniqueUsers()) { if (!user.isInstancePut()) { - throw new Unreachable("Unexpected usage left after field reads removed: " + user); + throw new Unreachable( + "Unexpected usage left in method `" + + method.method.toSourceString() + + "` after field reads removed: " + + user); } if (user.asInstancePut().getField().clazz != eligibleClass) { - throw new Unreachable("Unexpected field write left after field reads removed: " + user); + throw new Unreachable( + "Unexpected field write left in method `" + + method.method.toSourceString() + + "` after field reads removed: " + + user); } removeInstruction(user); } } - private InliningInfo isEligibleConstructorCall(InvokeDirect initInvoke) { - // Must be a constructor of the exact same class. - DexMethod init = initInvoke.getInvokedMethod(); - if (!factory.isConstructor(init)) { - return null; - } + private InliningInfo isEligibleConstructorCall( + InvokeDirect invoke, DexEncodedMethod singleTarget, Supplier<InliningOracle> defaultOracle) { + assert factory.isConstructor(invoke.getInvokedMethod()); + assert isEligibleSingleTarget(singleTarget); + // Must be a constructor called on the receiver. - if (initInvoke.inValues().lastIndexOf(eligibleInstance) != 0) { + if (invoke.inValues().lastIndexOf(eligibleInstance) != 0) { return null; } - assert init.holder == eligibleClass - : "Inlined constructor? [invoke: " + initInvoke + - ", expected class: " + eligibleClass + "]"; + DexMethod init = invoke.getInvokedMethod(); + // Must be a constructor of the exact same class. + if (init.holder != eligibleClass) { + // Calling a constructor on a class that is different from the type of the instance. + // Gracefully abort class inlining (see the test B116282409). + return null; + } - DexEncodedMethod definition = findSingleTarget(init, true); - if (definition == null || isProcessedConcurrently.test(definition)) { + // Check that the `eligibleInstance` does not escape via the constructor. + ParameterUsage parameterUsage = singleTarget.getOptimizationInfo().getParameterUsages(0); + if (!isEligibleParameterUsage(parameterUsage, invoke, defaultOracle)) { return null; } // Don't inline code w/o normal returns into block with catch handlers (b/64432527). - if (initInvoke.getBlock().hasCatchHandlers() && - definition.getOptimizationInfo().neverReturnsNormally()) { + if (invoke.getBlock().hasCatchHandlers() + && singleTarget.getOptimizationInfo().neverReturnsNormally()) { return null; } if (isDesugaredLambda) { // Lambda desugaring synthesizes eligible constructors. - markSizeForInlining(definition); - return new InliningInfo(definition, eligibleClass); + markSizeForInlining(singleTarget); + return new InliningInfo(singleTarget, eligibleClass); } // If the superclass of the initializer is NOT java.lang.Object, the super class @@ -485,34 +545,27 @@ // any class initializers in superclass chain or in superinterfaces, see // details in ClassInliner::computeClassEligible(...). if (eligibleClassDefinition.superType != factory.objectType) { - TrivialInitializer info = definition.getOptimizationInfo().getTrivialInitializerInfo(); + TrivialInitializer info = singleTarget.getOptimizationInfo().getTrivialInitializerInfo(); if (!(info instanceof TrivialInstanceInitializer)) { return null; } } - if (!definition.isInliningCandidate(method, Reason.SIMPLE, appInfo)) { - // We won't be able to inline it here. - - // Note that there may be some false negatives here since the method may - // reference private fields of its class which are supposed to be replaced - // with arguments after inlining. We should try and improve it later. - - // Using -allowaccessmodification mitigates this. - return null; - } - - return definition.getOptimizationInfo().getClassInlinerEligibility() != null - ? new InliningInfo(definition, eligibleClass) : null; + return singleTarget.getOptimizationInfo().getClassInlinerEligibility() != null + ? new InliningInfo(singleTarget, eligibleClass) + : null; } - private InliningInfo isEligibleDirectVirtualMethodCall(InvokeMethodWithReceiver invoke) { + private InliningInfo isEligibleDirectVirtualMethodCall( + InvokeMethodWithReceiver invoke, DexEncodedMethod singleTarget) { + assert isEligibleSingleTarget(singleTarget); if (invoke.inValues().lastIndexOf(eligibleInstance) > 0) { return null; // Instance passed as an argument. } return isEligibleVirtualMethodCall( !invoke.getBlock().hasCatchHandlers(), invoke.getInvokedMethod(), + singleTarget, eligibility -> !eligibility.returnsReceiver || invoke.outValue() == null @@ -520,13 +573,21 @@ } private InliningInfo isEligibleIndirectVirtualMethodCall(DexMethod callee) { - return isEligibleVirtualMethodCall(false, callee, eligibility -> !eligibility.returnsReceiver); + DexEncodedMethod singleTarget = eligibleClassDefinition.lookupVirtualMethod(callee); + if (isEligibleSingleTarget(singleTarget)) { + return isEligibleVirtualMethodCall( + false, callee, singleTarget, eligibility -> !eligibility.returnsReceiver); + } + return null; } private InliningInfo isEligibleVirtualMethodCall( boolean allowMethodsWithoutNormalReturns, DexMethod callee, + DexEncodedMethod singleTarget, Predicate<ClassInlinerEligibility> eligibilityAcceptanceCheck) { + assert isEligibleSingleTarget(singleTarget); + // We should not inline a method if the invocation has type interface or virtual and the // signature of the invocation resolves to a private or static method. ResolutionResult resolutionResult = appInfo.resolveMethod(callee.holder, callee); @@ -534,23 +595,13 @@ && !resolutionResult.asSingleTarget().isVirtualMethod()) { return null; } - - DexEncodedMethod singleTarget = findSingleTarget(callee, false); - if (singleTarget == null - || !singleTarget.isVirtualMethod() - || isProcessedConcurrently.test(singleTarget)) { + if (!singleTarget.isVirtualMethod()) { return null; } if (method == singleTarget) { return null; // Don't inline itself. } - if (isDesugaredLambda) { - // If this is the call to method of the desugared lambda, we consider only calls - // to main lambda method eligible (for both direct and indirect calls). - if (singleTarget.accessFlags.isBridge()) { - return null; - } markSizeForInlining(singleTarget); return new InliningInfo(singleTarget, eligibleClass); } @@ -573,21 +624,27 @@ return null; } - if (!singleTarget.isInliningCandidate(method, Reason.SIMPLE, appInfo)) { - // We won't be able to inline it here. - - // Note that there may be some false negatives here since the method may - // reference private fields of its class which are supposed to be replaced - // with arguments after inlining. We should try and improve it later. - - // Using -allowaccessmodification mitigates this. - return null; - } - markSizeForInlining(singleTarget); return new InliningInfo(singleTarget, eligibleClass); } + private boolean isExtraMethodCall(InvokeMethod invoke) { + if (invoke.isInvokeDirect() && factory.isConstructor(invoke.getInvokedMethod())) { + return false; + } + if (invoke.isInvokeMethodWithReceiver() + && invoke.asInvokeMethodWithReceiver().getReceiver() == eligibleInstance) { + return false; + } + if (invoke.isInvokeSuper()) { + return false; + } + if (invoke.isInvokePolymorphic()) { + return false; + } + return true; + } + // Analyzes if a method invoke the eligible instance is passed to is eligible. In short, // it can be eligible if: // @@ -607,42 +664,52 @@ // -- method itself can be inlined // private boolean isExtraMethodCallEligible( - Supplier<InliningOracle> defaultOracle, - InvokeMethod invokeMethod, - DexType invocationContext) { + InvokeMethod invoke, DexEncodedMethod singleTarget, Supplier<InliningOracle> defaultOracle) { + // Don't consider constructor invocations and super calls, since we don't want to forcibly + // inline them. + assert isExtraMethodCall(invoke); + assert isEligibleSingleTarget(singleTarget); - List<Value> arguments = Lists.newArrayList(invokeMethod.inValues()); + List<Value> arguments = Lists.newArrayList(invoke.inValues()); - // Don't consider constructor invocations and super calls, since - // we don't want to forcibly inline them. - if (invokeMethod.isInvokeSuper() || - (invokeMethod.isInvokeDirect() && factory.isConstructor(invokeMethod.getInvokedMethod()))) { + // If we got here with invocation on receiver the user is ineligible. + if (invoke.isInvokeMethodWithReceiver() && arguments.get(0) == eligibleInstance) { return false; } - // Remove receiver from arguments. - if (invokeMethod.isInvokeMethodWithReceiver()) { - if (arguments.get(0) == eligibleInstance) { - // If we got here with invocation on receiver the user is ineligible. - return false; - } - arguments.remove(0); - } - - // Need single target. - DexEncodedMethod singleTarget = invokeMethod.lookupSingleTarget(appInfo, invocationContext); - if (singleTarget == null || isProcessedConcurrently.test(singleTarget)) { - return false; // Not eligible. - } - OptimizationInfo optimizationInfo = singleTarget.getOptimizationInfo(); // Don't inline code w/o normal returns into block with catch handlers (b/64432527). - if (invokeMethod.getBlock().hasCatchHandlers() && optimizationInfo.neverReturnsNormally()) { + if (invoke.getBlock().hasCatchHandlers() && optimizationInfo.neverReturnsNormally()) { return false; } // Go through all arguments, see if all usages of eligibleInstance are good. + if (!isEligibleParameterUsages(invoke, arguments, singleTarget, defaultOracle)) { + return false; + } + + for (int argIndex = 0; argIndex < arguments.size(); argIndex++) { + Value argument = arguments.get(argIndex); + if (argument == eligibleInstance && optimizationInfo.getParameterUsages(argIndex).notUsed()) { + // Reference can be removed since it's not used. + unusedArguments.add(new Pair<>(invoke, argIndex)); + } + } + + extraMethodCalls.put(invoke, new InliningInfo(singleTarget, null)); + + // Looks good. + markSizeForInlining(singleTarget); + return true; + } + + private boolean isEligibleParameterUsages( + InvokeMethod invoke, + List<Value> arguments, + DexEncodedMethod singleTarget, + Supplier<InliningOracle> defaultOracle) { + // Go through all arguments, see if all usages of eligibleInstance are good. for (int argIndex = 0; argIndex < arguments.size(); argIndex++) { Value argument = arguments.get(argIndex); if (argument != eligibleInstance) { @@ -650,56 +717,79 @@ } // Have parameter usage info? + OptimizationInfo optimizationInfo = singleTarget.getOptimizationInfo(); ParameterUsage parameterUsage = optimizationInfo.getParameterUsages(argIndex); - if (parameterUsage == null) { - return false; // Don't know anything. + if (!isEligibleParameterUsage(parameterUsage, invoke, defaultOracle)) { + return false; } + } + return true; + } - if (parameterUsage.notUsed()) { - // Reference can be removed since it's not used. - unusedArguments.add(new Pair<>(invokeMethod, argIndex)); - continue; - } + private boolean isEligibleParameterUsage( + ParameterUsage parameterUsage, InvokeMethod invoke, Supplier<InliningOracle> defaultOracle) { + if (parameterUsage == null) { + return false; // Don't know anything. + } - if (parameterUsage.returnValue && - !(invokeMethod.outValue() == null || invokeMethod.outValue().numberOfAllUsers() == 0)) { + if (parameterUsage.notUsed()) { + return true; + } + + if (parameterUsage.isAssignedToField) { + return false; + } + + if (parameterUsage.isReturned) { + if (!(invoke.outValue() == null || invoke.outValue().numberOfAllUsers() == 0)) { // Used as return value which is not ignored. return false; } + } - if (!Sets.difference(parameterUsage.ifZeroTest, ALLOWED_ZERO_TEST_TYPES).isEmpty()) { - // Used in unsupported zero-check-if kinds. - return false; - } + if (!Sets.difference(parameterUsage.ifZeroTest, ALLOWED_ZERO_TEST_TYPES).isEmpty()) { + // Used in unsupported zero-check-if kinds. + return false; + } - for (Pair<Type, DexMethod> call : parameterUsage.callsReceiver) { - if (call.getFirst() != Type.VIRTUAL && call.getFirst() != Type.INTERFACE) { - // Don't support direct and super calls yet. - return false; - } + for (Pair<Type, DexMethod> call : parameterUsage.callsReceiver) { + Type type = call.getFirst(); + DexMethod target = call.getSecond(); + if (type == Type.VIRTUAL || type == Type.INTERFACE) { // Is the method called indirectly still eligible? - InliningInfo potentialInliningInfo = isEligibleIndirectVirtualMethodCall(call.getSecond()); + InliningInfo potentialInliningInfo = isEligibleIndirectVirtualMethodCall(target); if (potentialInliningInfo == null) { return false; } - - // Check if the method is inline-able by standard inliner. - InlineAction inlineAction = - invokeMethod.computeInlining(defaultOracle.get(), method.method.holder); - if (inlineAction == null) { + } else if (type == Type.DIRECT) { + if (!isTrivialInitializer(target)) { + // Only calls to trivial initializers are supported at this point. return false; } + } else { + // Static and super calls are not yet supported. + return false; } - extraMethodCalls.put(invokeMethod, new InliningInfo(singleTarget, null)); + // Check if the method is inline-able by standard inliner. + InlineAction inlineAction = invoke.computeInlining(defaultOracle.get(), method.method.holder); + if (inlineAction == null) { + return false; + } } - - // Looks good. - markSizeForInlining(singleTarget); return true; } + private boolean isTrivialInitializer(DexMethod method) { + if (method == appInfo.dexItemFactory.objectMethods.constructor) { + return true; + } + DexEncodedMethod encodedMethod = appInfo.definitionFor(method); + return encodedMethod != null + && encodedMethod.getOptimizationInfo().getTrivialInitializerInfo() != null; + } + private boolean exemptFromInstructionLimit(DexEncodedMethod inlinee) { DexType inlineeHolder = inlinee.method.holder; if (isDesugaredLambda && inlineeHolder == eligibleClass) { @@ -723,20 +813,49 @@ } } - private boolean isNewInstance() { - return root.isNewInstance(); - } - - private DexEncodedMethod findSingleTarget(DexMethod callee, boolean isDirect) { + private DexEncodedMethod findSingleTarget(InvokeMethod invoke) { + if (isExtraMethodCall(invoke)) { + DexType invocationContext = method.method.holder; + return invoke.lookupSingleTarget(appInfo, invocationContext); + } // We don't use computeSingleTarget(...) on invoke since it sometimes fails to // find the single target, while this code may be more successful since we exactly // know what is the actual type of the receiver. // Note that we also intentionally limit ourselves to methods directly defined in // the instance's class. This may be improved later. - return isDirect - ? eligibleClassDefinition.lookupDirectMethod(callee) - : eligibleClassDefinition.lookupVirtualMethod(callee); + return invoke.isInvokeDirect() + ? eligibleClassDefinition.lookupDirectMethod(invoke.getInvokedMethod()) + : eligibleClassDefinition.lookupVirtualMethod(invoke.getInvokedMethod()); + } + + private boolean isEligibleSingleTarget(DexEncodedMethod singleTarget) { + if (singleTarget == null) { + return false; + } + if (isProcessedConcurrently.test(singleTarget)) { + return false; + } + if (isDesugaredLambda && !singleTarget.accessFlags.isBridge()) { + // OK if this is the call to the main method of a desugared lambda (for both direct and + // indirect calls). + // + // Note: This is needed because lambda methods are generally processed in the same batch as + // they are class inlined, which means that the call to isInliningCandidate() below will + // return false. + return true; + } + if (!singleTarget.isInliningCandidate(method, Reason.SIMPLE, appInfo)) { + // If `singleTarget` is not an inlining candidate, we won't be able to inline it here. + // + // Note that there may be some false negatives here since the method may + // reference private fields of its class which are supposed to be replaced + // with arguments after inlining. We should try and improve it later. + // + // Using -allowaccessmodification mitigates this. + return false; + } + return true; } private void removeInstruction(Instruction instruction) {
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/staticizer/StaticizingProcessor.java b/src/main/java/com/android/tools/r8/ir/optimize/staticizer/StaticizingProcessor.java index 79f18ed..ee7edd0 100644 --- a/src/main/java/com/android/tools/r8/ir/optimize/staticizer/StaticizingProcessor.java +++ b/src/main/java/com/android/tools/r8/ir/optimize/staticizer/StaticizingProcessor.java
@@ -81,14 +81,12 @@ processMethodsConcurrently(methodsToBeStaticized, this::removeReferencesToThis, feedback); // Convert instance methods into static methods with an extra parameter. - Set<DexEncodedMethod> staticizedMethods = staticizeMethodSymbols(); + Set<DexEncodedMethod> methods = staticizeMethodSymbols(); // Process all other methods that may reference singleton fields and call methods on them. // (Note that we exclude the former instance methods, but include new static methods created as // a result of staticizing.) - Set<DexEncodedMethod> methods = Sets.newIdentityHashSet(); methods.addAll(referencingExtraMethods); - methods.addAll(staticizedMethods); methods.addAll(hostClassInits.keySet()); processMethodsConcurrently(methods, this::rewriteReferences, feedback); }
diff --git a/src/main/java/com/android/tools/r8/shaking/ClassInlineRule.java b/src/main/java/com/android/tools/r8/shaking/ClassInlineRule.java new file mode 100644 index 0000000..e7dc76c --- /dev/null +++ b/src/main/java/com/android/tools/r8/shaking/ClassInlineRule.java
@@ -0,0 +1,105 @@ +// 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; + +import com.android.tools.r8.errors.Unreachable; +import com.android.tools.r8.origin.Origin; +import com.android.tools.r8.position.Position; +import java.util.List; + +public class ClassInlineRule extends ProguardConfigurationRule { + + public enum Type { + NEVER + } + + public static class Builder extends ProguardConfigurationRule.Builder<ClassInlineRule, Builder> { + + private Builder() { + super(); + } + + Type type; + + @Override + public Builder self() { + return this; + } + + public Builder setType(Type type) { + this.type = type; + return this; + } + + @Override + public ClassInlineRule build() { + return new ClassInlineRule( + origin, + getPosition(), + source, + classAnnotation, + classAccessFlags, + negatedClassAccessFlags, + classTypeNegated, + classType, + classNames, + inheritanceAnnotation, + inheritanceClassName, + inheritanceIsExtends, + memberRules, + type); + } + } + + private final Type type; + + protected ClassInlineRule( + Origin origin, + Position position, + String source, + ProguardTypeMatcher classAnnotation, + ProguardAccessFlags classAccessFlags, + ProguardAccessFlags negatedClassAccessFlags, + boolean classTypeNegated, + ProguardClassType classType, + ProguardClassNameList classNames, + ProguardTypeMatcher inheritanceAnnotation, + ProguardTypeMatcher inheritanceClassName, + boolean inheritanceIsExtends, + List<ProguardMemberRule> memberRules, + Type type) { + super( + origin, + position, + source, + classAnnotation, + classAccessFlags, + negatedClassAccessFlags, + classTypeNegated, + classType, + classNames, + inheritanceAnnotation, + inheritanceClassName, + inheritanceIsExtends, + memberRules); + this.type = type; + } + + public static Builder builder() { + return new Builder(); + } + + public Type getType() { + return type; + } + + @Override + String typeString() { + switch (type) { + case NEVER: + return "neverclassinline"; + } + throw new Unreachable("Unknown class inline type " + type); + } +}
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 fc95770..31d60ea 100644 --- a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java +++ b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
@@ -1807,6 +1807,10 @@ */ public final Set<DexMethod> neverInline; /** + * All types that *must* never be inlined due to a configuration directive (testing only). + */ + public final Set<DexType> neverClassInline; + /** * All types that *must* never be merged due to a configuration directive (testing only). */ public final Set<DexType> neverMerge; @@ -1869,6 +1873,7 @@ this.alwaysInline = enqueuer.rootSet.alwaysInline; this.forceInline = enqueuer.rootSet.forceInline; this.neverInline = enqueuer.rootSet.neverInline; + this.neverClassInline = enqueuer.rootSet.neverClassInline; this.neverMerge = enqueuer.rootSet.neverMerge; this.identifierNameStrings = joinIdentifierNameStrings( enqueuer.rootSet.identifierNameStrings, enqueuer.identifierNameStrings); @@ -1912,6 +1917,7 @@ this.alwaysInline = previous.alwaysInline; this.forceInline = previous.forceInline; this.neverInline = previous.neverInline; + this.neverClassInline = previous.neverClassInline; this.neverMerge = previous.neverMerge; this.identifierNameStrings = previous.identifierNameStrings; this.prunedTypes = mergeSets(previous.prunedTypes, removedClasses); @@ -1968,6 +1974,7 @@ assert lense.assertDefinitionNotModified( previous.neverMerge.stream().map(this::definitionFor).filter(Objects::nonNull) .collect(Collectors.toList())); + this.neverClassInline = rewriteItems(previous.neverClassInline, lense::lookupType); this.neverMerge = previous.neverMerge; this.identifierNameStrings = lense.rewriteReferencesConservatively(previous.identifierNameStrings); @@ -2012,6 +2019,7 @@ this.alwaysInline = previous.alwaysInline; this.forceInline = previous.forceInline; this.neverInline = previous.neverInline; + this.neverClassInline = previous.neverClassInline; this.neverMerge = previous.neverMerge; this.identifierNameStrings = previous.identifierNameStrings; this.prunedTypes = previous.prunedTypes;
diff --git a/src/main/java/com/android/tools/r8/shaking/ProguardConfigurationParser.java b/src/main/java/com/android/tools/r8/shaking/ProguardConfigurationParser.java index 7d5f9a5..7694299 100644 --- a/src/main/java/com/android/tools/r8/shaking/ProguardConfigurationParser.java +++ b/src/main/java/com/android/tools/r8/shaking/ProguardConfigurationParser.java
@@ -16,7 +16,6 @@ import com.android.tools.r8.position.Position; import com.android.tools.r8.position.TextPosition; import com.android.tools.r8.position.TextRange; -import com.android.tools.r8.shaking.InlineRule.Type; import com.android.tools.r8.shaking.ProguardConfiguration.Builder; import com.android.tools.r8.shaking.ProguardTypeMatcher.ClassOrType; import com.android.tools.r8.shaking.ProguardTypeMatcher.MatchSpecificType; @@ -353,16 +352,19 @@ } else if (acceptString("packageobfuscationdictionary")) { configurationBuilder.setPackageObfuscationDictionary(parseFileName(false)); } else if (acceptString("alwaysinline")) { - InlineRule rule = parseInlineRule(Type.ALWAYS, optionStart); + InlineRule rule = parseInlineRule(InlineRule.Type.ALWAYS, optionStart); configurationBuilder.addRule(rule); } else if (allowTestOptions && acceptString("forceinline")) { - InlineRule rule = parseInlineRule(Type.FORCE, optionStart); + InlineRule rule = parseInlineRule(InlineRule.Type.FORCE, optionStart); configurationBuilder.addRule(rule); // Insert a matching -checkdiscard rule to ensure force inlining happens. ProguardCheckDiscardRule ruled = rule.asProguardCheckDiscardRule(); configurationBuilder.addRule(ruled); } else if (allowTestOptions && acceptString("neverinline")) { - InlineRule rule = parseInlineRule(Type.NEVER, optionStart); + InlineRule rule = parseInlineRule(InlineRule.Type.NEVER, optionStart); + configurationBuilder.addRule(rule); + } else if (allowTestOptions && acceptString("neverclassinline")) { + ClassInlineRule rule = parseClassInlineRule(ClassInlineRule.Type.NEVER, optionStart); configurationBuilder.addRule(rule); } else if (allowTestOptions && acceptString("nevermerge")) { ClassMergingRule rule = parseClassMergingRule(ClassMergingRule.Type.NEVER, optionStart); @@ -597,6 +599,17 @@ return keepRuleBuilder.build(); } + private ClassInlineRule parseClassInlineRule(ClassInlineRule.Type type, Position start) + throws ProguardRuleParserException { + ClassInlineRule.Builder keepRuleBuilder = + ClassInlineRule.builder().setOrigin(origin).setStart(start).setType(type); + parseClassSpec(keepRuleBuilder, false); + Position end = getPosition(); + keepRuleBuilder.setSource(getSourceSnippet(contents, start, end)); + keepRuleBuilder.setEnd(end); + return keepRuleBuilder.build(); + } + private ClassMergingRule parseClassMergingRule(ClassMergingRule.Type type, Position start) throws ProguardRuleParserException { ClassMergingRule.Builder keepRuleBuilder =
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 d90812c..ff87d3d 100644 --- a/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java +++ b/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java
@@ -65,6 +65,7 @@ private final Set<DexMethod> alwaysInline = Sets.newIdentityHashSet(); private final Set<DexMethod> forceInline = Sets.newIdentityHashSet(); private final Set<DexMethod> neverInline = Sets.newIdentityHashSet(); + private final Set<DexType> neverClassInline = Sets.newIdentityHashSet(); private final Set<DexType> neverMerge = Sets.newIdentityHashSet(); private final Map<DexDefinition, Map<DexDefinition, ProguardKeepRule>> dependentNoShrinking = new IdentityHashMap<>(); @@ -263,6 +264,10 @@ } } else if (rule instanceof InlineRule) { markMatchingMethods(clazz, memberKeepRules, rule, null); + } else if (rule instanceof ClassInlineRule) { + if (allRulesSatisfied(memberKeepRules, clazz)) { + markClass(clazz, rule); + } } else if (rule instanceof ProguardAssumeValuesRule) { markMatchingVisibleMethods(clazz, memberKeepRules, rule, null); markMatchingVisibleFields(clazz, memberKeepRules, rule, null); @@ -333,6 +338,7 @@ alwaysInline, forceInline, neverInline, + neverClassInline, neverMerge, noSideEffects, assumedValues, @@ -812,20 +818,26 @@ } else if (context instanceof ProguardCheckDiscardRule) { checkDiscarded.add(item); } else if (context instanceof InlineRule) { - switch (((InlineRule) context).getType()) { - case ALWAYS: - if (item.isDexEncodedMethod()) { + if (item.isDexEncodedMethod()) { + switch (((InlineRule) context).getType()) { + case ALWAYS: alwaysInline.add(item.asDexEncodedMethod().method); - } - break; - case FORCE: - if (item.isDexEncodedMethod()) { + break; + case FORCE: forceInline.add(item.asDexEncodedMethod().method); - } - break; - case NEVER: - if (item.isDexEncodedMethod()) { + break; + case NEVER: neverInline.add(item.asDexEncodedMethod().method); + break; + default: + throw new Unreachable(); + } + } + } else if (context instanceof ClassInlineRule) { + switch (((ClassInlineRule) context).getType()) { + case NEVER: + if (item.isDexClass()) { + neverClassInline.add(item.asDexClass().type); } break; default: @@ -861,6 +873,7 @@ public final Set<DexMethod> alwaysInline; public final Set<DexMethod> forceInline; public final Set<DexMethod> neverInline; + public final Set<DexType> neverClassInline; public final Set<DexType> neverMerge; public final Map<DexDefinition, ProguardMemberRule> noSideEffects; public final Map<DexDefinition, ProguardMemberRule> assumedValues; @@ -878,6 +891,7 @@ Set<DexMethod> alwaysInline, Set<DexMethod> forceInline, Set<DexMethod> neverInline, + Set<DexType> neverClassInline, Set<DexType> neverMerge, Map<DexDefinition, ProguardMemberRule> noSideEffects, Map<DexDefinition, ProguardMemberRule> assumedValues, @@ -893,6 +907,7 @@ this.alwaysInline = Collections.unmodifiableSet(alwaysInline); this.forceInline = Collections.unmodifiableSet(forceInline); this.neverInline = Collections.unmodifiableSet(neverInline); + this.neverClassInline = Collections.unmodifiableSet(neverClassInline); this.neverMerge = Collections.unmodifiableSet(neverMerge); this.noSideEffects = Collections.unmodifiableMap(noSideEffects); this.assumedValues = Collections.unmodifiableMap(assumedValues);
diff --git a/src/test/examples/inlining/AlwaysInline.java b/src/test/examples/inlining/AlwaysInline.java index 815d50a..22cb31e 100644 --- a/src/test/examples/inlining/AlwaysInline.java +++ b/src/test/examples/inlining/AlwaysInline.java
@@ -3,6 +3,8 @@ // BSD-style license that can be found in the LICENSE file. package inlining; -public @interface AlwaysInline { +import java.lang.annotation.ElementType; +import java.lang.annotation.Target; -} +@Target({ElementType.METHOD}) +public @interface AlwaysInline {}
diff --git a/src/test/examples/inlining/NeverInline.java b/src/test/examples/inlining/NeverInline.java new file mode 100644 index 0000000..0c5b581 --- /dev/null +++ b/src/test/examples/inlining/NeverInline.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 inlining; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Target; + +@Target({ElementType.METHOD}) +public @interface NeverInline {}
diff --git a/src/test/java/com/android/tools/r8/NeverClassInline.java b/src/test/java/com/android/tools/r8/NeverClassInline.java new file mode 100644 index 0000000..aa7f9b5 --- /dev/null +++ b/src/test/java/com/android/tools/r8/NeverClassInline.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; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Target; + +@Target({ElementType.TYPE}) +public @interface NeverClassInline {}
diff --git a/src/test/java/com/android/tools/r8/NeverMerge.java b/src/test/java/com/android/tools/r8/NeverMerge.java index 7fc97b9..7c6922a 100644 --- a/src/test/java/com/android/tools/r8/NeverMerge.java +++ b/src/test/java/com/android/tools/r8/NeverMerge.java
@@ -3,4 +3,8 @@ // BSD-style license that can be found in the LICENSE file. package com.android.tools.r8; +import java.lang.annotation.ElementType; +import java.lang.annotation.Target; + +@Target({ElementType.TYPE}) public @interface NeverMerge {}
diff --git a/src/test/java/com/android/tools/r8/R8RunExamplesAndroidOTest.java b/src/test/java/com/android/tools/r8/R8RunExamplesAndroidOTest.java index 101fbb8..5f6f362 100644 --- a/src/test/java/com/android/tools/r8/R8RunExamplesAndroidOTest.java +++ b/src/test/java/com/android/tools/r8/R8RunExamplesAndroidOTest.java
@@ -108,7 +108,9 @@ .withOptionConsumer(opts -> opts.enableClassInlining = true) .withBuilderTransformation( b -> b.addProguardConfiguration(PROGUARD_OPTIONS, Origin.unknown())) - .withDexCheck(inspector -> checkLambdaCount(inspector, 24, "lambdadesugaring")) + // TODO(b/120814598): Should be 24. Some lambdas are not class inlined because parameter + // usages for lambda methods are not present for the class inliner. + .withDexCheck(inspector -> checkLambdaCount(inspector, 46, "lambdadesugaring")) .run(); } @@ -128,7 +130,9 @@ .withOptionConsumer(opts -> opts.enableClassInlining = true) .withBuilderTransformation( b -> b.addProguardConfiguration(PROGUARD_OPTIONS, Origin.unknown())) - .withDexCheck(inspector -> checkLambdaCount(inspector, 24, "lambdadesugaring")) + // TODO(b/120814598): Should be 24. Some lambdas are not class inlined because parameter + // usages for lambda methods are not present for the class inliner. + .withDexCheck(inspector -> checkLambdaCount(inspector, 46, "lambdadesugaring")) .run(); } @@ -150,7 +154,9 @@ .withOptionConsumer(opts -> opts.enableClassInlining = true) .withBuilderTransformation( b -> b.addProguardConfiguration(PROGUARD_OPTIONS_N_PLUS, Origin.unknown())) - .withDexCheck(inspector -> checkLambdaCount(inspector, 5, "lambdadesugaringnplus")) + // TODO(b/120814598): Should be 5. Some lambdas are not class inlined because parameter + // usages for lambda methods are not present for the class inliner. + .withDexCheck(inspector -> checkLambdaCount(inspector, 22, "lambdadesugaringnplus")) .run(); } @@ -172,7 +178,9 @@ .withOptionConsumer(opts -> opts.enableClassInlining = true) .withBuilderTransformation( b -> b.addProguardConfiguration(PROGUARD_OPTIONS_N_PLUS, Origin.unknown())) - .withDexCheck(inspector -> checkLambdaCount(inspector, 5, "lambdadesugaringnplus")) + // TODO(b/120814598): Should be 5. Some lambdas are not class inlined because parameter + // usages for lambda methods are not present for the class inliner. + .withDexCheck(inspector -> checkLambdaCount(inspector, 22, "lambdadesugaringnplus")) .run(); }
diff --git a/src/test/java/com/android/tools/r8/R8TestBuilder.java b/src/test/java/com/android/tools/r8/R8TestBuilder.java index 3a249e6..5550415 100644 --- a/src/test/java/com/android/tools/r8/R8TestBuilder.java +++ b/src/test/java/com/android/tools/r8/R8TestBuilder.java
@@ -30,6 +30,7 @@ } private boolean enableInliningAnnotations = false; + private boolean enableClassInliningAnnotations = false; private boolean enableMergeAnnotations = false; @Override @@ -41,7 +42,7 @@ R8TestCompileResult internalCompile( Builder builder, Consumer<InternalOptions> optionsConsumer, Supplier<AndroidApp> app) throws CompilationFailedException { - if (enableInliningAnnotations || enableMergeAnnotations) { + if (enableInliningAnnotations || enableClassInliningAnnotations || enableMergeAnnotations) { ToolHelper.allowTestProguardOptions(builder); } StringBuilder proguardMapBuilder = new StringBuilder(); @@ -71,6 +72,14 @@ return self(); } + public R8TestBuilder enableClassInliningAnnotations() { + if (!enableClassInliningAnnotations) { + enableClassInliningAnnotations = true; + addKeepRules("-neverclassinline @com.android.tools.r8.NeverClassInline class *"); + } + return self(); + } + public R8TestBuilder enableMergeAnnotations() { if (!enableMergeAnnotations) { enableMergeAnnotations = true;
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/classinliner/BuilderWithInheritanceTest.java b/src/test/java/com/android/tools/r8/ir/optimize/classinliner/BuilderWithInheritanceTest.java new file mode 100644 index 0000000..c3deab3 --- /dev/null +++ b/src/test/java/com/android/tools/r8/ir/optimize/classinliner/BuilderWithInheritanceTest.java
@@ -0,0 +1,97 @@ +// 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.ir.optimize.classinliner; + +import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent; +import static org.junit.Assert.assertThat; + +import com.android.tools.r8.NeverInline; +import com.android.tools.r8.NeverMerge; +import com.android.tools.r8.TestBase; +import com.android.tools.r8.utils.codeinspector.CodeInspector; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameters; + +/** Regression test for b/120182628. */ +@RunWith(Parameterized.class) +public class BuilderWithInheritanceTest extends TestBase { + + private final Backend backend; + + @Parameters(name = "Backend: {0}") + public static Backend[] data() { + return Backend.values(); + } + + public BuilderWithInheritanceTest(Backend backend) { + this.backend = backend; + } + + @Test + public void test() throws Exception { + CodeInspector inspector = + testForR8(backend) + .addInnerClasses(BuilderWithInheritanceTest.class) + .addKeepMainRule(TestClass.class) + .enableInliningAnnotations() + .enableMergeAnnotations() + .run(TestClass.class) + .assertSuccessWithOutput("42") + .inspector(); + assertThat(inspector.clazz(Builder.class), isPresent()); + } + + static class TestClass { + + public static void main(String[] args) { + A a = new Builder(42).build(); + System.out.print(a.get()); + } + } + + static class A { + + private final int f; + + public A(int f) { + this.f = f; + } + + public int get() { + return f; + } + } + + @NeverMerge + static class BuilderBase { + + protected int f; + + public BuilderBase(int f) { + this.f = f; + } + + @NeverInline + public static int get(BuilderBase obj) { + return obj.f; + } + } + + static class Builder extends BuilderBase { + + public Builder(int f) { + super(f); + } + + public A build() { + // After force inlining of get() there will be a field-read "this.f", which is not allowed by + // the class inliner, because the class inliner only handles reads from fields that are + // declared on Builder. + return new A(get(this)); + } + } +}
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/classinliner/ClassInlinerTest.java b/src/test/java/com/android/tools/r8/ir/optimize/classinliner/ClassInlinerTest.java index caa697b..585a696 100644 --- a/src/test/java/com/android/tools/r8/ir/optimize/classinliner/ClassInlinerTest.java +++ b/src/test/java/com/android/tools/r8/ir/optimize/classinliner/ClassInlinerTest.java
@@ -4,6 +4,7 @@ package com.android.tools.r8.ir.optimize.classinliner; +import static com.android.tools.r8.ir.desugar.LambdaRewriter.LAMBDA_CLASS_NAME_PREFIX; import static org.hamcrest.CoreMatchers.containsString; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -43,6 +44,7 @@ import com.android.tools.r8.utils.codeinspector.ClassSubject; import com.android.tools.r8.utils.codeinspector.CodeInspector; import com.android.tools.r8.utils.codeinspector.FieldAccessInstructionSubject; +import com.android.tools.r8.utils.codeinspector.FoundClassSubject; import com.android.tools.r8.utils.codeinspector.InstructionSubject; import com.android.tools.r8.utils.codeinspector.NewInstanceInstructionSubject; import com.google.common.collect.Sets; @@ -352,14 +354,23 @@ "java.lang.StringBuilder"), collectTypes(clazz, "testStatelessLambda", "void")); + // TODO(b/120814598): Should only be "java.lang.StringBuilder". Lambdas are not class inlined + // because parameter usage is not available for each lambda constructor. + Set<String> expectedTypes = Sets.newHashSet("java.lang.StringBuilder"); + expectedTypes.addAll( + inspector.allClasses().stream() + .map(FoundClassSubject::getFinalName) + .filter(name -> name.contains(LAMBDA_CLASS_NAME_PREFIX)) + .collect(Collectors.toList())); assertEquals( - Sets.newHashSet( - "java.lang.StringBuilder"), + expectedTypes, collectTypes(clazz, "testStatefulLambda", "void", "java.lang.String", "java.lang.String")); - assertEquals(0, - inspector.allClasses().stream() - .filter(ClassSubject::isSynthesizedJavaLambdaClass).count()); + // TODO(b/120814598): Should be 0. Lambdas are not class inlined because parameter usage is not + // available for each lambda constructor. + assertEquals( + 3, + inspector.allClasses().stream().filter(ClassSubject::isSynthesizedJavaLambdaClass).count()); } private Set<String> collectTypes(
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/classinliner/ClassInliningOracleTest.java b/src/test/java/com/android/tools/r8/ir/optimize/classinliner/ClassInliningOracleTest.java new file mode 100644 index 0000000..6982656 --- /dev/null +++ b/src/test/java/com/android/tools/r8/ir/optimize/classinliner/ClassInliningOracleTest.java
@@ -0,0 +1,74 @@ +// 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.ir.optimize.classinliner; + +import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent; +import static org.junit.Assert.assertThat; + +import com.android.tools.r8.NeverClassInline; +import com.android.tools.r8.NeverInline; +import com.android.tools.r8.NeverMerge; +import com.android.tools.r8.TestBase; +import com.android.tools.r8.utils.codeinspector.CodeInspector; +import org.junit.Test; + +/** Regression test for b/121119666. */ +public class ClassInliningOracleTest extends TestBase { + + @Test + public void test() throws Exception { + CodeInspector inspector = + testForR8(Backend.DEX) + .addInnerClasses(ClassInliningOracleTest.class) + .addKeepMainRule(TestClass.class) + .enableInliningAnnotations() + .enableClassInliningAnnotations() + .enableMergeAnnotations() + .compile() + .inspector(); + assertThat(inspector.clazz(Builder.class), isPresent()); + } + + static class TestClass { + + public static void main(String[] args) { + // In order to class inline the Builder, we would need to force-inline the help() method. + // We can't do this alone, though, since force-inlining of help() would lead to an illegal + // invoke-super instruction in main(). + Builder builder = new Builder(); + new Helper().help(builder); + System.out.print(builder.build()); + } + } + + @NeverMerge + static class HelperBase { + + @NeverInline + public void hello() { + System.out.println("Hello"); + } + } + + @NeverClassInline + static class Helper extends HelperBase { + + @NeverInline + public void help(Builder builder) { + // TODO(b/120959040): To avoid unused argument removal; should be replaced by a testing rule). + if (builder != null) { + super.hello(); + } + } + } + + static class Builder { + + @NeverInline + public Object build() { + return new Object(); + } + } +}
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/classinliner/EscapingBuilderTest.java b/src/test/java/com/android/tools/r8/ir/optimize/classinliner/EscapingBuilderTest.java new file mode 100644 index 0000000..85857b3 --- /dev/null +++ b/src/test/java/com/android/tools/r8/ir/optimize/classinliner/EscapingBuilderTest.java
@@ -0,0 +1,166 @@ +// 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.ir.optimize.classinliner; + +import static com.android.tools.r8.ir.optimize.classinliner.EscapingBuilderTest.TestClass.escape; +import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent; +import static org.hamcrest.CoreMatchers.not; +import static org.junit.Assert.assertThat; + +import com.android.tools.r8.NeverInline; +import com.android.tools.r8.TestBase; +import com.android.tools.r8.utils.codeinspector.CodeInspector; +import org.junit.Test; + +/** Regression test for b/120182628. */ +public class EscapingBuilderTest extends TestBase { + + @Test + public void test() throws Exception { + testForR8(Backend.DEX) + .addInnerClasses(EscapingBuilderTest.class) + .addKeepMainRule(TestClass.class) + .enableInliningAnnotations() + .compile() + .inspect(this::inspect); + } + + private void inspect(CodeInspector inspector) { + assertThat(inspector.clazz(Builder0.class), not(isPresent())); + assertThat(inspector.clazz(Builder1.class), isPresent()); + assertThat(inspector.clazz(Builder2.class), isPresent()); + assertThat(inspector.clazz(Builder3.class), isPresent()); + assertThat(inspector.clazz(Builder4.class), isPresent()); + assertThat(inspector.clazz(Builder5.class), isPresent()); + } + + static class TestClass { + + public static void main(String[] args) { + testBuilder0(); + testBuilder1(); + testBuilder2(); + testBuilder3(); + testBuilder4(); + testBuilder5(); + } + + private static void testBuilder0() { + new Builder0().build(); + } + + private static void testBuilder1() { + new Builder1().build(); + } + + private static void testBuilder2() { + new Builder2().init().build(); + } + + private static void testBuilder3() { + Builder3 builder3 = new Builder3(); + builder3.init(builder3).build(); + } + + private static void testBuilder4() { + Builder4 builder4 = new Builder4(); + builder4.init(builder4).build(); + } + + private static void testBuilder5() { + Builder5.init(new Builder5()).build(); + } + + @NeverInline + public static void escape(Object o) {} + } + + // Simple builder that should be class inlined. + static class Builder0 { + + public Object build() { + return new Object(); + } + } + + // Builder that escapes via field `f` that is assigned in the constructor. + static class Builder1 { + + public Builder1 f = this; + + public Object build() { + escape(f); + return new Object(); + } + } + + // Builder that escapes via field `f` that is assigned in a virtual method. + static class Builder2 { + + public Builder2 f; + + @NeverInline + public Builder2 init() { + f = this; + return this; + } + + public Object build() { + escape(f); + return new Object(); + } + } + + // Builder that escapes via field `f` that is assigned in a virtual method. + static class Builder3 { + + public Builder3 f; + + @NeverInline + public Builder3 init(Builder3 builder) { + f = builder; + return this; + } + + public Object build() { + escape(f); + return new Object(); + } + } + + // Builder that escapes via field `f` that is assigned in a virtual method. + static class Builder4 { + + public Builder4 f; + + @NeverInline + public Builder4 init(Builder4 builder) { + builder.f = builder; + return this; + } + + public Object build() { + escape(f); + return new Object(); + } + } + + // Builder that escapes via field `f` that is assigned in a static method. + static class Builder5 { + + public Builder5 f; + + @NeverInline + public static Builder5 init(Builder5 builder) { + builder.f = builder; + return builder; + } + + public Object build() { + escape(f); + return new Object(); + } + } +}
diff --git a/src/test/java/com/android/tools/r8/naming/AdaptResourceFileContentsTest.java b/src/test/java/com/android/tools/r8/naming/AdaptResourceFileContentsTest.java index b03c257..cc5b7b7 100644 --- a/src/test/java/com/android/tools/r8/naming/AdaptResourceFileContentsTest.java +++ b/src/test/java/com/android/tools/r8/naming/AdaptResourceFileContentsTest.java
@@ -169,7 +169,8 @@ getProguardConfig(enableAdaptResourceFileContents, adaptResourceFileContentsPathFilter), "-neverinline class com.android.tools.r8.naming.AdaptResourceFileContentsTestClass$B {", " public void method();", - "}"); + "}", + "-neverclassinline class com.android.tools.r8.naming.AdaptResourceFileContentsTestClass$B"); } @Test @@ -325,13 +326,7 @@ .addDataResources(getDataResources()) .enableProguardTestOptions() .addKeepRules(proguardConfig) - .addOptionsModification( - o -> { - // TODO(christofferqa): Class inliner should respect -neverinline. - o.enableClassInlining = false; - o.enableVerticalClassMerging = true; - o.dataResourceConsumer = dataResourceConsumer; - }) + .addOptionsModification(o -> o.dataResourceConsumer = dataResourceConsumer) .compile(); }
diff --git a/src/test/java/com/android/tools/r8/naming/AdaptResourceFileNamesTest.java b/src/test/java/com/android/tools/r8/naming/AdaptResourceFileNamesTest.java index 8876860..7d4c47a 100644 --- a/src/test/java/com/android/tools/r8/naming/AdaptResourceFileNamesTest.java +++ b/src/test/java/com/android/tools/r8/naming/AdaptResourceFileNamesTest.java
@@ -27,6 +27,7 @@ import com.android.tools.r8.utils.ArchiveResourceProvider; import com.android.tools.r8.utils.FileUtils; import com.android.tools.r8.utils.KeepingDiagnosticHandler; +import com.android.tools.r8.utils.StringUtils; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import java.io.File; @@ -95,15 +96,15 @@ private static String getProguardConfigWithNeverInline( boolean enableAdaptResourceFileNames, String adaptResourceFileNamesPathFilter) { - return String.join( - System.lineSeparator(), + return StringUtils.lines( getProguardConfig(enableAdaptResourceFileNames, adaptResourceFileNamesPathFilter), "-neverinline class " + adaptresourcefilenames.A.class.getName() + " {", " public void method();", "}", "-neverinline class " + adaptresourcefilenames.B.Inner.class.getName() + " {", " public void method();", - "}"); + "}", + "-neverclassinline class *"); } @Test @@ -264,9 +265,6 @@ return ToolHelper.runR8( command, options -> { - // TODO(christofferqa): Class inliner should respect -neverinline. - options.enableClassInlining = false; - options.enableVerticalClassMerging = true; options.dataResourceConsumer = dataResourceConsumer; options.proguardMapConsumer = proguardMapConsumer; options.testing.suppressExperimentalCfBackendWarning = true;