Fix var hoisting issue when no previous store (#8122)

In some cases the range of a local var may not have previous init
in the immediate neighborhood because of basic block arrangement.
Limit the rewirite of previous instruction for store only and looking
 back only 10 instructions

Author: jpbempel

dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/ASMHelper.java

@@ -340,6 +340,10 @@ public static String extractSuperClass(ClassNode classNode) {
     return Strings.getClassName(classNode.superName);
   }
 
+  public static boolean isStore(int opcode) {
+    return opcode >= Opcodes.ISTORE && opcode <= Opcodes.ASTORE;
+  }
+
   /** Wraps ASM's {@link org.objectweb.asm.Type} with associated generic types */
   public static class Type {
     private final org.objectweb.asm.Type mainType;

dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/CapturedContextInstrumentor.java

@@ -7,6 +7,7 @@
 import static com.datadog.debugger.instrumentation.ASMHelper.invokeVirtual;
 import static com.datadog.debugger.instrumentation.ASMHelper.isFinalField;
 import static com.datadog.debugger.instrumentation.ASMHelper.isStaticField;
+import static com.datadog.debugger.instrumentation.ASMHelper.isStore;
 import static com.datadog.debugger.instrumentation.ASMHelper.ldc;
 import static com.datadog.debugger.instrumentation.ASMHelper.newInstance;
 import static com.datadog.debugger.instrumentation.Types.CAPTURED_CONTEXT_TYPE;
@@ -68,7 +69,7 @@
 import org.slf4j.LoggerFactory;
 
 public class CapturedContextInstrumentor extends Instrumentor {
-  private static final Logger log = LoggerFactory.getLogger(CapturedContextInstrumentor.class);
+  private static final Logger LOGGER = LoggerFactory.getLogger(CapturedContextInstrumentor.class);
   private final boolean captureSnapshot;
   private final Limits limits;
   private final LabelNode contextInitLabel = new LabelNode();
@@ -639,8 +640,8 @@ private void rewriteLocalVarInsn(LocalVariableNode localVar, int oldSlot, int ne
   //   10: astore_1
   //   11: aload_1
   // range for slot 1 starts at 11
-  // javac always starts the range right after the init of the local var, so we can just look for
-  // the previous instruction
+  // javac often starts the range right after the init of the local var, so we can just look for
+  // the previous instruction. But not always, and we put an arbitrary limit to 10 instructions
   // for kotlinc, many instructions can separate the init and the range start
   // ex:
   //    LocalVariableTable:
@@ -656,18 +657,26 @@ private void rewriteLocalVarInsn(LocalVariableNode localVar, int oldSlot, int ne
   private static void rewritePreviousStoreInsn(
       LocalVariableNode localVar, int oldSlot, int newSlot) {
     AbstractInsnNode previous = localVar.start.getPrevious();
-    while (previous != null
-        && (!(previous instanceof VarInsnNode) || ((VarInsnNode) previous).var != oldSlot)) {
+    int processed = 0;
+    // arbitrary fixing limit to 10 previous instructions to look at
+    while (previous != null && !isVarStoreForSlot(previous, oldSlot) && processed < 10) {
       previous = previous.getPrevious();
+      processed++;
     }
-    if (previous != null) {
+    if (isVarStoreForSlot(previous, oldSlot)) {
       VarInsnNode varInsnNode = (VarInsnNode) previous;
       if (varInsnNode.var == oldSlot) {
         varInsnNode.var = newSlot;
       }
     }
   }
 
+  private static boolean isVarStoreForSlot(AbstractInsnNode node, int slotIdx) {
+    return node instanceof VarInsnNode
+        && isStore(node.getOpcode())
+        && ((VarInsnNode) node).var == slotIdx;
+  }
+
   private void createInProbeFinallyHandler(LabelNode inProbeStartLabel, LabelNode inProbeEndLabel) {
     LabelNode handlerLabel = new LabelNode();
     InsnList handler = new InsnList();
@@ -1234,7 +1243,7 @@ private static void addInheritedStaticFields(
             FieldNode fieldNode =
                 new FieldNode(field.getModifiers(), field.getName(), desc, null, field);
             results.add(fieldNode);
-            log.debug("Adding static inherited field {}", fieldNode.name);
+            LOGGER.debug("Adding static inherited field {}", fieldNode.name);
             fieldCount++;
             if (fieldCount > limits.maxFieldCount) {
               return;

dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/CapturedSnapshotTest.java

@@ -66,7 +66,6 @@
 import java.lang.reflect.Modifier;
 import java.net.URISyntaxException;
 import java.net.URL;
-import java.nio.file.Paths;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
@@ -134,6 +133,17 @@ public void methodProbe() throws IOException, URISyntaxException {
     assertEquals("CapturedSnapshot01.main", snapshot.getStack().get(0).getFunction());
   }
 
+  @Test
+  public void localVarHoistingNoPreviousStore() throws IOException, URISyntaxException {
+    final String CLASS_NAME = "com.fasterxml.jackson.core.json.ByteSourceJsonBootstrapper";
+    TestSnapshotListener listener = installSingleProbe(CLASS_NAME, "detectEncoding", null);
+    Class<?> testClass =
+        loadClass(
+            CLASS_NAME,
+            getClass().getResource("/classfiles/ByteSourceJsonBootstrapper.classfile").getFile());
+    assertNotNull(testClass);
+  }
+
   @Test
   public void singleLineProbe() throws IOException, URISyntaxException {
     final String CLASS_NAME = "CapturedSnapshot01";
@@ -219,12 +229,8 @@ public void oldClass1_1() throws Exception {
     when(config.isDebuggerVerifyByteCode()).thenReturn(true);
     Class<?> testClass =
         loadClass(
-            CLASS_NAME,
-            Paths.get(
-                    CapturedSnapshotTest.class
-                        .getResource("/classfiles/BooleanUtils.classfile")
-                        .toURI())
-                .toString());
+            CLASS_NAME, getClass().getResource("/classfiles/BooleanUtils.classfile").getFile());
+
     boolean result = Reflect.onClass(testClass).call("toBoolean", Boolean.TRUE).get();
     assertTrue(result);
     assertOneSnapshot(listener);