Merge "Avoid processing empty bridge methods in VisibilityBridgeRemover."
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/naming/ClassNameMinifier.java b/src/main/java/com/android/tools/r8/naming/ClassNameMinifier.java
index e3ec746..01fff01 100644
--- a/src/main/java/com/android/tools/r8/naming/ClassNameMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/ClassNameMinifier.java
@@ -161,7 +161,7 @@
}
private void parseError(DexItem item, Origin origin, GenericSignatureFormatError e) {
- StringBuilder message = new StringBuilder("Invalid class signature for ");
+ StringBuilder message = new StringBuilder("Invalid signature for ");
if (item instanceof DexClass) {
message.append("class ");
message.append(((DexClass) item).getType().toSourceString());
@@ -183,12 +183,14 @@
clazz.annotations = rewriteGenericSignatures(clazz.annotations,
genericSignatureParser::parseClassSignature,
e -> parseError(clazz, clazz.getOrigin(), e));
- clazz.forEachField(field -> rewriteGenericSignatures(
- field.annotations, genericSignatureParser::parseFieldSignature,
- e -> parseError(field, clazz.getOrigin(), e)));
- clazz.forEachMethod(method -> rewriteGenericSignatures(
- method.annotations, genericSignatureParser::parseMethodSignature,
- e -> parseError(method, clazz.getOrigin(), e)));
+ clazz.forEachField(field ->
+ field.annotations = rewriteGenericSignatures(
+ field.annotations, genericSignatureParser::parseFieldSignature,
+ e -> parseError(field, clazz.getOrigin(), e)));
+ clazz.forEachMethod(method ->
+ method.annotations = rewriteGenericSignatures(
+ method.annotations, genericSignatureParser::parseMethodSignature,
+ e -> parseError(method, clazz.getOrigin(), e)));
}
}
@@ -506,11 +508,18 @@
String enclosingRenamedBinaryName =
getClassBinaryNameFromDescriptor(
renaming.getOrDefault(enclosingType, enclosingType.descriptor).toString());
- String renamed =
- getClassBinaryNameFromDescriptor(
- renaming.getOrDefault(type, type.descriptor).toString());
- String outName = renamed.substring(enclosingRenamedBinaryName.length() + 1);
- renamedSignature.append(outName);
+ DexString renamedDescriptor = renaming.get(type);
+ if (renamedDescriptor != null) {
+ // Pick the renamed inner class from the fully renamed binary name.
+ String fullRenamedBinaryName =
+ getClassBinaryNameFromDescriptor(renamedDescriptor.toString());
+ renamedSignature.append(
+ fullRenamedBinaryName.substring(enclosingRenamedBinaryName.length() + 1));
+ } else {
+ // Did not find the class - keep the inner class name as is.
+ // TODO(110085899): Warn about missing classes in signatures?
+ renamedSignature.append(name);
+ }
return type;
}
diff --git a/src/main/java/com/android/tools/r8/naming/signature/GenericSignatureParser.java b/src/main/java/com/android/tools/r8/naming/signature/GenericSignatureParser.java
index 5b549a3..c1f6a7d 100644
--- a/src/main/java/com/android/tools/r8/naming/signature/GenericSignatureParser.java
+++ b/src/main/java/com/android/tools/r8/naming/signature/GenericSignatureParser.java
@@ -84,7 +84,7 @@
throw e;
} catch (Throwable t) {
Error e = new GenericSignatureFormatError(
- "Unknown error parsing generic signature: " + t.getMessage());
+ "Unknown error parsing class signature: " + t.getMessage());
e.addSuppressed(t);
throw e;
}
@@ -100,7 +100,7 @@
throw e;
} catch (Throwable t) {
Error e = new GenericSignatureFormatError(
- "Unknown error parsing generic signature: " + t.getMessage());
+ "Unknown error parsing method signature: " + t.getMessage());
e.addSuppressed(t);
throw e;
}
@@ -112,14 +112,14 @@
setInput(signature);
parseFieldTypeSignature();
actions.stop();
- } catch (GenericSignatureFormatError e) {
- throw e;
- } catch (Throwable t) {
- Error e = new GenericSignatureFormatError(
- "Unknown error parsing generic signature: " + t.getMessage());
- e.addSuppressed(t);
- throw e;
- }
+ } catch (GenericSignatureFormatError e) {
+ throw e;
+ } catch (Throwable t) {
+ Error e = new GenericSignatureFormatError(
+ "Unknown error parsing field signature: " + t.getMessage());
+ e.addSuppressed(t);
+ throw e;
+ }
}
private void setInput(String input) {
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..d54c95c 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,85 @@
}
}
- 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);
+ DexType oldHolder = field.field.clazz;
+ DexString newName = getFreshName(oldName.toSourceString(), oldHolder);
+ DexField newSignature =
+ application.dexItemFactory.createField(target.type, field.field.type, newName);
+ DexEncodedField result = field.toTypeSubstitutedField(newSignature);
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();
}
diff --git a/src/test/java/com/android/tools/r8/naming/GenericSignatureParserTest.java b/src/test/java/com/android/tools/r8/naming/GenericSignatureParserTest.java
index 1d1604b..957bc1d 100644
--- a/src/test/java/com/android/tools/r8/naming/GenericSignatureParserTest.java
+++ b/src/test/java/com/android/tools/r8/naming/GenericSignatureParserTest.java
@@ -350,7 +350,8 @@
forMethodSignatures(this::parseMethodSignature);
}
- private void failingParseAction(Consumer<GenericSignatureParser<String>> parse)
+ private void failingParseAction(
+ Consumer<GenericSignatureParser<String>> parse, String errorMessageType)
throws Exception {
class ThrowsInParserActionBase<E extends Error> extends ReGenerateGenericSignatureRewriter {
protected Supplier<? extends E> exceptionSupplier;
@@ -452,7 +453,8 @@
assertEquals("ERROR", e.getMessage());
} else {
plainErrorCount++;
- assertEquals("Unknown error parsing generic signature: ERROR", e.getMessage());
+ assertEquals("Unknown error parsing "
+ + errorMessageType + " signature: ERROR", e.getMessage());
}
}
}
@@ -463,10 +465,10 @@
public void failingParseAction() throws Exception {
// These signatures hits all action callbacks.
failingParseAction(parser -> parser.parseClassSignature(
- "<U:Ljava/lang/Object;>LOuter<TT;>.Inner;Ljava/util/List<TU;>;"));
+ "<U:Ljava/lang/Object;>LOuter<TT;>.Inner;Ljava/util/List<TU;>;"), "class");
failingParseAction(
- parser -> parser.parseFieldSignature("LOuter$InnerInterface<TU;>.Inner;"));
+ parser -> parser.parseFieldSignature("LOuter$InnerInterface<TU;>.Inner;"), "field");
failingParseAction(
- parser -> parser.parseMethodSignature("(LOuter$InnerInterface<TU;>.Inner;)V"));
+ parser -> parser.parseMethodSignature("(LOuter$InnerInterface<TU;>.Inner;)V"), "method");
}
}
diff --git a/src/test/java/com/android/tools/r8/naming/MinifierClassSignatureTest.java b/src/test/java/com/android/tools/r8/naming/MinifierClassSignatureTest.java
index 68cd36f..b9ff7ea 100644
--- a/src/test/java/com/android/tools/r8/naming/MinifierClassSignatureTest.java
+++ b/src/test/java/com/android/tools/r8/naming/MinifierClassSignatureTest.java
@@ -424,7 +424,7 @@
@Test
public void classSignatureOuter_classNotFound() throws Exception {
- String signature = "<T:LNotFound;>LNotFound;";
+ String signature = "<T:LNotFound;>LAlsoNotFound;";
testSingleClass("Outer", signature, this::noWarnings, inspector -> {
assertThat(inspector.clazz("NotFound"), not(isPresent()));
ClassSubject outer = inspector.clazz("Outer");
@@ -433,11 +433,51 @@
}
@Test
+ public void classSignatureExtendsInner_innerClassNotFound() throws Exception {
+ String signature = "LOuter<TT;>.NotFound;";
+ testSingleClass("Outer$ExtendsInner", signature, this::noWarnings, inspector -> {
+ assertThat(inspector.clazz("NotFound"), not(isPresent()));
+ ClassSubject outer = inspector.clazz("Outer$ExtendsInner");
+ assertEquals(signature, outer.getOriginalSignatureAttribute());
+ });
+ }
+
+ @Test
+ public void classSignatureExtendsInner_outerAndInnerClassNotFound() throws Exception {
+ String signature = "LNotFound<TT;>.AlsoNotFound;";
+ testSingleClass("Outer$ExtendsInner", signature, this::noWarnings, inspector -> {
+ assertThat(inspector.clazz("NotFound"), not(isPresent()));
+ ClassSubject outer = inspector.clazz("Outer$ExtendsInner");
+ assertEquals(signature, outer.getOriginalSignatureAttribute());
+ });
+ }
+
+ @Test
+ public void classSignatureExtendsInner_nestedInnerClassNotFound() throws Exception {
+ String signature = "LOuter<TT;>.Inner.NotFound;";
+ testSingleClass("Outer$ExtendsInner", signature, this::noWarnings, inspector -> {
+ assertThat(inspector.clazz("NotFound"), not(isPresent()));
+ ClassSubject outer = inspector.clazz("Outer$ExtendsInner");
+ assertEquals(signature, outer.getOriginalSignatureAttribute());
+ });
+ }
+
+ @Test
+ public void classSignatureExtendsInner_multipleMestedInnerClassesNotFound() throws Exception {
+ String signature = "LOuter<TT;>.NotFound.AlsoNotFound;";
+ testSingleClass("Outer$ExtendsInner", signature, this::noWarnings, inspector -> {
+ assertThat(inspector.clazz("NotFound"), not(isPresent()));
+ ClassSubject outer = inspector.clazz("Outer$ExtendsInner");
+ assertEquals(signature, outer.getOriginalSignatureAttribute());
+ });
+ }
+
+ @Test
public void classSignatureOuter_invalid() throws Exception {
testSingleClass("Outer", "X", diagnostics -> {
assertEquals(1, diagnostics.warnings.size());
DiagnosticsChecker.checkDiagnostic(diagnostics.warnings.get(0), this::isOriginUnknown,
- "Invalid class signature for class Outer", "Expected L at position 1");
+ "Invalid signature for class Outer", "Expected L at position 1");
}, inspector -> noSignatureAttribute(inspector.clazz("Outer")));
}
@@ -446,7 +486,7 @@
testSingleClass("Outer", "<L", diagnostics -> {
assertEquals(1, diagnostics.warnings.size());
DiagnosticsChecker.checkDiagnostic(diagnostics.warnings.get(0), this::isOriginUnknown,
- "Invalid class signature for class Outer", "Unexpected end of signature at position 3");
+ "Invalid signature for class Outer", "Unexpected end of signature at position 3");
}, inspector -> noSignatureAttribute(inspector.clazz("Outer")));
}
@@ -455,7 +495,7 @@
testSingleClass("Outer$ExtendsInner", "X", diagnostics -> {
assertEquals(1, diagnostics.warnings.size());
DiagnosticsChecker.checkDiagnostic(diagnostics.warnings.get(0), this::isOriginUnknown,
- "Invalid class signature for class Outer$ExtendsInner", "Expected L at position 1");
+ "Invalid signature for class Outer$ExtendsInner", "Expected L at position 1");
}, inspector -> noSignatureAttribute(inspector.clazz("Outer$ExtendsInner")));
}
@@ -464,7 +504,7 @@
testSingleClass("Outer$Inner$ExtendsInnerInner", "X", diagnostics -> {
assertEquals(1, diagnostics.warnings.size());
DiagnosticsChecker.checkDiagnostic(diagnostics.warnings.get(0), this::isOriginUnknown,
- "Invalid class signature for class Outer$Inner$ExtendsInnerInner",
+ "Invalid signature for class Outer$Inner$ExtendsInnerInner",
"Expected L at position 1");
}, inspector -> noSignatureAttribute(inspector.clazz("Outer$Inner$ExtendsInnerInner")));
}
@@ -488,7 +528,7 @@
testSingleClass("Outer$ExtendsInner", signature, diagnostics -> {
assertEquals(1, diagnostics.warnings.size());
DiagnosticsChecker.checkDiagnostic(diagnostics.warnings.get(0), this::isOriginUnknown,
- "Invalid class signature for class Outer$ExtendsInner", "Expected ; at position 16");
+ "Invalid signature for class Outer$ExtendsInner", "Expected ; at position 16");
}, inspector -> {
noSignatureAttribute(inspector.clazz("Outer$ExtendsInner"));
});
diff --git a/src/test/java/com/android/tools/r8/naming/MinifierFieldSignatureTest.java b/src/test/java/com/android/tools/r8/naming/MinifierFieldSignatureTest.java
new file mode 100644
index 0000000..e48ce56
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/naming/MinifierFieldSignatureTest.java
@@ -0,0 +1,289 @@
+// 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.naming;
+
+import static com.android.tools.r8.utils.DexInspectorMatchers.isPresent;
+import static com.android.tools.r8.utils.DexInspectorMatchers.isRenamed;
+import static org.hamcrest.CoreMatchers.not;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertThat;
+import static org.objectweb.asm.Opcodes.ACC_FINAL;
+import static org.objectweb.asm.Opcodes.ACC_PUBLIC;
+import static org.objectweb.asm.Opcodes.ACC_SUPER;
+import static org.objectweb.asm.Opcodes.ACC_SYNTHETIC;
+import static org.objectweb.asm.Opcodes.ALOAD;
+import static org.objectweb.asm.Opcodes.INVOKESPECIAL;
+import static org.objectweb.asm.Opcodes.PUTFIELD;
+import static org.objectweb.asm.Opcodes.RETURN;
+import static org.objectweb.asm.Opcodes.V1_8;
+
+import com.android.tools.r8.DexIndexedConsumer;
+import com.android.tools.r8.DiagnosticsChecker;
+import com.android.tools.r8.R8Command;
+import com.android.tools.r8.StringConsumer;
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.ToolHelper;
+import com.android.tools.r8.graph.invokesuper.Consumer;
+import com.android.tools.r8.origin.Origin;
+import com.android.tools.r8.utils.DexInspector;
+import com.android.tools.r8.utils.DexInspector.ClassSubject;
+import com.android.tools.r8.utils.DexInspector.FieldSubject;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import java.util.Map;
+import org.junit.Test;
+import org.objectweb.asm.ClassWriter;
+import org.objectweb.asm.FieldVisitor;
+import org.objectweb.asm.MethodVisitor;
+
+
+public class MinifierFieldSignatureTest extends TestBase {
+ /*
+
+ class Fields<X extends String> {
+ class Inner {
+ }
+ public X anX;
+ public X[] anArrayOfX;
+ public Fields<X> aFieldsOfX;
+ public Fields<X>.Inner aFieldsOfXInner;
+ }
+
+ */
+
+ private String anXSignature = "TX;";
+ private String anArrayOfXSignature = "[TX;";
+ private String aFieldsOfXSignature = "LFields<TX;>;";
+ private String aFieldsOfXInnerSignature = "LFields<TX;>.Inner;";
+
+ public byte[] dumpFields(Map<String, String> signatures) throws Exception {
+
+ ClassWriter cw = new ClassWriter(0);
+ FieldVisitor fv;
+ MethodVisitor mv;
+ String signature;
+
+ cw.visit(V1_8, ACC_SUPER, "Fields", "<X:Ljava/lang/String;>Ljava/lang/Object;",
+ "java/lang/Object", null);
+
+ cw.visitInnerClass("Fields$Inner", "Fields", "Inner", 0);
+
+ {
+ signature = signatures.get("anX");
+ signature = signature == null ? anXSignature : signature;
+ fv = cw.visitField(ACC_PUBLIC, "anX", "Ljava/lang/String;", signature, null);
+ fv.visitEnd();
+ }
+ {
+ signature = signatures.get("anArrayOfX");
+ signature = signature == null ? anArrayOfXSignature : signature;
+ fv = cw.visitField(
+ ACC_PUBLIC, "anArrayOfX", "[Ljava/lang/String;", signature, null);
+ fv.visitEnd();
+ }
+ {
+ signature = signatures.get("aFieldsOfX");
+ signature = signature == null ? aFieldsOfXSignature : signature;
+ fv = cw.visitField(ACC_PUBLIC, "aFieldsOfX", "LFields;", signature, null);
+ fv.visitEnd();
+ }
+ {
+ signature = signatures.get("aFieldsOfXInner");
+ signature = signature == null ? aFieldsOfXInnerSignature : signature;
+ fv = cw.visitField(
+ ACC_PUBLIC, "aFieldsOfXInner", "LFields$Inner;", signature, null);
+ fv.visitEnd();
+ }
+ {
+ mv = cw.visitMethod(0, "<init>", "()V", null, null);
+ mv.visitCode();
+ mv.visitVarInsn(ALOAD, 0);
+ mv.visitMethodInsn(INVOKESPECIAL, "java/lang/Object", "<init>", "()V", false);
+ mv.visitInsn(RETURN);
+ mv.visitMaxs(1, 1);
+ mv.visitEnd();
+ }
+ cw.visitEnd();
+
+ return cw.toByteArray();
+ }
+
+ public byte[] dumpInner() throws Exception {
+
+ ClassWriter cw = new ClassWriter(0);
+ FieldVisitor fv;
+ MethodVisitor mv;
+
+ cw.visit(V1_8, ACC_SUPER, "Fields$Inner", null, "java/lang/Object", null);
+
+ cw.visitInnerClass("Fields$Inner", "Fields", "Inner", 0);
+
+ {
+ fv = cw.visitField(ACC_FINAL + ACC_SYNTHETIC, "this$0", "LFields;", null, null);
+ fv.visitEnd();
+ }
+ {
+ mv = cw.visitMethod(0, "<init>", "(LFields;)V", null, null);
+ mv.visitCode();
+ mv.visitVarInsn(ALOAD, 0);
+ mv.visitVarInsn(ALOAD, 1);
+ mv.visitFieldInsn(PUTFIELD, "Fields$Inner", "this$0", "LFields;");
+ mv.visitVarInsn(ALOAD, 0);
+ mv.visitMethodInsn(INVOKESPECIAL, "java/lang/Object", "<init>", "()V", false);
+ mv.visitInsn(RETURN);
+ mv.visitMaxs(2, 2);
+ mv.visitEnd();
+ }
+ cw.visitEnd();
+
+ return cw.toByteArray();
+ }
+
+ private FieldSubject lookupAnX(DexInspector inspector) {
+ ClassSubject clazz = inspector.clazz("Fields");
+ return clazz.field("java.lang.String", "anX");
+ }
+
+ private FieldSubject lookupAnArrayOfX(DexInspector inspector) {
+ ClassSubject clazz = inspector.clazz("Fields");
+ return clazz.field("java.lang.String[]", "anArrayOfX");
+ }
+
+ private FieldSubject lookupAFieldsOfX(DexInspector inspector) {
+ ClassSubject clazz = inspector.clazz("Fields");
+ return clazz.field("Fields", "aFieldsOfX");
+ }
+
+ public void runTest(
+ ImmutableMap<String, String> signatures,
+ Consumer<DiagnosticsChecker> diagnostics,
+ Consumer<DexInspector> inspect)
+ throws Exception {
+ DiagnosticsChecker checker = new DiagnosticsChecker();
+ DexInspector inspector = new DexInspector(
+ ToolHelper.runR8(R8Command.builder(checker)
+ .addClassProgramData(dumpFields(signatures), Origin.unknown())
+ .addClassProgramData(dumpInner(), Origin.unknown())
+ .addProguardConfiguration(ImmutableList.of(
+ "-keepattributes InnerClasses,EnclosingMethod,Signature",
+ "-keep,allowobfuscation class ** { *; }"
+ ), Origin.unknown())
+ .setProgramConsumer(DexIndexedConsumer.emptyConsumer())
+ .setProguardMapConsumer(StringConsumer.emptyConsumer())
+ .build()));
+ // All classes are kept, and renamed.
+ ClassSubject clazz = inspector.clazz("Fields");
+ assertThat(clazz, isRenamed());
+ assertThat(inspector.clazz("Fields$Inner"), isRenamed());
+
+ FieldSubject anX = lookupAnX(inspector);
+ FieldSubject anArrayOfX = lookupAnArrayOfX(inspector);
+ FieldSubject aFieldsOfX =lookupAFieldsOfX(inspector);
+ FieldSubject aFieldsOfXInner = clazz.field("Fields$Inner", "aFieldsOfXInner");
+
+ // Check that all fields have been renamed
+ assertThat(anX, isRenamed());
+ assertThat(anArrayOfX, isRenamed());
+ assertThat(aFieldsOfX, isRenamed());
+ assertThat(aFieldsOfXInner, isRenamed());
+
+ //System.out.println(generic.getFinalSignatureAttribute());
+ //System.out.println(generic.getOriginalSignatureAttribute());
+
+ // Test that methods have their original signature if the default was provided.
+ if (!signatures.containsKey("anX")) {
+ assertEquals(anXSignature, anX.getOriginalSignatureAttribute());
+ }
+ if (!signatures.containsKey("anArrayOfX")) {
+ assertEquals(anArrayOfXSignature, anArrayOfX.getOriginalSignatureAttribute());
+ }
+ if (!signatures.containsKey("aFieldsOfX")) {
+ assertEquals(
+ aFieldsOfXSignature, aFieldsOfX.getOriginalSignatureAttribute());
+ }
+ if (!signatures.containsKey("aFieldsOfXInner")) {
+ assertEquals(
+ aFieldsOfXInnerSignature, aFieldsOfXInner.getOriginalSignatureAttribute());
+ }
+
+ diagnostics.accept(checker);
+ inspect.accept(inspector);
+ }
+
+ private void testSingleField(String name, String signature,
+ Consumer<DiagnosticsChecker> diagnostics,
+ Consumer<DexInspector> inspector)
+ throws Exception {
+ ImmutableMap<String, String> signatures = ImmutableMap.of(name, signature);
+ runTest(signatures, diagnostics, inspector);
+ }
+
+ private void isOriginUnknown(Origin origin) {
+ assertSame(Origin.unknown(), origin);
+ }
+
+ private void noWarnings(DiagnosticsChecker checker) {
+ assertEquals(0, checker.warnings.size());
+ }
+
+ private void noInspection(DexInspector inspector) {
+ }
+
+ private void noSignatureAttribute(FieldSubject field) {
+ assertThat(field, isPresent());
+ assertNull(field.getFinalSignatureAttribute());
+ assertNull(field.getOriginalSignatureAttribute());
+ }
+
+ @Test
+ public void originalJavacSignatures() throws Exception {
+ // Test using the signatures generated by javac.
+ runTest(ImmutableMap.of(), this::noWarnings, this::noInspection);
+ }
+
+ @Test
+ public void signatureEmpty() throws Exception {
+ testSingleField("anX", "", this::noWarnings,
+ inspector -> noSignatureAttribute(lookupAnX(inspector)));
+ }
+
+ @Test
+ public void signatureInvalid() throws Exception {
+ testSingleField("anX", "X", diagnostics -> {
+ assertEquals(1, diagnostics.warnings.size());
+ // TODO(sgjesse): The position 2 reported here is one off.
+ DiagnosticsChecker.checkDiagnostic(diagnostics.warnings.get(0), this::isOriginUnknown,
+ "Invalid signature for field",
+ "java.lang.String Fields.anX",
+ "Expected L, [ or T at position 2");
+ }, inspector -> noSignatureAttribute(lookupAnX(inspector)));
+ }
+
+ @Test
+ public void classNotFound() throws Exception {
+ String signature = "LNotFound<TX;>.InnerNotFound.InnerAlsoNotFound;";
+ testSingleField("anX", signature, this::noWarnings, inspector -> {
+ assertThat(inspector.clazz("NotFound"), not(isPresent()));
+ assertEquals(signature, lookupAnX(inspector).getOriginalSignatureAttribute());
+ });
+ }
+
+ @Test
+ public void multipleWarnings() throws Exception {
+ runTest(ImmutableMap.of(
+ "anX", "X",
+ "anArrayOfX", "X",
+ "aFieldsOfX", "X"
+ ), diagnostics -> {
+ assertEquals(3, diagnostics.warnings.size());
+ }, inspector -> {
+ noSignatureAttribute(lookupAnX(inspector));
+ noSignatureAttribute(lookupAnArrayOfX(inspector));
+ noSignatureAttribute(lookupAFieldsOfX(inspector));
+ });
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/naming/MinifierMethodSignatureTest.java b/src/test/java/com/android/tools/r8/naming/MinifierMethodSignatureTest.java
new file mode 100644
index 0000000..41750ae
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/naming/MinifierMethodSignatureTest.java
@@ -0,0 +1,314 @@
+// 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.naming;
+
+import static com.android.tools.r8.utils.DexInspectorMatchers.isPresent;
+import static com.android.tools.r8.utils.DexInspectorMatchers.isRenamed;
+import static org.hamcrest.CoreMatchers.not;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertThat;
+import static org.objectweb.asm.Opcodes.ACC_FINAL;
+import static org.objectweb.asm.Opcodes.ACC_PUBLIC;
+import static org.objectweb.asm.Opcodes.ACC_STATIC;
+import static org.objectweb.asm.Opcodes.ACC_SUPER;
+import static org.objectweb.asm.Opcodes.ACC_SYNTHETIC;
+import static org.objectweb.asm.Opcodes.ACONST_NULL;
+import static org.objectweb.asm.Opcodes.ALOAD;
+import static org.objectweb.asm.Opcodes.ARETURN;
+import static org.objectweb.asm.Opcodes.INVOKESPECIAL;
+import static org.objectweb.asm.Opcodes.PUTFIELD;
+import static org.objectweb.asm.Opcodes.RETURN;
+import static org.objectweb.asm.Opcodes.V1_8;
+
+import com.android.tools.r8.DexIndexedConsumer;
+import com.android.tools.r8.DiagnosticsChecker;
+import com.android.tools.r8.R8Command;
+import com.android.tools.r8.StringConsumer;
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.ToolHelper;
+import com.android.tools.r8.graph.invokesuper.Consumer;
+import com.android.tools.r8.origin.Origin;
+import com.android.tools.r8.utils.DexInspector;
+import com.android.tools.r8.utils.DexInspector.ClassSubject;
+import com.android.tools.r8.utils.DexInspector.MethodSubject;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import java.util.Map;
+import org.junit.Test;
+import org.objectweb.asm.ClassWriter;
+import org.objectweb.asm.FieldVisitor;
+import org.objectweb.asm.MethodVisitor;
+
+public class MinifierMethodSignatureTest extends TestBase {
+ /*
+
+ class Methods<X extends Throwable> {
+ class Inner {
+ }
+ public static <T extends Throwable> T generic(T a, Methods<T>.Inner b) { return null; }
+ public Methods<X>.Inner parameterizedReturn() { return null; }
+ public void parameterizedArguments(X a, Methods<X>.Inner b) { }
+ public void parametrizedThrows() throws X { }
+ }
+
+ */
+
+ private String genericSignature = "<T:Ljava/lang/Throwable;>(TT;LMethods<TT;>.Inner;)TT;";
+ private String parameterizedReturnSignature = "()LMethods<TX;>.Inner;";
+ private String parameterizedArgumentsSignature = "(TX;LMethods<TX;>.Inner;)V";
+ private String parametrizedThrowsSignature = "()V^TX;";
+
+ private byte[] dumpMethods(Map<String, String> signatures) throws Exception {
+
+ ClassWriter cw = new ClassWriter(0);
+ MethodVisitor mv;
+ String signature;
+
+ cw.visit(V1_8, ACC_SUPER, "Methods", "<X:Ljava/lang/Throwable;>Ljava/lang/Object;",
+ "java/lang/Object", null);
+
+ cw.visitInnerClass("Methods$Inner", "Methods", "Inner", 0);
+
+ {
+ mv = cw.visitMethod(0, "<init>", "()V", null, null);
+ mv.visitCode();
+ mv.visitVarInsn(ALOAD, 0);
+ mv.visitMethodInsn(INVOKESPECIAL, "java/lang/Object", "<init>", "()V", false);
+ mv.visitInsn(RETURN);
+ mv.visitMaxs(1, 1);
+ mv.visitEnd();
+ }
+ {
+ signature = signatures.get("generic");
+ signature = signature == null ? genericSignature : signature;
+ mv = cw.visitMethod(ACC_PUBLIC + ACC_STATIC, "generic",
+ "(Ljava/lang/Throwable;LMethods$Inner;)Ljava/lang/Throwable;",
+ signature, null);
+ mv.visitCode();
+ mv.visitInsn(ACONST_NULL);
+ mv.visitInsn(ARETURN);
+ mv.visitMaxs(1, 2);
+ mv.visitEnd();
+ }
+ {
+ signature = signatures.get("parameterizedReturn");
+ signature = signature == null ? parameterizedReturnSignature : signature;
+ mv = cw.visitMethod(ACC_PUBLIC, "parameterizedReturn", "()LMethods$Inner;",
+ signature, null);
+ mv.visitCode();
+ mv.visitInsn(ACONST_NULL);
+ mv.visitInsn(ARETURN);
+ mv.visitMaxs(1, 1);
+ mv.visitEnd();
+ }
+ {
+ signature = signatures.get("parameterizedArguments");
+ signature = signature == null ? parameterizedArgumentsSignature : signature;
+ mv = cw.visitMethod(ACC_PUBLIC, "parameterizedArguments",
+ "(Ljava/lang/Throwable;LMethods$Inner;)V", signature, null);
+ mv.visitCode();
+ mv.visitInsn(RETURN);
+ mv.visitMaxs(0, 3);
+ mv.visitEnd();
+ }
+ {
+ signature = signatures.get("parametrizedThrows");
+ signature = signature == null ? parametrizedThrowsSignature : signature;
+ mv = cw.visitMethod(ACC_PUBLIC, "parametrizedThrows", "()V", signature,
+ new String[] { "java/lang/Throwable" });
+ mv.visitCode();
+ mv.visitInsn(RETURN);
+ mv.visitMaxs(0, 1);
+ mv.visitEnd();
+ }
+ cw.visitEnd();
+
+ return cw.toByteArray();
+ }
+
+ private byte[] dumpInner() throws Exception {
+
+ ClassWriter cw = new ClassWriter(0);
+ FieldVisitor fv;
+ MethodVisitor mv;
+
+ cw.visit(V1_8, ACC_SUPER, "Methods$Inner", null, "java/lang/Object", null);
+
+ cw.visitInnerClass("Methods$Inner", "Methods", "Inner", 0);
+
+ {
+ fv = cw.visitField(ACC_FINAL + ACC_SYNTHETIC, "this$0", "LMethods;", null, null);
+ fv.visitEnd();
+ }
+ {
+ mv = cw.visitMethod(0, "<init>", "(LMethods;)V", null, null);
+ mv.visitCode();
+ mv.visitVarInsn(ALOAD, 0);
+ mv.visitVarInsn(ALOAD, 1);
+ mv.visitFieldInsn(PUTFIELD, "Methods$Inner", "this$0", "LMethods;");
+ mv.visitVarInsn(ALOAD, 0);
+ mv.visitMethodInsn(INVOKESPECIAL, "java/lang/Object", "<init>", "()V", false);
+ mv.visitInsn(RETURN);
+ mv.visitMaxs(2, 2);
+ mv.visitEnd();
+ }
+ cw.visitEnd();
+
+ return cw.toByteArray();
+ }
+
+ private MethodSubject lookupGeneric(DexInspector inspector) {
+ ClassSubject clazz = inspector.clazz("Methods");
+ return clazz.method(
+ "java.lang.Throwable", "generic", ImmutableList.of("java.lang.Throwable", "Methods$Inner"));
+ }
+
+ private MethodSubject lookupParameterizedReturn(DexInspector inspector) {
+ ClassSubject clazz = inspector.clazz("Methods");
+ return clazz.method(
+ "Methods$Inner", "parameterizedReturn", ImmutableList.of());
+ }
+
+ private MethodSubject lookupParameterizedArguments(DexInspector inspector) {
+ ClassSubject clazz = inspector.clazz("Methods");
+ return clazz.method(
+ "void", "parameterizedArguments", ImmutableList.of("java.lang.Throwable", "Methods$Inner"));
+ }
+
+ public void runTest(
+ ImmutableMap<String, String> signatures,
+ Consumer<DiagnosticsChecker> diagnostics,
+ Consumer<DexInspector> inspect)
+ throws Exception {
+ DiagnosticsChecker checker = new DiagnosticsChecker();
+ DexInspector inspector = new DexInspector(
+ ToolHelper.runR8(R8Command.builder(checker)
+ .addClassProgramData(dumpMethods(signatures), Origin.unknown())
+ .addClassProgramData(dumpInner(), Origin.unknown())
+ .addProguardConfiguration(ImmutableList.of(
+ "-keepattributes InnerClasses,EnclosingMethod,Signature",
+ "-keep,allowobfuscation class ** { *; }"
+ ), Origin.unknown())
+ .setProgramConsumer(DexIndexedConsumer.emptyConsumer())
+ .setProguardMapConsumer(StringConsumer.emptyConsumer())
+ .build()));
+ // All classes are kept, and renamed.
+ ClassSubject clazz = inspector.clazz("Methods");
+ assertThat(clazz, isRenamed());
+ assertThat(inspector.clazz("Methods$Inner"), isRenamed());
+
+ MethodSubject generic = lookupGeneric(inspector);
+ MethodSubject parameterizedReturn = lookupParameterizedReturn(inspector);
+ MethodSubject parameterizedArguments = lookupParameterizedArguments(inspector);
+ MethodSubject parametrizedThrows =
+ clazz.method("void", "parametrizedThrows", ImmutableList.of());
+
+ // Check that all methods have been renamed
+ assertThat(generic, isRenamed());
+ assertThat(parameterizedReturn, isRenamed());
+ assertThat(parameterizedArguments, isRenamed());
+ assertThat(parametrizedThrows, isRenamed());
+
+ // Test that methods have their original signature if the default was provided.
+ if (!signatures.containsKey("generic")) {
+ assertEquals(genericSignature, generic.getOriginalSignatureAttribute());
+ }
+ if (!signatures.containsKey("parameterizedReturn")) {
+ assertEquals(
+ parameterizedReturnSignature, parameterizedReturn.getOriginalSignatureAttribute());
+ }
+ if (!signatures.containsKey("parameterizedArguments")) {
+ assertEquals(
+ parameterizedArgumentsSignature, parameterizedArguments.getOriginalSignatureAttribute());
+ }
+ if (!signatures.containsKey("parametrizedThrows")) {
+ assertEquals(
+ parametrizedThrowsSignature, parametrizedThrows.getOriginalSignatureAttribute());
+ }
+
+ diagnostics.accept(checker);
+ inspect.accept(inspector);
+ }
+
+ private void testSingleMethod(String name, String signature,
+ Consumer<DiagnosticsChecker> diagnostics,
+ Consumer<DexInspector> inspector)
+ throws Exception {
+ ImmutableMap<String, String> signatures = ImmutableMap.of(name, signature);
+ runTest(signatures, diagnostics, inspector);
+ }
+
+ private void isOriginUnknown(Origin origin) {
+ assertSame(Origin.unknown(), origin);
+ }
+
+ private void noWarnings(DiagnosticsChecker checker) {
+ assertEquals(0, checker.warnings.size());
+ }
+
+ private void noInspection(DexInspector inspector) {
+ }
+
+ private void noSignatureAttribute(MethodSubject method) {
+ assertThat(method, isPresent());
+ assertNull(method.getFinalSignatureAttribute());
+ assertNull(method.getOriginalSignatureAttribute());
+ }
+
+ @Test
+ public void originalJavacSignatures() throws Exception {
+ // Test using the signatures generated by javac.
+ runTest(ImmutableMap.of(), this::noWarnings, this::noInspection);
+ }
+
+ @Test
+ public void signatureEmpty() throws Exception {
+ testSingleMethod("generic", "", this::noWarnings, inspector -> {
+ noSignatureAttribute(lookupGeneric(inspector));
+ });
+ }
+
+ @Test
+ public void signatureInvalid() throws Exception {
+ testSingleMethod("generic", "X", diagnostics -> {
+ assertEquals(1, diagnostics.warnings.size());
+ DiagnosticsChecker.checkDiagnostic(diagnostics.warnings.get(0), this::isOriginUnknown,
+ "Invalid signature for method",
+ "java.lang.Throwable Methods.generic(java.lang.Throwable, Methods$Inner)",
+ "Expected ( at position 1");
+ }, inspector -> noSignatureAttribute(lookupGeneric(inspector)));
+ }
+
+ @Test
+ public void classNotFound() throws Exception {
+ String signature = "<T:LNotFound;>(TT;LAlsoNotFound<TT;>.InnerNotFound.InnerAlsoNotFound;)TT;";
+ testSingleMethod("generic", signature, this::noWarnings,
+ inspector -> {
+ ClassSubject methods = inspector.clazz("Methods");
+ MethodSubject method =
+ methods.method("java.lang.Throwable", "generic",
+ ImmutableList.of("java.lang.Throwable", "Methods$Inner"));
+ assertThat(inspector.clazz("NotFound"), not(isPresent()));
+ assertEquals(signature, method.getOriginalSignatureAttribute());
+ });
+ }
+
+ @Test
+ public void multipleWarnings() throws Exception {
+ runTest(ImmutableMap.of(
+ "generic", "X",
+ "parameterizedReturn", "X",
+ "parameterizedArguments", "X"
+ ), diagnostics -> {
+ assertEquals(3, diagnostics.warnings.size());
+ }, inspector -> {
+ noSignatureAttribute(lookupGeneric(inspector));
+ noSignatureAttribute(lookupParameterizedReturn(inspector));
+ noSignatureAttribute(lookupParameterizedArguments(inspector));
+ });
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/utils/DexInspector.java b/src/test/java/com/android/tools/r8/utils/DexInspector.java
index e7e882a..fd32ec7 100644
--- a/src/test/java/com/android/tools/r8/utils/DexInspector.java
+++ b/src/test/java/com/android/tools/r8/utils/DexInspector.java
@@ -886,7 +886,31 @@
@Override
public MethodSignature getOriginalSignature() {
MethodSignature signature = getFinalSignature();
- MemberNaming memberNaming = clazz.naming != null ? clazz.naming.lookup(signature) : null;
+ if (clazz.naming == null) {
+ return signature;
+ }
+
+ // Map the parameters and return type to original names. This is needed as the in the
+ // Proguard map the names on the left side are the original names. E.g.
+ //
+ // X -> a
+ // X method(X) -> a
+ //
+ // whereas the final signature is for X.a is "a (a)"
+ String[] OriginalParameters = new String[signature.parameters.length];
+ for (int i = 0; i < OriginalParameters.length; i++) {
+ String obfuscated = signature.parameters[i];
+ String original = originalToObfuscatedMapping.inverse().get(obfuscated);
+ OriginalParameters[i] = original != null ? original : obfuscated;
+ }
+ String obfuscatedReturnType = signature.type;
+ String originalReturnType = originalToObfuscatedMapping.inverse().get(obfuscatedReturnType);
+ String returnType = originalReturnType != null ? originalReturnType : obfuscatedReturnType;
+
+ MethodSignature lookupSignature =
+ new MethodSignature(signature.name, returnType, OriginalParameters);
+
+ MemberNaming memberNaming = clazz.naming.lookup(lookupSignature);
return memberNaming != null
? (MethodSignature) memberNaming.getOriginalSignature()
: signature;
@@ -1035,7 +1059,24 @@
@Override
public FieldSignature getOriginalSignature() {
FieldSignature signature = getFinalSignature();
- MemberNaming memberNaming = clazz.naming != null ? clazz.naming.lookup(signature) : null;
+ if (clazz.naming == null) {
+ return signature;
+ }
+
+ // Map the type to the original name. This is needed as the in the Proguard map the
+ // names on the left side are the original names. E.g.
+ //
+ // X -> a
+ // X field -> a
+ //
+ // whereas the final signature is for X.a is "a a"
+ String obfuscatedType = signature.type;
+ String originalType = originalToObfuscatedMapping.inverse().get(obfuscatedType);
+ String fieldType = originalType != null ? originalType : obfuscatedType;
+
+ FieldSignature lookupSignature = new FieldSignature(signature.name, fieldType);
+
+ MemberNaming memberNaming = clazz.naming.lookup(lookupSignature);
return memberNaming != null
? (FieldSignature) memberNaming.getOriginalSignature()
: signature;
