Merge "Warn about and remove invalid field and method signatures"
diff --git a/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java b/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java
index 8771b72..819c6f2 100644
--- a/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java
+++ b/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java
@@ -6,7 +6,6 @@
import static com.android.tools.r8.ir.code.IRCode.INSTRUCTION_NUMBER_DELTA;
import com.android.tools.r8.errors.CompilationError;
-import com.android.tools.r8.errors.Unreachable;
import com.android.tools.r8.graph.DebugLocalInfo;
import com.android.tools.r8.graph.DebugLocalInfo.PrintLevel;
import com.android.tools.r8.graph.DexItemFactory;
@@ -468,7 +467,7 @@
return instruction;
}
}
- throw new Unreachable();
+ return null;
}
public void clearUserInfo() {
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 a52637f..7d7e5f3 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
@@ -46,6 +46,7 @@
import com.android.tools.r8.ir.optimize.NonNullTracker;
import com.android.tools.r8.ir.optimize.Outliner;
import com.android.tools.r8.ir.optimize.PeepholeOptimizer;
+import com.android.tools.r8.ir.optimize.RedundantFieldLoadElimination;
import com.android.tools.r8.ir.optimize.classinliner.ClassInliner;
import com.android.tools.r8.ir.optimize.lambda.LambdaMerger;
import com.android.tools.r8.ir.regalloc.LinearScanRegisterAllocator;
@@ -140,7 +141,7 @@
this.memberValuePropagation =
options.enableValuePropagation ?
new MemberValuePropagation(appInfo.withLiveness()) : null;
- this.lensCodeRewriter = new LensCodeRewriter(graphLense, appInfo.withSubtyping());
+ this.lensCodeRewriter = new LensCodeRewriter(graphLense, appInfo.withSubtyping(), options);
if (appInfo.hasLiveness()) {
// When disabling the pruner here, also disable the ProtoLiteExtension in R8.java.
this.protoLiteRewriter =
@@ -686,6 +687,7 @@
codeRewriter.rewriteSwitch(code);
codeRewriter.processMethodsNeverReturningNormally(code);
codeRewriter.simplifyIf(code, typeEnvironment);
+ new RedundantFieldLoadElimination(code).run();
if (options.testing.invertConditionals) {
invertConditionalsForTesting(code);
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/LensCodeRewriter.java b/src/main/java/com/android/tools/r8/ir/conversion/LensCodeRewriter.java
index 1926b14..0e4be8b 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/LensCodeRewriter.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/LensCodeRewriter.java
@@ -34,6 +34,7 @@
import com.android.tools.r8.ir.code.StaticGet;
import com.android.tools.r8.ir.code.StaticPut;
import com.android.tools.r8.ir.code.Value;
+import com.android.tools.r8.utils.InternalOptions;
import java.util.List;
import java.util.ListIterator;
import java.util.stream.Collectors;
@@ -42,10 +43,13 @@
private final GraphLense graphLense;
private final AppInfoWithSubtyping appInfo;
+ private final InternalOptions options;
- public LensCodeRewriter(GraphLense graphLense, AppInfoWithSubtyping appInfo) {
+ public LensCodeRewriter(
+ GraphLense graphLense, AppInfoWithSubtyping appInfo, InternalOptions options) {
this.graphLense = graphLense;
this.appInfo = appInfo;
+ this.options = options;
}
private Value makeOutValue(Instruction insn, IRCode code) {
@@ -97,7 +101,7 @@
continue;
}
DexMethod actualTarget = graphLense.lookupMethod(invokedMethod, method, invoke.getType());
- Invoke.Type invokeType = getInvokeType(invoke, actualTarget, invokedMethod);
+ Invoke.Type invokeType = getInvokeType(invoke, actualTarget, invokedMethod, method);
if (actualTarget != invokedMethod || invoke.getType() != invokeType) {
Invoke newInvoke = Invoke.create(invokeType, actualTarget, null,
invoke.outValue(), invoke.inValues());
@@ -242,7 +246,8 @@
private Type getInvokeType(
InvokeMethod invoke,
DexMethod actualTarget,
- DexMethod originalTarget) {
+ DexMethod originalTarget,
+ DexEncodedMethod invocationContext) {
// We might move methods from interfaces to classes and vice versa. So we have to support
// fixing the invoke kind, yet only if it was correct to start with.
if (invoke.isInvokeVirtual() || invoke.isInvokeInterface()) {
@@ -250,23 +255,42 @@
DexClass newTargetClass = appInfo.definitionFor(actualTarget.holder);
if (newTargetClass == null) {
return invoke.getType();
- } else {
- DexClass originalTargetClass = appInfo.definitionFor(originalTarget.holder);
- if (originalTargetClass != null
- && (originalTargetClass.isInterface() ^ (invoke.getType() == Type.INTERFACE))) {
- // The invoke was wrong to start with, so we keep it wrong. This is to ensure we get
- // the IncompatibleClassChangeError the original invoke would have triggered.
- return newTargetClass.accessFlags.isInterface()
- ? Type.VIRTUAL
- : Type.INTERFACE;
- } else {
- return newTargetClass.accessFlags.isInterface()
- ? Type.INTERFACE
- : Type.VIRTUAL;
+ }
+ DexClass originalTargetClass = appInfo.definitionFor(originalTarget.holder);
+ if (originalTargetClass != null
+ && (originalTargetClass.isInterface() ^ (invoke.getType() == Type.INTERFACE))) {
+ // The invoke was wrong to start with, so we keep it wrong. This is to ensure we get
+ // the IncompatibleClassChangeError the original invoke would have triggered.
+ return newTargetClass.accessFlags.isInterface() ? Type.VIRTUAL : Type.INTERFACE;
+ }
+ return newTargetClass.accessFlags.isInterface() ? Type.INTERFACE : Type.VIRTUAL;
+ }
+ if (options.enableClassMerging && invoke.isInvokeSuper()) {
+ if (actualTarget.getHolder() == invocationContext.method.getHolder()) {
+ DexClass targetClass = appInfo.definitionFor(actualTarget.holder);
+ if (targetClass == null) {
+ return invoke.getType();
+ }
+
+ // If the super class A of the enclosing class B (i.e., invocationContext.method.holder)
+ // has been merged into B during vertical class merging, and this invoke-super instruction
+ // was resolving to a method in A, then the target method has been changed to a direct
+ // method and moved into B, so that we need to use an invoke-direct instruction instead of
+ // invoke-super.
+ //
+ // At this point, we have an invoke-super instruction where the static target is the
+ // enclosing class. However, such an instruction could occur even if a subclass has never
+ // been merged into the enclosing class. Therefore, to determine if vertical class merging
+ // has been applied, we look if there is a direct method with the right signature, and only
+ // return Type.DIRECT in that case.
+ DexEncodedMethod method = targetClass.lookupDirectMethod(actualTarget);
+ if (method != null) {
+ // The target method has been moved from the super class into the sub class during class
+ // merging such that we now need to use an invoke-direct instruction.
+ return Type.DIRECT;
}
}
- } else {
- return invoke.getType();
}
+ return invoke.getType();
}
}
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/Inliner.java b/src/main/java/com/android/tools/r8/ir/optimize/Inliner.java
index 25cd969..029b169 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/Inliner.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/Inliner.java
@@ -271,7 +271,7 @@
Origin origin = appInfo.originFor(target.method.holder);
IRCode code = target.buildInliningIR(appInfo, options, generator, callerPosition, origin);
if (!target.isProcessed()) {
- new LensCodeRewriter(graphLense, appInfo).rewrite(code, target);
+ new LensCodeRewriter(graphLense, appInfo, options).rewrite(code, target);
}
return code;
}
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/RedundantFieldLoadElimination.java b/src/main/java/com/android/tools/r8/ir/optimize/RedundantFieldLoadElimination.java
new file mode 100644
index 0000000..24012b1
--- /dev/null
+++ b/src/main/java/com/android/tools/r8/ir/optimize/RedundantFieldLoadElimination.java
@@ -0,0 +1,193 @@
+// 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;
+
+import com.android.tools.r8.graph.DexField;
+import com.android.tools.r8.ir.code.BasicBlock;
+import com.android.tools.r8.ir.code.DominatorTree;
+import com.android.tools.r8.ir.code.FieldInstruction;
+import com.android.tools.r8.ir.code.IRCode;
+import com.android.tools.r8.ir.code.Instruction;
+import com.android.tools.r8.ir.code.InstructionListIterator;
+import com.android.tools.r8.ir.code.Phi;
+import com.android.tools.r8.ir.code.Value;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+
+/**
+ * Eliminate redundant field loads.
+ *
+ * <p>Simple algorithm that goes through all blocks in one pass in dominator order and propagates
+ * active field sets across control-flow edges where the target has only one predecessor.
+ */
+// TODO(ager): Evaluate speed/size for computing active field sets in a fixed-point computation.
+public class RedundantFieldLoadElimination {
+
+ private final IRCode code;
+ private final DominatorTree dominatorTree;
+
+ // Maps keeping track of fields that have an already loaded value at basic block entry.
+ private final HashMap<BasicBlock, HashMap<FieldAndObject, Instruction>>
+ activeInstanceFieldsAtEntry = new HashMap<>();
+ private final HashMap<BasicBlock, HashMap<DexField, Instruction>> activeStaticFieldsAtEntry =
+ new HashMap<>();
+
+ // Maps keeping track of fields with already loaded values for the current block during
+ // elimination.
+ private HashMap<FieldAndObject, Instruction> activeInstanceFields;
+ private HashMap<DexField, Instruction> activeStaticFields;
+
+ public RedundantFieldLoadElimination(IRCode code) {
+ this.code = code;
+ dominatorTree = new DominatorTree(code);
+ }
+
+ private static class FieldAndObject {
+ private final DexField field;
+ private final Value object;
+
+ private FieldAndObject(DexField field, Value receiver) {
+ this.field = field;
+ this.object = receiver;
+ }
+
+ @Override
+ public int hashCode() {
+ return field.hashCode() * 7 + object.hashCode();
+ }
+
+ @Override
+ public boolean equals(Object other) {
+ if (!(other instanceof FieldAndObject)) {
+ return false;
+ }
+ FieldAndObject o = (FieldAndObject) other;
+ return o.object == object && o.field == field;
+ }
+ }
+
+ public void run() {
+ for (BasicBlock block : dominatorTree.getSortedBlocks()) {
+ activeInstanceFields =
+ activeInstanceFieldsAtEntry.containsKey(block)
+ ? activeInstanceFieldsAtEntry.get(block)
+ : new HashMap<>();
+ activeStaticFields =
+ activeStaticFieldsAtEntry.containsKey(block)
+ ? activeStaticFieldsAtEntry.get(block)
+ : new HashMap<>();
+ InstructionListIterator it = block.listIterator();
+ while (it.hasNext()) {
+ Instruction instruction = it.next();
+ if (instruction.isFieldInstruction()) {
+ DexField field = instruction.asFieldInstruction().getField();
+ if (instruction.isInstancePut() || instruction.isStaticPut()) {
+ killActiveFields(instruction.asFieldInstruction());
+ } else if (instruction.isInstanceGet() && !instruction.outValue().hasLocalInfo()) {
+ Value object = instruction.asInstanceGet().object();
+ FieldAndObject fieldAndObject = new FieldAndObject(field, object);
+ if (activeInstanceFields.containsKey(fieldAndObject)) {
+ Instruction active = activeInstanceFields.get(fieldAndObject);
+ eliminateRedundantRead(it, instruction, active);
+ } else {
+ activeInstanceFields.put(fieldAndObject, instruction);
+ }
+ } else if (instruction.isStaticGet() && !instruction.outValue().hasLocalInfo()) {
+ if (activeStaticFields.containsKey(field)) {
+ Instruction active = activeStaticFields.get(field);
+ eliminateRedundantRead(it, instruction, active);
+ } else {
+ // A field get on a different class can cause <clinit> to run and change static
+ // field values.
+ killActiveFields(instruction.asFieldInstruction());
+ activeStaticFields.put(field, instruction);
+ }
+ }
+ }
+ if (instruction.isMonitor() || instruction.isInvokeMethod()) {
+ activeInstanceFields.clear();
+ activeStaticFields.clear();
+ }
+ }
+ propagateActiveFieldsFrom(block);
+ }
+ assert code.isConsistentSSA();
+ }
+
+ private void propagateActiveFieldsFrom(BasicBlock block) {
+ for (BasicBlock successor : block.getSuccessors()) {
+ // Allow propagation across exceptional edges, just be careful not to propagate if the
+ // throwing instruction is a field instruction.
+ if (successor.getPredecessors().size() == 1) {
+ if (block.hasCatchSuccessor(successor)) {
+ Instruction exceptionalExit = block.exceptionalExit();
+ if (exceptionalExit != null && exceptionalExit.isFieldInstruction()) {
+ killActiveFieldsForExceptionalExit(exceptionalExit.asFieldInstruction());
+ }
+ }
+ assert !activeInstanceFieldsAtEntry.containsKey(successor);
+ activeInstanceFieldsAtEntry.put(successor, new HashMap<>(activeInstanceFields));
+ assert !activeStaticFieldsAtEntry.containsKey(successor);
+ activeStaticFieldsAtEntry.put(successor, new HashMap<>(activeStaticFields));
+ }
+ }
+ }
+
+ private void killActiveFields(FieldInstruction instruction) {
+ DexField field = instruction.getField();
+ if (instruction.isInstancePut()) {
+ // Remove all the field/object pairs that refer to this field to make sure
+ // that we are conservative.
+ List<FieldAndObject> keysToRemove = new ArrayList<>();
+ for (FieldAndObject key : activeInstanceFields.keySet()) {
+ if (key.field == field) {
+ keysToRemove.add(key);
+ }
+ }
+ keysToRemove.forEach((k) -> activeInstanceFields.remove(k));
+ } else if (instruction.isInstanceGet()) {
+ Value object = instruction.asInstanceGet().object();
+ FieldAndObject fieldAndObject = new FieldAndObject(field, object);
+ activeInstanceFields.remove(fieldAndObject);
+ } else if (instruction.isStaticPut()) {
+ if (field.clazz != code.method.method.holder) {
+ // Accessing a static field on a different object could cause <clinit> to run which
+ // could modify any static field on any other object.
+ activeStaticFields.clear();
+ } else {
+ activeStaticFields.remove(field);
+ }
+ } else if (instruction.isStaticGet()) {
+ if (field.clazz != code.method.method.holder) {
+ // Accessing a static field on a different object could cause <clinit> to run which
+ // could modify any static field on any other object.
+ activeStaticFields.clear();
+ }
+ }
+ }
+
+ // If a field get instruction throws an exception it did not have an effect on the
+ // value of the field. Therefore, when propagating across exceptional edges for a
+ // field get instruction we have to exclude that field from the set of known
+ // field values.
+ private void killActiveFieldsForExceptionalExit(FieldInstruction instruction) {
+ DexField field = instruction.getField();
+ if (instruction.isInstanceGet()) {
+ Value object = instruction.asInstanceGet().object();
+ FieldAndObject fieldAndObject = new FieldAndObject(field, object);
+ activeInstanceFields.remove(fieldAndObject);
+ } else if (instruction.isStaticGet()) {
+ activeStaticFields.remove(field);
+ }
+ }
+
+ private void eliminateRedundantRead(
+ InstructionListIterator it, Instruction redundant, Instruction active) {
+ redundant.outValue().replaceUsers(active.outValue());
+ it.removeOrReplaceByDebugLocalRead();
+ active.outValue().uniquePhiUsers().forEach(Phi::removeTrivialPhi);
+ }
+}
diff --git a/src/main/java/com/android/tools/r8/ir/synthetic/ForwardMethodSourceCode.java b/src/main/java/com/android/tools/r8/ir/synthetic/ForwardMethodSourceCode.java
index 0ba49d3..6ea7d60 100644
--- a/src/main/java/com/android/tools/r8/ir/synthetic/ForwardMethodSourceCode.java
+++ b/src/main/java/com/android/tools/r8/ir/synthetic/ForwardMethodSourceCode.java
@@ -45,6 +45,7 @@
assert checkSignatures();
switch (invokeType) {
+ case DIRECT:
case STATIC:
case SUPER:
case INTERFACE:
diff --git a/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java b/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
index d196dd2..e46f94e 100644
--- a/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
+++ b/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
@@ -4,6 +4,7 @@
package com.android.tools.r8.shaking;
import com.android.tools.r8.errors.CompilationError;
+import com.android.tools.r8.graph.DexAnnotationSet;
import com.android.tools.r8.graph.DexApplication;
import com.android.tools.r8.graph.DexClass;
import com.android.tools.r8.graph.DexEncodedField;
@@ -18,7 +19,12 @@
import com.android.tools.r8.graph.GraphLense;
import com.android.tools.r8.graph.GraphLense.Builder;
import com.android.tools.r8.graph.KeyedDexItem;
+import com.android.tools.r8.graph.MethodAccessFlags;
+import com.android.tools.r8.graph.ParameterAnnotationsList;
import com.android.tools.r8.graph.PresortedComparable;
+import com.android.tools.r8.ir.code.Invoke.Type;
+import com.android.tools.r8.ir.synthetic.ForwardMethodSourceCode;
+import com.android.tools.r8.ir.synthetic.SynthesizedCode;
import com.android.tools.r8.logging.Log;
import com.android.tools.r8.optimize.InvokeSingleTargetExtractor;
import com.android.tools.r8.shaking.Enqueuer.AppInfoWithLiveness;
@@ -33,6 +39,7 @@
import it.unimi.dsi.fastutil.objects.Reference2IntMap;
import it.unimi.dsi.fastutil.objects.Reference2IntOpenHashMap;
import java.util.ArrayDeque;
+import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.Deque;
@@ -41,6 +48,7 @@
import java.util.IdentityHashMap;
import java.util.Iterator;
import java.util.LinkedHashSet;
+import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.function.BiFunction;
@@ -74,12 +82,20 @@
this.timing = timing;
}
- private boolean isMergeCandidate(DexProgramClass clazz) {
+ // Returns a set of types that must not be merged into other types.
+ private Set<DexType> getPinnedTypes(Iterable<DexProgramClass> classes) {
+ // TODO(christofferqa): Compute types from [classes] that must be pinned in order for vertical
+ // class merging to work.
+ return new HashSet<>();
+ }
+
+ private boolean isMergeCandidate(DexProgramClass clazz, Set<DexType> pinnedTypes) {
// We can merge program classes if they are not instantiated, have a single subtype
// and we do not have to keep them.
return !clazz.isLibraryClass()
&& !appInfo.instantiatedTypes.contains(clazz.type)
&& !appInfo.isPinned(clazz.type)
+ && !pinnedTypes.contains(clazz.type)
&& clazz.type.getSingleSubtype() != null;
}
@@ -170,6 +186,9 @@
int numberOfMerges = 0;
+ // Types that are pinned (in addition to those where appInfo.isPinned returns true).
+ Set<DexType> pinnedTypes = getPinnedTypes(classes);
+
// The resulting graph lense that should be used after class merging.
VerticalClassMergerGraphLense.Builder renamedMembersLense =
new VerticalClassMergerGraphLense.Builder();
@@ -188,7 +207,7 @@
}
DexProgramClass clazz = worklist.removeFirst();
- if (!seenBefore.add(clazz) || !isMergeCandidate(clazz)) {
+ if (!seenBefore.add(clazz) || !isMergeCandidate(clazz, pinnedTypes)) {
continue;
}
@@ -227,6 +246,9 @@
if (merged) {
// Commit the changes to the graph lense.
renamedMembersLense.merge(merger.getRenamings());
+ // Do not allow merging the resulting class into its subclass.
+ // TODO(christofferqa): Get rid of this limitation.
+ pinnedTypes.add(targetClass.type);
}
if (Log.ENABLED) {
if (merged) {
@@ -278,25 +300,81 @@
Set<Wrapper<DexMethod>> existingMethods = new HashSet<>();
addAll(existingMethods, target.directMethods(), MethodSignatureEquivalence.get());
addAll(existingMethods, target.virtualMethods(), MethodSignatureEquivalence.get());
- Collection<DexEncodedMethod> mergedDirectMethods = mergeItems(
- Iterators.transform(Iterators.forArray(source.directMethods()), this::renameConstructors),
- target.directMethods(),
- MethodSignatureEquivalence.get(),
- existingMethods,
- this::renameMethod
- );
- Iterator<DexEncodedMethod> methods = Iterators.forArray(source.virtualMethods());
- if (source.accessFlags.isInterface()) {
- // If merging an interface, only merge methods that are not otherwise defined in the
- // target class.
- methods = Iterators.transform(methods, this::filterShadowedInterfaceMethods);
+
+ List<DexEncodedMethod> directMethods = new ArrayList<>();
+ for (DexEncodedMethod directMethod : source.directMethods()) {
+ if (directMethod.isInstanceInitializer()) {
+ directMethods.add(renameConstructor(directMethod));
+ } else {
+ directMethods.add(directMethod);
+ }
}
- Collection<DexEncodedMethod> mergedVirtualMethods = mergeItems(
- methods,
- target.virtualMethods(),
- MethodSignatureEquivalence.get(),
- existingMethods,
- this::abortOnNonAbstract);
+
+ List<DexEncodedMethod> virtualMethods = new ArrayList<>();
+ for (DexEncodedMethod virtualMethod : source.virtualMethods()) {
+ DexEncodedMethod shadowedBy = findMethodInTarget(virtualMethod);
+ if (shadowedBy != null) {
+ if (virtualMethod.accessFlags.isAbstract()) {
+ // Remove abstract/interface methods that are shadowed.
+ deferredRenamings.map(virtualMethod.method, shadowedBy.method);
+ continue;
+ }
+ } else {
+ // The method is shadowed. If it is abstract, we can simply move it to the subclass.
+ // Non-abstract methods are handled below (they cannot simply be moved to the subclass as
+ // a virtual method, because they might be the target of an invoke-super instruction).
+ if (virtualMethod.accessFlags.isAbstract()) {
+ DexEncodedMethod resultingVirtualMethod =
+ renameMethod(virtualMethod, target.type, false);
+ deferredRenamings.map(virtualMethod.method, resultingVirtualMethod.method);
+ virtualMethods.add(resultingVirtualMethod);
+ continue;
+ }
+ }
+
+ // This virtual method could be called directly from a sub class via an invoke-super
+ // instruction. Therefore, we translate this virtual method into a direct method, such that
+ // relevant invoke-super instructions can be rewritten into invoke-direct instructions.
+ DexEncodedMethod resultingDirectMethod = renameMethod(virtualMethod, target.type, true);
+ makePrivate(resultingDirectMethod);
+ directMethods.add(resultingDirectMethod);
+
+ // Record that invoke-super instructions in the target class should be redirected to the
+ // newly created direct method.
+ deferredRenamings.mapVirtualMethodToDirectInType(
+ virtualMethod.method, resultingDirectMethod.method, target.type);
+
+ if (shadowedBy == null) {
+ // In addition to the newly added direct method, create a virtual method such that we do
+ // not accidentally remove the method from the interface of this class.
+ // Note that this method is added independently of whether it will actually be used. If
+ // it turns out that the method is never used, it will be removed by the final round
+ // of tree shaking.
+ shadowedBy = buildBridgeMethod(virtualMethod, resultingDirectMethod.method);
+ virtualMethods.add(shadowedBy);
+ }
+
+ deferredRenamings.map(virtualMethod.method, shadowedBy.method);
+ }
+
+ Collection<DexEncodedMethod> mergedDirectMethods =
+ mergeItems(
+ directMethods.iterator(),
+ target.directMethods(),
+ MethodSignatureEquivalence.get(),
+ existingMethods,
+ (existing, method) -> {
+ DexEncodedMethod renamedMethod = renameMethod(method, target.type, true);
+ deferredRenamings.map(method.method, renamedMethod.method);
+ return renamedMethod;
+ });
+ Collection<DexEncodedMethod> mergedVirtualMethods =
+ mergeItems(
+ virtualMethods.iterator(),
+ target.virtualMethods(),
+ MethodSignatureEquivalence.get(),
+ existingMethods,
+ this::abortOnNonAbstract);
if (abortMerge) {
return false;
}
@@ -354,17 +432,39 @@
return deferredRenamings;
}
- private DexEncodedMethod filterShadowedInterfaceMethods(DexEncodedMethod m) {
- DexEncodedMethod actual = appInfo.resolveMethod(target.type, m.method).asSingleTarget();
- assert actual != null;
- if (actual != m) {
- // We will drop a method here, so record it as a potential renaming.
- deferredRenamings.map(m.method, actual.method);
+ private DexEncodedMethod buildBridgeMethod(
+ DexEncodedMethod signature, DexMethod invocationTarget) {
+ DexType holder = target.type;
+ DexProto proto = invocationTarget.proto;
+ DexString name = signature.method.name;
+ MethodAccessFlags accessFlags = signature.accessFlags.copy();
+ accessFlags.setBridge();
+ accessFlags.setSynthetic();
+ accessFlags.unsetAbstract();
+ return new DexEncodedMethod(
+ application.dexItemFactory.createMethod(holder, proto, name),
+ accessFlags,
+ DexAnnotationSet.empty(),
+ ParameterAnnotationsList.empty(),
+ new SynthesizedCode(
+ new ForwardMethodSourceCode(holder, proto, holder, invocationTarget, Type.DIRECT),
+ registry -> registry.registerInvokeDirect(invocationTarget)));
+ }
+
+ // Returns the method that shadows the given method, or null if method is not shadowed.
+ private DexEncodedMethod findMethodInTarget(DexEncodedMethod method) {
+ DexEncodedMethod actual = appInfo.resolveMethod(target.type, method.method).asSingleTarget();
+ if (actual == null) {
+ // May happen in case of missing classes.
+ abortMerge = true;
return null;
}
- // We will keep the method, so the class better be abstract.
- assert target.accessFlags.isAbstract();
- return m;
+ if (actual != method) {
+ return actual;
+ }
+ // We will keep the method, so the class better be abstract if there is no implementation.
+ assert !method.accessFlags.isAbstract() || target.accessFlags.isAbstract();
+ return null;
}
private <T extends KeyedDexItem<S>, S extends PresortedComparable<S>> void addAll(
@@ -420,80 +520,84 @@
}
}
- private DexString makeMergedName(String nameString, DexType holder) {
- return application.dexItemFactory
- .createString(nameString + "$" + holder.toSourceString().replace('.', '$'));
+ // Note that names returned by this function are not necessarily unique. However, class merging
+ // is aborted if it turns out that the returned name is not unique.
+ // TODO(christofferqa): Write a test that checks that class merging is aborted if this function
+ // generates a name that is not unique.
+ private DexString getFreshName(String nameString, DexType holder) {
+ String freshName = nameString + "$" + holder.toSourceString().replace('.', '$');
+ return application.dexItemFactory.createString(freshName);
}
private DexEncodedMethod abortOnNonAbstract(DexEncodedMethod existing,
DexEncodedMethod method) {
- if (existing == null) {
- // This is a conflict between a static and virtual method. Abort.
- abortMerge = true;
- return method;
- }
- // Ignore if we merge in an abstract method or if we override a bridge method that would
- // bridge to the superclasses method.
- if (method.accessFlags.isAbstract()) {
- // We make a method disappear here, so record the renaming so that calls to the previous
- // target get forwarded properly.
- deferredRenamings.map(method.method, existing.method);
- return existing;
- } else if (existing.accessFlags.isBridge()) {
+ // Ignore if we override a bridge method that would bridge to the superclasses method.
+ if (existing != null && existing.accessFlags.isBridge()) {
InvokeSingleTargetExtractor extractor = new InvokeSingleTargetExtractor();
existing.getCode().registerCodeReferences(extractor);
- if (extractor.getTarget() != method.method) {
- abortMerge = true;
+ if (extractor.getTarget() == method.method) {
+ return method;
}
- return method;
- } else {
- abortMerge = true;
- return existing;
}
+
+ // Abstract shadowed methods are removed prior to calling mergeItems.
+ assert !method.accessFlags.isAbstract();
+
+ // If [existing] is null, there is a conflict between a static and virtual method. Otherwise,
+ // there is a non-abstract method that is shadowed. We translate virtual shadowed methods into
+ // direct methods with a fresh name, so this situation should never happen. We currently get
+ // in this situation if getFreshName returns a name that is not unique, though.
+ abortMerge = true;
+ return method;
}
- private DexEncodedMethod renameConstructors(DexEncodedMethod method) {
- // Only rename instance initializers.
- if (!method.isInstanceInitializer()) {
- return method;
- }
+ private DexEncodedMethod renameConstructor(DexEncodedMethod method) {
+ assert method.isInstanceInitializer();
DexType holder = method.method.holder;
- DexEncodedMethod result = method
- .toRenamedMethod(makeMergedName(CONSTRUCTOR_NAME, holder), application.dexItemFactory);
+ DexEncodedMethod result =
+ method.toRenamedMethod(
+ getFreshName(CONSTRUCTOR_NAME, holder), application.dexItemFactory);
result.markForceInline();
deferredRenamings.map(method.method, result.method);
// Renamed constructors turn into ordinary private functions. They can be private, as
// they are only references from their direct subclass, which they were merged into.
result.accessFlags.unsetConstructor();
- result.accessFlags.unsetPublic();
- result.accessFlags.unsetProtected();
- result.accessFlags.setPrivate();
+ makePrivate(result);
return result;
}
- private DexEncodedMethod renameMethod(DexEncodedMethod existing, DexEncodedMethod method) {
+ private DexEncodedMethod renameMethod(
+ DexEncodedMethod method, DexType newHolder, boolean useFreshName) {
// We cannot handle renaming static initializers yet and constructors should have been
// renamed already.
assert !method.accessFlags.isConstructor();
- DexType holder = method.method.holder;
- String name = method.method.name.toSourceString();
- DexEncodedMethod result = method
- .toRenamedMethod(makeMergedName(name, holder), application.dexItemFactory);
- deferredRenamings.map(method.method, result.method);
- return result;
+ DexType oldHolder = method.method.holder;
+ DexString oldName = method.method.name;
+ DexString newName =
+ useFreshName ? getFreshName(oldName.toSourceString(), oldHolder) : oldName;
+ DexMethod newSignature =
+ application.dexItemFactory.createMethod(newHolder, method.method.proto, newName);
+ return method.toTypeSubstitutedMethod(newSignature);
}
private DexEncodedField renameField(DexEncodedField existing, DexEncodedField field) {
DexString oldName = field.field.name;
DexType holder = field.field.clazz;
- DexEncodedField result = field
- .toRenamedField(makeMergedName(oldName.toSourceString(), holder),
- application.dexItemFactory);
+ DexEncodedField result =
+ field.toRenamedField(
+ getFreshName(oldName.toSourceString(), holder), application.dexItemFactory);
deferredRenamings.map(field.field, result.field);
return result;
}
}
+ private static void makePrivate(DexEncodedMethod method) {
+ assert !method.accessFlags.isAbstract();
+ method.accessFlags.unsetPublic();
+ method.accessFlags.unsetProtected();
+ method.accessFlags.setPrivate();
+ }
+
private class TreeFixer {
private final Builder lense = GraphLense.builder();
diff --git a/src/main/java/com/android/tools/r8/shaking/VerticalClassMergerGraphLense.java b/src/main/java/com/android/tools/r8/shaking/VerticalClassMergerGraphLense.java
index 54393b5..ea57d1d 100644
--- a/src/main/java/com/android/tools/r8/shaking/VerticalClassMergerGraphLense.java
+++ b/src/main/java/com/android/tools/r8/shaking/VerticalClassMergerGraphLense.java
@@ -12,6 +12,7 @@
import com.android.tools.r8.ir.code.Invoke.Type;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
+import java.util.HashMap;
import java.util.Map;
import java.util.Set;
@@ -44,14 +45,17 @@
private final Map<DexField, DexField> fieldMap;
private final Map<DexMethod, DexMethod> methodMap;
+ private final Map<DexType, Map<DexMethod, DexMethod>> contextualVirtualToDirectMethodMaps;
public VerticalClassMergerGraphLense(
Map<DexField, DexField> fieldMap,
Map<DexMethod, DexMethod> methodMap,
+ Map<DexType, Map<DexMethod, DexMethod>> contextualVirtualToDirectMethodMaps,
GraphLense previousLense) {
this.previousLense = previousLense;
this.fieldMap = fieldMap;
this.methodMap = methodMap;
+ this.contextualVirtualToDirectMethodMaps = contextualVirtualToDirectMethodMaps;
}
@Override
@@ -61,11 +65,18 @@
@Override
public DexMethod lookupMethod(DexMethod method, DexEncodedMethod context, Type type) {
- // TODO(christofferqa): If [type] is Type.SUPER and [method] has been merged into the class of
- // [context], then return the DIRECT method that has been created for [method] by SimpleClass-
- // Merger. Otherwise, return the VIRTUAL method corresponding to [method].
- assert isContextFreeForMethod(method) || context != null;
+ assert isContextFreeForMethod(method) || (context != null && type != null);
DexMethod previous = previousLense.lookupMethod(method, context, type);
+ if (type == Type.SUPER) {
+ Map<DexMethod, DexMethod> virtualToDirectMethodMap =
+ contextualVirtualToDirectMethodMaps.get(context.method.holder);
+ if (virtualToDirectMethodMap != null) {
+ DexMethod directMethod = virtualToDirectMethodMap.get(previous);
+ if (directMethod != null) {
+ return directMethod;
+ }
+ }
+ }
return methodMap.getOrDefault(previous, previous);
}
@@ -74,6 +85,13 @@
ImmutableSet.Builder<DexMethod> builder = ImmutableSet.builder();
for (DexMethod previous : previousLense.lookupMethodInAllContexts(method)) {
builder.add(methodMap.getOrDefault(previous, previous));
+ for (Map<DexMethod, DexMethod> virtualToDirectMethodMap :
+ contextualVirtualToDirectMethodMaps.values()) {
+ DexMethod directMethod = virtualToDirectMethodMap.get(previous);
+ if (directMethod != null) {
+ builder.add(directMethod);
+ }
+ }
}
return builder.build();
}
@@ -86,14 +104,22 @@
@Override
public boolean isContextFreeForMethods() {
- return previousLense.isContextFreeForMethods();
+ return contextualVirtualToDirectMethodMaps.isEmpty() && previousLense.isContextFreeForMethods();
}
@Override
public boolean isContextFreeForMethod(DexMethod method) {
- // TODO(christofferqa): Should return false for methods where this graph lense is context
- // sensitive.
- return previousLense.isContextFreeForMethod(method);
+ if (!previousLense.isContextFreeForMethod(method)) {
+ return false;
+ }
+ DexMethod previous = previousLense.lookupMethod(method);
+ for (Map<DexMethod, DexMethod> virtualToDirectMethodMap :
+ contextualVirtualToDirectMethodMaps.values()) {
+ if (virtualToDirectMethodMap.containsKey(previous)) {
+ return false;
+ }
+ }
+ return true;
}
public static class Builder {
@@ -101,16 +127,21 @@
private final ImmutableMap.Builder<DexField, DexField> fieldMapBuilder = ImmutableMap.builder();
private final ImmutableMap.Builder<DexMethod, DexMethod> methodMapBuilder =
ImmutableMap.builder();
+ private final Map<DexType, Map<DexMethod, DexMethod>> contextualVirtualToDirectMethodMaps =
+ new HashMap<>();
public Builder() {}
public GraphLense build(GraphLense previousLense) {
Map<DexField, DexField> fieldMap = fieldMapBuilder.build();
Map<DexMethod, DexMethod> methodMap = methodMapBuilder.build();
- if (fieldMap.isEmpty() && methodMap.isEmpty()) {
+ if (fieldMap.isEmpty()
+ && methodMap.isEmpty()
+ && contextualVirtualToDirectMethodMaps.isEmpty()) {
return previousLense;
}
- return new VerticalClassMergerGraphLense(fieldMap, methodMap, previousLense);
+ return new VerticalClassMergerGraphLense(
+ fieldMap, methodMap, contextualVirtualToDirectMethodMaps, previousLense);
}
public void map(DexField from, DexField to) {
@@ -121,9 +152,24 @@
methodMapBuilder.put(from, to);
}
+ public void mapVirtualMethodToDirectInType(DexMethod from, DexMethod to, DexType type) {
+ Map<DexMethod, DexMethod> virtualToDirectMethodMap =
+ contextualVirtualToDirectMethodMaps.computeIfAbsent(type, key -> new HashMap<>());
+ virtualToDirectMethodMap.put(from, to);
+ }
+
public void merge(VerticalClassMergerGraphLense.Builder builder) {
fieldMapBuilder.putAll(builder.fieldMapBuilder.build());
methodMapBuilder.putAll(builder.methodMapBuilder.build());
+ for (DexType context : builder.contextualVirtualToDirectMethodMaps.keySet()) {
+ Map<DexMethod, DexMethod> current = contextualVirtualToDirectMethodMaps.get(context);
+ Map<DexMethod, DexMethod> other = builder.contextualVirtualToDirectMethodMaps.get(context);
+ if (current != null) {
+ current.putAll(other);
+ } else {
+ contextualVirtualToDirectMethodMaps.put(context, other);
+ }
+ }
}
}
}
diff --git a/src/test/examples/uninitializedfinal/UninitializedFinalFieldLeak.java b/src/test/examples/uninitializedfinal/UninitializedFinalFieldLeak.java
new file mode 100644
index 0000000..781d2b4
--- /dev/null
+++ b/src/test/examples/uninitializedfinal/UninitializedFinalFieldLeak.java
@@ -0,0 +1,57 @@
+// 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 uninitializedfinal;
+
+// Test that leaks an instance before its final field has been initialized to a thread that
+// reads that field. This tests that redundant field load elimination does not eliminate
+// field reads (even of final fields) that cross a monitor operation.
+public class UninitializedFinalFieldLeak {
+
+ public static class PollingThread extends Thread {
+ public int result = 0;
+ UninitializedFinalFieldLeak f;
+
+ PollingThread(UninitializedFinalFieldLeak f) {
+ this.f = f;
+ }
+
+ // Read the field a number of times. Then lock on the object to await field initialization.
+ public void run() {
+ result += f.i;
+ result += f.i;
+ result += f.i;
+ f.threadReadsDone = true;
+ synchronized (f) {
+ result += f.i;
+ }
+ // The right result is 42. Reading the uninitialized 0 three times and then
+ // reading the initialized value. It is safe to remove the two redundant loads
+ // before the monitor operation.
+ System.out.println(result);
+ }
+ }
+
+ public final int i;
+ public volatile boolean threadReadsDone = false;
+
+ public UninitializedFinalFieldLeak() throws InterruptedException {
+ // Leak the object to a thread and start the thread with the lock on the object taken.
+ // Then allow the other thread to run and read the uninitialized field.
+ // Finally, initialize the field and release the lock.
+ PollingThread t = new PollingThread(this);
+ synchronized (this) {
+ t.start();
+ while (!threadReadsDone) {
+ Thread.yield();
+ }
+ i = 42;
+ }
+ t.join();
+ }
+
+ public static void main(String[] args) throws InterruptedException {
+ new UninitializedFinalFieldLeak();
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/R8RunExamplesTest.java b/src/test/java/com/android/tools/r8/R8RunExamplesTest.java
index 087012c..36d924f 100644
--- a/src/test/java/com/android/tools/r8/R8RunExamplesTest.java
+++ b/src/test/java/com/android/tools/r8/R8RunExamplesTest.java
@@ -45,9 +45,11 @@
"instancevariable.InstanceVariable",
"instanceofstring.InstanceofString",
"invoke.Invoke",
+ "invokeempty.InvokeEmpty",
"jumbostring.JumboString",
"loadconst.LoadConst",
"loop.UdpServer",
+ "nestedtrycatches.NestedTryCatches",
"newarray.NewArray",
"regalloc.RegAlloc",
"returns.Returns",
@@ -58,9 +60,7 @@
"throwing.Throwing",
"trivial.Trivial",
"trycatch.TryCatch",
- "nestedtrycatches.NestedTryCatches",
"trycatchmany.TryCatchMany",
- "invokeempty.InvokeEmpty",
"regress.Regress",
"regress2.Regress2",
"regress_37726195.Regress",
@@ -81,6 +81,7 @@
"enclosingmethod_proguarded.Main",
"interfaceinlining.Main",
"switchmaps.Switches",
+ "uninitializedfinal.UninitializedFinalFieldLeak",
};
List<String[]> fullTestList = new ArrayList<>(tests.length * 2);
diff --git a/src/test/java/com/android/tools/r8/classmerging/ClassMergingTest.java b/src/test/java/com/android/tools/r8/classmerging/ClassMergingTest.java
index 9e018e3..52e2313 100644
--- a/src/test/java/com/android/tools/r8/classmerging/ClassMergingTest.java
+++ b/src/test/java/com/android/tools/r8/classmerging/ClassMergingTest.java
@@ -17,6 +17,8 @@
import com.android.tools.r8.code.Instruction;
import com.android.tools.r8.code.MoveException;
import com.android.tools.r8.graph.DexCode;
+import com.android.tools.r8.origin.Origin;
+import com.android.tools.r8.smali.SmaliBuilder;
import com.android.tools.r8.utils.AndroidApp;
import com.android.tools.r8.utils.DexInspector;
import com.android.tools.r8.utils.DexInspector.ClassSubject;
@@ -106,7 +108,6 @@
assertTrue(inspector.clazz("classmerging.ConflictingInterfaceImpl").isPresent());
}
- @Ignore("b/73958515")
@Test
public void testSuperCallWasDetected() throws Exception {
String main = "classmerging.SuperCallRewritingTest";
@@ -123,6 +124,65 @@
runTest(main, programFiles, preservedClassNames);
}
+ // When a subclass A has been merged into its subclass B, we rewrite invoke-super calls that hit
+ // methods in A to invoke-direct calls. However, we should be careful not to transform invoke-
+ // super instructions into invoke-direct instructions simply because the static target is a method
+ // in the enclosing class.
+ //
+ // This test hand-crafts an invoke-super instruction in SubClassThatReferencesSuperMethod that
+ // targets SubClassThatReferencesSuperMethod.referencedMethod. When running without class
+ // merging, R8 should not rewrite the invoke-super instruction into invoke-direct.
+ @Test
+ public void testSuperCallNotRewrittenToDirect() throws Exception {
+ String main = "classmerging.SuperCallRewritingTest";
+ Path[] programFiles =
+ new Path[] {
+ CF_DIR.resolve("SuperClassWithReferencedMethod.class"),
+ CF_DIR.resolve("SuperCallRewritingTest.class")
+ };
+ Set<String> preservedClassNames =
+ ImmutableSet.of(
+ "classmerging.SubClassThatReferencesSuperMethod",
+ "classmerging.SuperClassWithReferencedMethod",
+ "classmerging.SuperCallRewritingTest");
+
+ // Build SubClassThatReferencesMethod.
+ SmaliBuilder smaliBuilder =
+ new SmaliBuilder(
+ "classmerging.SubClassThatReferencesSuperMethod",
+ "classmerging.SuperClassWithReferencedMethod");
+ smaliBuilder.addInitializer(
+ ImmutableList.of(),
+ 0,
+ "invoke-direct {p0}, Lclassmerging/SuperClassWithReferencedMethod;-><init>()V",
+ "return-void");
+ smaliBuilder.addInstanceMethod(
+ "java.lang.String",
+ "referencedMethod",
+ ImmutableList.of(),
+ 2,
+ "sget-object v0, Ljava/lang/System;->out:Ljava/io/PrintStream;",
+ "const-string v1, \"In referencedMethod on SubClassThatReferencesSuperMethod\"",
+ "invoke-virtual {v0, v1}, Ljava/io/PrintStream;->println(Ljava/lang/String;)V",
+ "invoke-super {p0}, Lclassmerging/SubClassThatReferencesSuperMethod;->referencedMethod()Ljava/lang/String;",
+ "move-result-object v1",
+ "return-object v1");
+
+ // Build app.
+ AndroidApp.Builder builder = AndroidApp.builder();
+ builder.addProgramFiles(programFiles);
+ builder.addDexProgramData(smaliBuilder.compile(), Origin.unknown());
+
+ // Run test.
+ runTestOnInput(
+ main,
+ builder.build(),
+ preservedClassNames,
+ // Prevent class merging, such that the generated code would be invalid if we rewrite the
+ // invoke-super instruction into an invoke-direct instruction.
+ getProguardConfig(EXAMPLE_KEEP, "-keep class *"));
+ }
+
@Test
public void testConflictingInterfaceSignatures() throws Exception {
String main = "classmerging.ConflictingInterfaceSignaturesTest";
@@ -229,13 +289,19 @@
private DexInspector runTest(String main, Path[] programFiles, Set<String> preservedClassNames)
throws Exception {
- return runTest(main, programFiles, preservedClassNames, getProguardConfig(EXAMPLE_KEEP, null));
+ return runTest(main, programFiles, preservedClassNames, getProguardConfig(EXAMPLE_KEEP));
}
private DexInspector runTest(
String main, Path[] programFiles, Set<String> preservedClassNames, String proguardConfig)
throws Exception {
- AndroidApp input = readProgramFiles(programFiles);
+ return runTestOnInput(
+ main, readProgramFiles(programFiles), preservedClassNames, proguardConfig);
+ }
+
+ private DexInspector runTestOnInput(
+ String main, AndroidApp input, Set<String> preservedClassNames, String proguardConfig)
+ throws Exception {
AndroidApp output = compileWithR8(input, proguardConfig, this::configure);
DexInspector inspector = new DexInspector(output);
// Check that all classes in [preservedClassNames] are in fact preserved.
@@ -254,14 +320,14 @@
return inspector;
}
- private String getProguardConfig(Path path, String additionalRules) throws IOException {
+ private String getProguardConfig(Path path, String... additionalRules) throws IOException {
StringBuilder builder = new StringBuilder();
for (String line : Files.readAllLines(path)) {
builder.append(line);
builder.append(System.lineSeparator());
}
- if (additionalRules != null) {
- builder.append(additionalRules);
+ for (String rule : additionalRules) {
+ builder.append(rule);
}
return builder.toString();
}
