Insert nop instructions to be step-out compatible with JVMs
For non-void invokes, the required pop instruction in the classfile
format would ensure that stepping out of such an invoke would return to
the same line as stepping in. In DEX no such instruction follows the
invoke and a nop is inserted to retain the same behavior.
This behavior is default disabled with this CL and setting the property `com.android.tools.r8.enableJvmCompatibleStepOutBehavior` is needed to enable it.
Bug: b/67671565
Change-Id: I85d4382d65f878057a3a2f34ab4dc903ac714dda
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/DexBuilder.java b/src/main/java/com/android/tools/r8/ir/conversion/DexBuilder.java
index 3dadbc0..bdeeb96 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/DexBuilder.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/DexBuilder.java
@@ -50,11 +50,13 @@
import com.android.tools.r8.ir.analysis.type.TypeElement;
import com.android.tools.r8.ir.code.Argument;
import com.android.tools.r8.ir.code.BasicBlock;
+import com.android.tools.r8.ir.code.BasicBlockInstructionListIterator;
import com.android.tools.r8.ir.code.CatchHandlers;
import com.android.tools.r8.ir.code.DebugPosition;
import com.android.tools.r8.ir.code.IRCode;
import com.android.tools.r8.ir.code.IRCode.BasicBlockIteratorCallback;
import com.android.tools.r8.ir.code.If;
+import com.android.tools.r8.ir.code.Instruction;
import com.android.tools.r8.ir.code.InstructionIterator;
import com.android.tools.r8.ir.code.InstructionListIterator;
import com.android.tools.r8.ir.code.IntSwitch;
@@ -223,6 +225,9 @@
// unnecessary nops.
removeRedundantDebugPositions(appView, ir);
+ // Add back debug positions to perserve step-out behavior compatible with pops on JVMs.
+ amendDebugPositionsForUnusedNonVoidMethods();
+
// Reset the state of the builder to start from scratch.
reset();
@@ -573,6 +578,39 @@
}
}
+ private void amendDebugPositionsForUnusedNonVoidMethods() {
+ if (!appView.options().debug || !appView.options().ensureJvmCompatibleStepOutBehavior) {
+ return;
+ }
+ // Insert additional debug positions (materializing as nop) after all method calls that would
+ // require a pop on JVM. See b/340669208 for context.
+ ir.forEachBlockWithPreviousAndNext(
+ (currentBlock, unused, nextBlock) -> {
+ BasicBlockInstructionListIterator it = currentBlock.listIterator(ir);
+ while (it.hasNext()) {
+ Instruction instruction = it.next();
+ if (!instruction.isInvoke()
+ || instruction.getPosition().isNone()
+ || instruction.getPosition().isSyntheticPosition()
+ || instruction.asInvoke().hasReturnTypeVoid(appView.dexItemFactory())
+ || (instruction.hasOutValue() && instruction.outValue().needsRegister())) {
+ continue;
+ }
+ Position invokePosition = instruction.getPosition();
+ Instruction nextInstruction = it.next();
+ if (nextInstruction.isGoto() && nextInstruction.asGoto().getTarget() == nextBlock) {
+ nextInstruction = nextBlock.entry();
+ }
+ if (!invokePosition.equals(nextInstruction.getPosition())) {
+ it.previous();
+ DebugPosition debugPosition = new DebugPosition();
+ debugPosition.setPosition(invokePosition);
+ it.add(debugPosition);
+ }
+ }
+ });
+ }
+
// Rewrite ifs with offsets that are too large for the if encoding. The rewriting transforms:
//
//
diff --git a/src/main/java/com/android/tools/r8/utils/InternalOptions.java b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
index d89369a..b242562 100644
--- a/src/main/java/com/android/tools/r8/utils/InternalOptions.java
+++ b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
@@ -366,6 +366,9 @@
// To print memory one also have to enable printtimes.
public boolean printMemory = System.getProperty("com.android.tools.r8.printmemory") != null;
+ public boolean ensureJvmCompatibleStepOutBehavior =
+ System.getProperty("com.android.tools.r8.enableJvmCompatibleStepOutBehavior") != null;
+
// Flag to toggle if DEX code objects should pass-through without IR processing.
public boolean passthroughDexCode = false;
diff --git a/src/test/java/com/android/tools/r8/debug/StepOutOfMethodWithUnusedReturnValueTest.java b/src/test/java/com/android/tools/r8/debug/StepOutOfMethodWithUnusedReturnValueTest.java
new file mode 100644
index 0000000..5085d1b
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/debug/StepOutOfMethodWithUnusedReturnValueTest.java
@@ -0,0 +1,22 @@
+// Copyright (c) 2024, 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.debug;
+
+public class StepOutOfMethodWithUnusedReturnValueTest {
+
+ public static int methodWithReturnValue() {
+ return System.nanoTime() > 0 ? 42 : -1;
+ }
+
+ public static void methodWithoutReturnValue() {
+ System.nanoTime();
+ }
+
+ public static void main(String[] args) {
+ methodWithReturnValue();
+ methodWithoutReturnValue();
+ methodWithReturnValue(); // another call to have a consistent continuation for two the calls.
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/debug/StepOutOfMethodWithUnusedReturnValueTestRunner.java b/src/test/java/com/android/tools/r8/debug/StepOutOfMethodWithUnusedReturnValueTestRunner.java
new file mode 100644
index 0000000..a6663e7
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/debug/StepOutOfMethodWithUnusedReturnValueTestRunner.java
@@ -0,0 +1,71 @@
+// Copyright (c) 2024, 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.debug;
+
+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.utils.AndroidApiLevel;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameter;
+import org.junit.runners.Parameterized.Parameters;
+
+@RunWith(Parameterized.class)
+public class StepOutOfMethodWithUnusedReturnValueTestRunner extends DebugTestBase {
+
+ private static final Class<?> CLASS = StepOutOfMethodWithUnusedReturnValueTest.class;
+ private static final String FILE = CLASS.getSimpleName() + ".java";
+ private static final String NAME = CLASS.getCanonicalName();
+
+ @Parameter public TestParameters parameters;
+
+ @Parameters(name = "{0}")
+ public static TestParametersCollection data() {
+ return TestParameters.builder()
+ .withDefaultCfRuntime()
+ .withDexRuntimesStartingFromIncluding(Version.V5_1_1)
+ .withApiLevel(AndroidApiLevel.B)
+ .enableApiLevelsForCf()
+ .build();
+ }
+
+ @Test
+ public void testHitOnEntryOnly() throws Throwable {
+ DebugTestConfig config =
+ testForD8(parameters.getBackend())
+ .setMinApi(parameters)
+ .addProgramClasses(CLASS)
+ .addOptionsModification(o -> o.ensureJvmCompatibleStepOutBehavior = true)
+ .debugConfig(parameters.getRuntime());
+
+ runDebugTest(
+ config,
+ CLASS,
+ breakpoint(NAME, "main"),
+ run(),
+ checkLine(FILE, 18),
+ stepInto(),
+ checkLine(FILE, 10),
+ stepOut(),
+ checkLine(FILE, 18),
+ stepOver(),
+ checkLine(FILE, 19),
+ stepInto(),
+ checkLine(FILE, 14),
+ stepOut(),
+ // Notice that stepping out of the void method will go to next line.
+ checkLine(FILE, 20),
+ stepInto(),
+ checkLine(FILE, 10),
+ stepOut(),
+ // Notice also that we step to the same line even when the continuation is a 'return' that
+ // would not need to pop the value.
+ checkLine(FILE, 20),
+ stepInto(),
+ checkLine(FILE, 21),
+ run());
+ }
+}
