[Retrace] Identify ranges with cardinal to split overloaded mappings
Bug: 199058242
Bug: 145897713
Change-Id: Ibd0d2acec5072b17c5eea24e8162d751e2ac6e41
diff --git a/src/main/java/com/android/tools/r8/naming/ClassNaming.java b/src/main/java/com/android/tools/r8/naming/ClassNaming.java
index c9de8ab..83b7b3f 100644
--- a/src/main/java/com/android/tools/r8/naming/ClassNaming.java
+++ b/src/main/java/com/android/tools/r8/naming/ClassNaming.java
@@ -26,7 +26,7 @@
public abstract MappedRange addMappedRange(
Range obfuscatedRange,
MemberNaming.MethodSignature originalSignature,
- Object originalRange,
+ Range originalRange,
String obfuscatedName);
public abstract void addMappingInformation(
diff --git a/src/main/java/com/android/tools/r8/naming/ClassNamingForMapApplier.java b/src/main/java/com/android/tools/r8/naming/ClassNamingForMapApplier.java
index a222acb..3c83c90 100644
--- a/src/main/java/com/android/tools/r8/naming/ClassNamingForMapApplier.java
+++ b/src/main/java/com/android/tools/r8/naming/ClassNamingForMapApplier.java
@@ -88,7 +88,7 @@
public MappedRange addMappedRange(
Range obfuscatedRange,
MemberNaming.MethodSignature originalSignature,
- Object originalRange,
+ Range originalRange,
String obfuscatedName) {
return null;
}
diff --git a/src/main/java/com/android/tools/r8/naming/ClassNamingForNameMapper.java b/src/main/java/com/android/tools/r8/naming/ClassNamingForNameMapper.java
index 1d837b3..7a0a20c 100644
--- a/src/main/java/com/android/tools/r8/naming/ClassNamingForNameMapper.java
+++ b/src/main/java/com/android/tools/r8/naming/ClassNamingForNameMapper.java
@@ -85,7 +85,7 @@
public MappedRange addMappedRange(
Range minifiedRange,
MemberNaming.MethodSignature originalSignature,
- Object originalRange,
+ Range originalRange,
String renamedName) {
MappedRange range =
new MappedRange(minifiedRange, originalSignature, originalRange, renamedName);
@@ -413,7 +413,7 @@
public final Range minifiedRange; // Can be null, if so then originalRange must also be null.
public final MethodSignature signature;
- public final Object originalRange; // null, Integer or Range.
+ public final Range originalRange;
public final String renamedName;
/**
@@ -425,11 +425,7 @@
private List<MappingInformation> additionalMappingInfo = new ArrayList<>();
private MappedRange(
- Range minifiedRange, MethodSignature signature, Object originalRange, String renamedName) {
-
- assert originalRange == null
- || originalRange instanceof Integer
- || originalRange instanceof Range;
+ Range minifiedRange, MethodSignature signature, Range originalRange, String renamedName) {
this.minifiedRange = minifiedRange;
this.signature = signature;
@@ -466,13 +462,8 @@
if (originalRange == null) {
// Concrete identity mapping: "x:y:a() -> b"
return lineNumberAfterMinification;
- } else if (originalRange instanceof Integer) {
- // Inlinee: "x:y:a():u -> b"
- return (int) originalRange;
} else {
// "x:y:a():u:v -> b"
- assert originalRange instanceof Range;
- Range originalRange = (Range) this.originalRange;
if (originalRange.to == originalRange.from) {
// This is a single line mapping which we should report as the actual line.
return originalRange.to;
@@ -484,10 +475,8 @@
public int getFirstLineNumberOfOriginalRange() {
if (originalRange == null) {
return 0;
- } else if (originalRange instanceof Integer) {
- return (int) originalRange;
} else {
- return ((Range) originalRange).from;
+ return originalRange.from;
}
}
diff --git a/src/main/java/com/android/tools/r8/naming/ProguardMapReader.java b/src/main/java/com/android/tools/r8/naming/ProguardMapReader.java
index 52b63e0..1dcd902 100644
--- a/src/main/java/com/android/tools/r8/naming/ProguardMapReader.java
+++ b/src/main/java/com/android/tools/r8/naming/ProguardMapReader.java
@@ -293,8 +293,7 @@
MappedRange activeMappedRange = null;
Range previousMappedRange = null;
do {
- Object originalRange = null;
- Range mappedRange = null;
+ Range originalRange = null;
// Try to parse any information added in comments above member namings
if (isCommentLineWithJsonBrace()) {
final MemberNaming currentMember = activeMemberNaming;
@@ -326,13 +325,12 @@
break;
}
skipWhitespace();
- Object maybeRangeOrInt = maybeParseRangeOrInt();
- if (maybeRangeOrInt != null) {
- if (!(maybeRangeOrInt instanceof Range)) {
+ Range mappedRange = parseRange();
+ if (mappedRange != null) {
+ if (mappedRange.isCardinal) {
throw new ParseException(
- String.format("Invalid obfuscated line number range (%s).", maybeRangeOrInt));
+ String.format("Invalid obfuscated line number range (%s).", mappedRange));
}
- mappedRange = (Range) maybeRangeOrInt;
skipWhitespace();
expect(':');
}
@@ -343,7 +341,7 @@
// This is a mapping or inlining definition
nextChar();
skipWhitespace();
- originalRange = maybeParseRangeOrInt();
+ originalRange = parseRange();
if (originalRange == null) {
throw new ParseException("No number follows the colon after the method signature.");
}
@@ -371,7 +369,11 @@
if (activeMemberNaming != null) {
boolean changedName = !activeMemberNaming.getRenamedName().equals(renamedName);
boolean changedMappedRange = !Objects.equals(previousMappedRange, mappedRange);
- if (changedName || previousMappedRange == null || changedMappedRange) {
+ boolean originalRangeChange = originalRange == null || !originalRange.isCardinal;
+ if (changedName
+ || previousMappedRange == null
+ || changedMappedRange
+ || originalRangeChange) {
if (lastAddedNaming == null
|| !lastAddedNaming.getOriginalSignature().equals(activeMemberNaming.signature)) {
classNamingBuilder.addMemberEntry(activeMemberNaming);
@@ -407,7 +409,7 @@
nextChar();
isInit = true;
}
- // Progard sometimes outputs a ? as a method name. We have tools (dexsplitter) that depends
+ // Proguard sometimes outputs a ? as a method name. We have tools (dexsplitter) that depends
// on being able to map class names back to the original, but does not care if methods are
// correctly mapped. Using this on proguard output for anything else might not give correct
// remappings.
@@ -530,14 +532,14 @@
return '0' <= c && c <= '9';
}
- private Object maybeParseRangeOrInt() {
+ private Range parseRange() {
if (!isSimpleDigit(peekChar(0))) {
return null;
}
int from = parseNumber();
skipWhitespace();
if (peekChar(0) != ':') {
- return from;
+ return new Range(from);
}
expect(':');
skipWhitespace();
diff --git a/src/main/java/com/android/tools/r8/naming/Range.java b/src/main/java/com/android/tools/r8/naming/Range.java
index c91ae99..a8d8563 100644
--- a/src/main/java/com/android/tools/r8/naming/Range.java
+++ b/src/main/java/com/android/tools/r8/naming/Range.java
@@ -3,16 +3,28 @@
// BSD-style license that can be found in the LICENSE file.
package com.android.tools.r8.naming;
+import java.util.Objects;
+
/** Represents a line number range. */
public class Range {
public final int from;
public final int to;
+ public final boolean isCardinal;
+
+ public Range(int line) {
+ this(line, line, true);
+ }
public Range(int from, int to) {
+ this(from, to, false);
+ }
+
+ private Range(int from, int to, boolean isCardinal) {
this.from = from;
this.to = to;
- // TODO(b/145897713): Seems like we should be able to assert from <= to.
+ this.isCardinal = isCardinal;
+ assert from <= to;
}
public boolean contains(int value) {
@@ -21,7 +33,7 @@
@Override
public String toString() {
- return from + ":" + to;
+ return isCardinal ? (from + "") : (from + ":" + to);
}
@Override
@@ -34,13 +46,11 @@
}
Range range = (Range) o;
- return from == range.from && to == range.to;
+ return from == range.from && to == range.to && isCardinal == range.isCardinal;
}
@Override
public int hashCode() {
- int result = from;
- result = 31 * result + to;
- return result;
+ return Objects.hash(from, to, isCardinal);
}
}
diff --git a/src/main/java/com/android/tools/r8/retrace/internal/RetraceClassResultImpl.java b/src/main/java/com/android/tools/r8/retrace/internal/RetraceClassResultImpl.java
index 1163213..cfc923a 100644
--- a/src/main/java/com/android/tools/r8/retrace/internal/RetraceClassResultImpl.java
+++ b/src/main/java/com/android/tools/r8/retrace/internal/RetraceClassResultImpl.java
@@ -18,6 +18,7 @@
import com.android.tools.r8.retrace.RetraceClassResult;
import com.android.tools.r8.retrace.RetraceFrameResult;
import com.android.tools.r8.retrace.Retracer;
+import com.android.tools.r8.utils.ListUtils;
import com.android.tools.r8.utils.Pair;
import com.google.common.collect.ImmutableList;
import java.util.ArrayList;
@@ -141,29 +142,48 @@
List<Pair<RetraceClassElementImpl, List<MappedRange>>> mappings = new ArrayList<>();
internalStream()
.forEach(
- element ->
- mappings.add(
- new Pair<>(element, getMappedRangesForFrame(element, definition, position))));
+ element -> {
+ getMappedRangesForFrame(element, definition, position)
+ .forEach(
+ mappedRanges -> {
+ mappings.add(new Pair<>(element, mappedRanges));
+ });
+ });
return new RetraceFrameResultImpl(this, mappings, definition, position, retracer);
}
- private List<MappedRange> getMappedRangesForFrame(
+ private List<List<MappedRange>> getMappedRangesForFrame(
RetraceClassElementImpl element, MethodDefinition definition, int position) {
+ List<List<MappedRange>> overloadedRanges = new ArrayList<>();
if (mapper == null) {
- return null;
+ overloadedRanges.add(null);
+ return overloadedRanges;
}
assert element.mapper != null;
MappedRangesOfName mappedRanges = mapper.mappedRangesByRenamedName.get(definition.getName());
if (mappedRanges == null || mappedRanges.getMappedRanges().isEmpty()) {
- return null;
+ overloadedRanges.add(null);
+ return overloadedRanges;
}
- if (position < 0) {
- return mappedRanges.getMappedRanges();
+ List<MappedRange> mappedRangesForPosition = null;
+ if (position >= 0) {
+ mappedRangesForPosition = mappedRanges.allRangesForLine(position, false);
}
- List<MappedRange> mappedRangesForPosition = mappedRanges.allRangesForLine(position, false);
- return mappedRangesForPosition.isEmpty()
- ? mappedRanges.getMappedRanges()
- : mappedRangesForPosition;
+ if (mappedRangesForPosition == null || mappedRangesForPosition.isEmpty()) {
+ mappedRangesForPosition = mappedRanges.getMappedRanges();
+ }
+ assert mappedRangesForPosition != null && !mappedRangesForPosition.isEmpty();
+ // Mapped ranges can have references to overloaded signatures. We distinguish those by looking
+ // at the cardinal mapping range.
+ for (MappedRange mappedRange : mappedRangesForPosition) {
+ if (overloadedRanges.isEmpty()
+ || mappedRange.originalRange == null
+ || !mappedRange.originalRange.isCardinal) {
+ overloadedRanges.add(new ArrayList<>());
+ }
+ ListUtils.last(overloadedRanges).add(mappedRange);
+ }
+ return overloadedRanges;
}
@Override
@@ -334,14 +354,16 @@
private RetraceFrameResultImpl lookupFrame(MethodDefinition definition, int position) {
MethodDefinition methodDefinition =
MethodDefinition.create(classReference.getClassReference(), definition.getName());
+ ImmutableList.Builder<Pair<RetraceClassElementImpl, List<MappedRange>>> builder =
+ ImmutableList.builder();
+ classResult
+ .getMappedRangesForFrame(this, methodDefinition, position)
+ .forEach(
+ mappedRanges -> {
+ builder.add(new Pair<>(this, mappedRanges));
+ });
return new RetraceFrameResultImpl(
- classResult,
- ImmutableList.of(
- new Pair<>(
- this, classResult.getMappedRangesForFrame(this, methodDefinition, position))),
- methodDefinition,
- position,
- classResult.retracer);
+ classResult, builder.build(), methodDefinition, position, classResult.retracer);
}
}
}
diff --git a/src/main/java/com/android/tools/r8/utils/LineNumberOptimizer.java b/src/main/java/com/android/tools/r8/utils/LineNumberOptimizer.java
index ec1b7d0..48af35f 100644
--- a/src/main/java/com/android/tools/r8/utils/LineNumberOptimizer.java
+++ b/src/main/java/com/android/tools/r8/utils/LineNumberOptimizer.java
@@ -482,7 +482,7 @@
classNamingBuilder.addMappedRange(
obfuscatedRange,
getOriginalMethodSignature.apply(caller.method),
- Math.max(caller.line, 0), // Prevent against "no-position".
+ new Range(Math.max(caller.line, 0)), // Prevent against "no-position".
obfuscatedName);
caller = caller.callerPosition;
}
diff --git a/src/test/java/com/android/tools/r8/debuginfo/InliningWithoutPositionsTestRunner.java b/src/test/java/com/android/tools/r8/debuginfo/InliningWithoutPositionsTestRunner.java
index 9efecb4..17dadf2 100644
--- a/src/test/java/com/android/tools/r8/debuginfo/InliningWithoutPositionsTestRunner.java
+++ b/src/test/java/com/android/tools/r8/debuginfo/InliningWithoutPositionsTestRunner.java
@@ -195,8 +195,10 @@
MappedRange range) {
assertEquals(function, range.signature.name);
int expectedLineNumber = hasPosition ? location.line : 0;
- Object expectedOriginalRange =
- innermostFrame ? new Range(expectedLineNumber, expectedLineNumber) : expectedLineNumber;
+ Range expectedOriginalRange =
+ innermostFrame
+ ? new Range(expectedLineNumber, expectedLineNumber)
+ : new Range(expectedLineNumber);
assertEquals(expectedOriginalRange, range.originalRange);
}
}
diff --git a/src/test/java/com/android/tools/r8/retrace/stacktraces/InlineFileNameStackTrace.java b/src/test/java/com/android/tools/r8/retrace/stacktraces/InlineFileNameStackTrace.java
index ebc23a1..ef36965 100644
--- a/src/test/java/com/android/tools/r8/retrace/stacktraces/InlineFileNameStackTrace.java
+++ b/src/test/java/com/android/tools/r8/retrace/stacktraces/InlineFileNameStackTrace.java
@@ -33,8 +33,8 @@
"com.android.tools.r8.naming.retrace.Main -> com.android.tools.r8.naming.retrace.Main:",
" 3:3:void foo.Bar$Baz.baz(long):0:0 -> main",
" 3:3:void Foo$Bar.bar(int):2 -> main",
- " 3:3:void com.android.tools.r8.naming.retrace.Main$Foo.method1(java.lang.String):8:8"
- + " -> main",
+ " 3:3:void com.android.tools.r8.naming.retrace.Main$Foo.method1(java.lang.String):8 ->"
+ + " main",
" 3:3:void main(java.lang.String[]):7 -> main");
}
diff --git a/src/test/java/com/android/tools/r8/retrace/stacktraces/OverloadSameLineTest.java b/src/test/java/com/android/tools/r8/retrace/stacktraces/OverloadSameLineTest.java
index cacbe03..d1659cd 100644
--- a/src/test/java/com/android/tools/r8/retrace/stacktraces/OverloadSameLineTest.java
+++ b/src/test/java/com/android/tools/r8/retrace/stacktraces/OverloadSameLineTest.java
@@ -32,8 +32,8 @@
"Exception in thread \"main\" java.lang.NullPointerException",
// TODO(b/199058242): Should be ambiguous and not inline frames
"\tat com.android.tools.r8.naming.retrace.Main.overload(Main.java:7)",
- "\tat com.android.tools.r8.naming.retrace.Main.overload(Main.java:13)",
- "\tat com.android.tools.r8.naming.retrace.Main.overload(Main.java:15)");
+ "\t<OR> at com.android.tools.r8.naming.retrace.Main.overload(Main.java:15)",
+ "\t<OR> at com.android.tools.r8.naming.retrace.Main.overload(Main.java:13)");
}
@Override
