Update LineNumberOptimizer to always insert positions for overloads
Bug: b/221015863
Bug: b/251677184
Change-Id: Ie9268c22589d32f71a223cc28bcdf7eef9e13b6f
diff --git a/src/main/java/com/android/tools/r8/utils/positions/ClassFilePositionToMappedRangeMapper.java b/src/main/java/com/android/tools/r8/utils/positions/ClassFilePositionToMappedRangeMapper.java
index cd6efb7..dd60a3e 100644
--- a/src/main/java/com/android/tools/r8/utils/positions/ClassFilePositionToMappedRangeMapper.java
+++ b/src/main/java/com/android/tools/r8/utils/positions/ClassFilePositionToMappedRangeMapper.java
@@ -11,8 +11,10 @@
import com.android.tools.r8.cf.code.CfPosition;
import com.android.tools.r8.graph.AppView;
import com.android.tools.r8.graph.CfCode;
+import com.android.tools.r8.graph.DexMethod;
import com.android.tools.r8.graph.ProgramMethod;
import com.android.tools.r8.ir.code.Position;
+import com.android.tools.r8.ir.code.Position.SyntheticPosition;
import com.android.tools.r8.utils.Pair;
import java.util.ArrayList;
import java.util.List;
@@ -34,7 +36,7 @@
int pcEncodingCutoff) {
return appView.options().getTestingOptions().usePcEncodingInCfForTesting
? getPcEncodedPositions(method, positionRemapper)
- : getMappedPositionsRemapped(method, positionRemapper);
+ : getMappedPositionsRemapped(method, positionRemapper, hasOverloads);
}
@Override
@@ -43,15 +45,17 @@
}
private List<MappedPosition> getMappedPositionsRemapped(
- ProgramMethod method, PositionRemapper positionRemapper) {
+ ProgramMethod method, PositionRemapper positionRemapper, boolean hasOverloads) {
List<MappedPosition> mappedPositions = new ArrayList<>();
// Do the actual processing for each method.
CfCode oldCode = method.getDefinition().getCode().asCfCode();
List<CfInstruction> oldInstructions = oldCode.getInstructions();
List<CfInstruction> newInstructions = new ArrayList<>(oldInstructions.size());
+ boolean seenPosition = false;
for (CfInstruction oldInstruction : oldInstructions) {
CfInstruction newInstruction;
if (oldInstruction.isPosition()) {
+ seenPosition = true;
CfPosition cfPosition = oldInstruction.asPosition();
newInstruction =
new CfPosition(
@@ -62,6 +66,24 @@
}
newInstructions.add(newInstruction);
}
+ if (!seenPosition && hasOverloads) {
+ // If a method with overloads does not have an actual position then map it to the implicit
+ // preamble position.
+ DexMethod reference = method.getReference();
+ DexMethod original = appView.graphLens().getOriginalMethodSignature(reference);
+ CfPosition preamblePositionForOverload =
+ new CfPosition(
+ new CfLabel(),
+ remapAndAdd(
+ SyntheticPosition.builder().setMethod(original).setLine(0).build(),
+ positionRemapper,
+ mappedPositions));
+ List<CfInstruction> shiftedPositions = new ArrayList<>(oldInstructions.size() + 2);
+ shiftedPositions.add(preamblePositionForOverload);
+ shiftedPositions.add(preamblePositionForOverload.getLabel());
+ shiftedPositions.addAll(newInstructions);
+ newInstructions = shiftedPositions;
+ }
method.setCode(
new CfCode(
method.getHolderType(),
diff --git a/src/main/java/com/android/tools/r8/utils/positions/LineNumberOptimizer.java b/src/main/java/com/android/tools/r8/utils/positions/LineNumberOptimizer.java
index 993ca5f..c1e6850 100644
--- a/src/main/java/com/android/tools/r8/utils/positions/LineNumberOptimizer.java
+++ b/src/main/java/com/android/tools/r8/utils/positions/LineNumberOptimizer.java
@@ -118,13 +118,21 @@
for (ProgramMethod method : methods) {
DexEncodedMethod definition = method.getDefinition();
+ DexMethod methodReference = method.getReference();
+ if (methodName == method.getName()
+ && appView.graphLens().getOriginalMethodSignature(methodReference) == methodReference
+ && !mustHaveResidualDebugInfo(appView.options(), definition)
+ && !definition.isD8R8Synthesized()
+ && methods.size() <= 1) {
+ continue;
+ }
positionRemapper.setCurrentMethod(definition);
List<MappedPosition> mappedPositions;
int pcEncodingCutoff =
methods.size() == 1 ? representation.getDexPcEncodingCutoff(method) : -1;
boolean canUseDexPc = pcEncodingCutoff > 0;
if (definition.getCode() != null
- && mustHaveResidualDebugInfo(appView.options(), definition)
+ && (definition.getCode().isCfCode() || definition.getCode().isDexCode())
&& !appView.isCfByteCodePassThrough(definition)) {
mappedPositions =
positionToMappedRangeMapper.getMappedPositions(
@@ -229,14 +237,9 @@
DexEncodedMethod definition = programMethod.getDefinition();
DexMethod method = programMethod.getReference();
DexString renamedName = appView.getNamingLens().lookupName(method);
- if (renamedName != method.name
- || appView.graphLens().getOriginalMethodSignature(method) != method
- || mustHaveResidualDebugInfo(appView.options(), definition)
- || definition.isD8R8Synthesized()) {
- methodsByRenamedName
- .computeIfAbsent(renamedName, key -> new ArrayList<>())
- .add(programMethod);
- }
+ methodsByRenamedName
+ .computeIfAbsent(renamedName, key -> new ArrayList<>())
+ .add(programMethod);
}
return methodsByRenamedName;
}
diff --git a/src/test/examples/shaking1/print-mapping-dex.ref b/src/test/examples/shaking1/print-mapping-dex.ref
index 3635115..bb72965 100644
--- a/src/test/examples/shaking1/print-mapping-dex.ref
+++ b/src/test/examples/shaking1/print-mapping-dex.ref
@@ -1,6 +1,7 @@
shaking1.Shaking -> shaking1.Shaking:
shaking1.Used -> a.a:
0:2:void <init>(java.lang.String):12:12 -> <init>
+ 0:2:java.lang.String aMethodThatIsNotUsedButKept():0:0 -> aMethodThatIsNotUsedButKept
3:5:void <init>(java.lang.String):13:13 -> <init>
0:6:void main(java.lang.String[]):8:8 -> main
7:21:void main(java.lang.String[]):9:9 -> main
diff --git a/src/test/java/com/android/tools/r8/debuginfo/OverloadWithNoLineNumberTest.java b/src/test/java/com/android/tools/r8/debuginfo/OverloadWithNoLineNumberTest.java
index 47f6e4b..7a519a6 100644
--- a/src/test/java/com/android/tools/r8/debuginfo/OverloadWithNoLineNumberTest.java
+++ b/src/test/java/com/android/tools/r8/debuginfo/OverloadWithNoLineNumberTest.java
@@ -4,17 +4,20 @@
package com.android.tools.r8.debuginfo;
-import static com.android.tools.r8.naming.retrace.StackTrace.isSame;
-import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
+import com.android.tools.r8.R8TestRunResult;
import com.android.tools.r8.TestBase;
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.TestParametersCollection;
-import com.android.tools.r8.ToolHelper.DexVm.Version;
import com.android.tools.r8.debuginfo.testclasses.SimpleCallChainClassWithOverloads;
-import com.android.tools.r8.naming.retrace.StackTrace;
-import com.android.tools.r8.naming.retrace.StackTrace.StackTraceLine;
+import com.android.tools.r8.retrace.ProguardMapProducer;
+import com.android.tools.r8.retrace.ProguardMappingSupplier;
+import com.android.tools.r8.retrace.Retrace;
+import com.android.tools.r8.retrace.RetraceCommand;
import com.android.tools.r8.transformers.ClassFileTransformer.MethodPredicate;
+import com.android.tools.r8.utils.StringUtils;
+import java.util.stream.Collectors;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
@@ -35,68 +38,43 @@
@Test
public void testR8() throws Exception {
- testForR8(parameters.getBackend())
- .addProgramClassFileData(
- transformer(SimpleCallChainClassWithOverloads.class)
- .removeLineNumberTable(MethodPredicate.onName("test"))
- .transform())
- .setMinApi(parameters.getApiLevel())
- .addKeepMainRule(SimpleCallChainClassWithOverloads.class)
- .addKeepClassAndMembersRules(SimpleCallChainClassWithOverloads.class)
- .addKeepAttributeLineNumberTable()
- .run(parameters.getRuntime(), SimpleCallChainClassWithOverloads.class)
- .assertFailureWithErrorThatThrows(RuntimeException.class)
- .inspectStackTrace(
- (stackTrace, inspector) -> {
- StackTraceLine mainLine =
- StackTraceLine.builder()
- .setClassName(typeName(SimpleCallChainClassWithOverloads.class))
- .setMethodName("main")
- .setFileName(SOURCE_FILE_NAME)
- .setLineNumber(10)
- .build();
- if (parameters.isCfRuntime()
- || parameters.getDexRuntimeVersion().isOlderThan(Version.V8_1_0)) {
- StackTraceLine testStackTraceLine =
- StackTraceLine.builder()
- .setClassName(typeName(SimpleCallChainClassWithOverloads.class))
- .setMethodName("test")
- .setFileName(SOURCE_FILE_NAME)
- .build();
- assertThat(
- stackTrace,
- isSame(
- StackTrace.builder()
- // TODO(b/251677184): Stack trace lines should still be distinguishable
- // even if there are no original line numbers to map back two.
- .add(testStackTraceLine)
- .add(testStackTraceLine)
- .add(mainLine)
- .build()));
- } else {
- assertThat(
- stackTrace,
- isSame(
- StackTrace.builder()
- // TODO(b/251677184): Strange that we are able to distinguish when using
- // pc mapping.
- .add(
- StackTraceLine.builder()
- .setClassName(typeName(SimpleCallChainClassWithOverloads.class))
- .setMethodName("test")
- .setFileName(SOURCE_FILE_NAME)
- .setLineNumber(11)
- .build())
- .add(
- StackTraceLine.builder()
- .setClassName(typeName(SimpleCallChainClassWithOverloads.class))
- .setMethodName("test")
- .setFileName(SOURCE_FILE_NAME)
- .setLineNumber(4)
- .build())
- .add(mainLine)
- .build()));
- }
- });
+ R8TestRunResult result =
+ testForR8(parameters.getBackend())
+ .addProgramClassFileData(
+ transformer(SimpleCallChainClassWithOverloads.class)
+ .removeLineNumberTable(MethodPredicate.onName("test"))
+ .transform())
+ .setMinApi(parameters.getApiLevel())
+ .addKeepMainRule(SimpleCallChainClassWithOverloads.class)
+ .addKeepClassAndMembersRules(SimpleCallChainClassWithOverloads.class)
+ .addKeepAttributeLineNumberTable()
+ .run(parameters.getRuntime(), SimpleCallChainClassWithOverloads.class)
+ .assertFailureWithErrorThatThrows(RuntimeException.class);
+ Retrace.run(
+ RetraceCommand.builder()
+ .setMappingSupplier(
+ ProguardMappingSupplier.builder()
+ .setProguardMapProducer(ProguardMapProducer.fromString(result.proguardMap()))
+ .build())
+ .setStackTrace(
+ result.getOriginalStackTrace().getStackTraceLines().stream()
+ .map(line -> line.originalLine)
+ .collect(Collectors.toList()))
+ .setRetracedStackTraceConsumer(
+ retraced -> {
+ String className = typeName(SimpleCallChainClassWithOverloads.class);
+ assertEquals(
+ StringUtils.joinLines(
+ "\tat " + className + ".void test(long)(" + SOURCE_FILE_NAME + ":0)",
+ "\tat " + className + ".void test()(" + SOURCE_FILE_NAME + ":0)",
+ "\tat "
+ + className
+ + ".void main(java.lang.String[])("
+ + SOURCE_FILE_NAME
+ + ":10)"),
+ StringUtils.joinLines(retraced));
+ })
+ .setVerbose(true)
+ .build());
}
}
diff --git a/src/test/java/com/android/tools/r8/naming/retrace/VerticalClassMergingRetraceTest.java b/src/test/java/com/android/tools/r8/naming/retrace/VerticalClassMergingRetraceTest.java
index fc1e432..9e361e6 100644
--- a/src/test/java/com/android/tools/r8/naming/retrace/VerticalClassMergingRetraceTest.java
+++ b/src/test/java/com/android/tools/r8/naming/retrace/VerticalClassMergingRetraceTest.java
@@ -130,7 +130,6 @@
// at com.android.tools.r8.naming.retraceproguard.MainApp.main(MainApp.java:7)
//
// We should instead translate to:
- // at com.android.tools.r8.naming.retraceproguard.ResourceWrapper.foo(ResourceWrapper.java:1)
// at com.android.tools.r8.naming.retraceproguard.ResourceWrapper.foo(ResourceWrapper.java:0)
// at com.android.tools.r8.naming.retraceproguard.MainApp.main(MainApp.java:7)
// since the synthetic bridge belongs to ResourceWrapper.foo.
@@ -140,12 +139,7 @@
runTest(
ImmutableList.of(),
(StackTrace actualStackTrace, StackTrace retracedStackTrace) -> {
- StackTrace reprocessedStackTrace =
- mode == CompilationMode.DEBUG
- ? retracedStackTrace
- : retracedStackTrace.filter(this::filterSynthesizedBridgeMethod);
- assertThat(
- reprocessedStackTrace, isSameExceptForFileNameAndLineNumber(expectedStackTrace));
+ assertThat(retracedStackTrace, isSameExceptForFileNameAndLineNumber(expectedStackTrace));
assertEquals(
expectedActualStackTraceHeight(), actualStackTrace.getStackTraceLines().size());
});
diff --git a/src/test/java/com/android/tools/r8/retrace/OverloadsWithoutLineNumberTest.java b/src/test/java/com/android/tools/r8/retrace/OverloadsWithoutLineNumberTest.java
index 3aa2f49..2ba06ba 100644
--- a/src/test/java/com/android/tools/r8/retrace/OverloadsWithoutLineNumberTest.java
+++ b/src/test/java/com/android/tools/r8/retrace/OverloadsWithoutLineNumberTest.java
@@ -63,21 +63,11 @@
.setRetracedStackTraceConsumer(box::set)
.setVerbose(true);
Retrace.run(builder.build());
- // TODO(b/221015863): This should ideally be:
- // at " + typeName(ClassWithOverload.class) + ".test(int)(OverloadsWithoutLineNumberTest.java)
- if (parameters.canUseNativeDexPC()) {
- assertEquals(
- "\tat "
- + typeName(ClassWithOverload.class)
- + ".test(OverloadsWithoutLineNumberTest.java:0)",
- box.get().get(1));
- } else {
- assertEquals(
- "\tat "
- + typeName(ClassWithOverload.class)
- + ".test(OverloadsWithoutLineNumberTest.java)",
- box.get().get(1));
- }
+ assertEquals(
+ "\tat "
+ + typeName(ClassWithOverload.class)
+ + ".void test(int)(OverloadsWithoutLineNumberTest.java:0)",
+ box.get().get(1));
}
public static class ClassWithOverload {
diff --git a/src/test/java/com/android/tools/r8/retrace/RetraceStackTraceFunctionalCompositionTest.java b/src/test/java/com/android/tools/r8/retrace/RetraceStackTraceFunctionalCompositionTest.java
index 9d9d7a4..367e1b6 100644
--- a/src/test/java/com/android/tools/r8/retrace/RetraceStackTraceFunctionalCompositionTest.java
+++ b/src/test/java/com/android/tools/r8/retrace/RetraceStackTraceFunctionalCompositionTest.java
@@ -36,7 +36,6 @@
import java.util.Map;
import java.util.Map.Entry;
import java.util.function.Supplier;
-import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
@@ -157,7 +156,6 @@
return rewrittenR8Jar;
}
- @Ignore("b/251677184: Failing since update to target 11")
@Test
public void testR8RetraceAndComposition() throws Exception {
Path rewrittenR8Jar = getRewrittenR8Jar();
