diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/CapturedContextInstrumentor.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/CapturedContextInstrumentor.java index 980a4488fa9..09d775683a6 100644 --- a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/CapturedContextInstrumentor.java +++ b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/CapturedContextInstrumentor.java @@ -6,7 +6,6 @@ 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; @@ -40,10 +39,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; -import java.util.HashMap; -import java.util.HashSet; import java.util.List; -import java.util.Map; import java.util.Set; import java.util.stream.Collectors; import org.objectweb.asm.Opcodes; @@ -52,7 +48,6 @@ import org.objectweb.asm.tree.ClassNode; import org.objectweb.asm.tree.FieldInsnNode; import org.objectweb.asm.tree.FieldNode; -import org.objectweb.asm.tree.IincInsnNode; import org.objectweb.asm.tree.InsnList; import org.objectweb.asm.tree.InsnNode; import org.objectweb.asm.tree.JumpInsnNode; @@ -75,7 +70,7 @@ public class CapturedContextInstrumentor extends Instrumentor { private int exitContextVar = -1; private int timestampStartVar = -1; private int throwableListVar = -1; - private Collection unscopedLocalVars; + private Collection hoistedLocalVars = Collections.emptyList(); public CapturedContextInstrumentor( ProbeDefinition definition, @@ -413,12 +408,7 @@ private void instrumentMethodEnter() { if (methodNode.tryCatchBlocks.size() > 0) { throwableListVar = declareThrowableList(insnList); } - unscopedLocalVars = Collections.emptyList(); - if (Config.get().isDynamicInstrumentationHoistLocalVarsEnabled() - && language == JvmLanguage.JAVA) { - // for now, only hoist local vars for Java - unscopedLocalVars = initAndHoistLocalVars(insnList); - } + hoistedLocalVars = initAndHoistLocalVars(methodNode); insnList.add(contextInitLabel); if (definition instanceof SpanDecorationProbe && definition.getEvaluateAt() == MethodLocation.EXIT) { @@ -485,212 +475,18 @@ private void instrumentMethodEnter() { // Initialize and hoist local variables to the top of the method // if there is name/slot conflict, do nothing for the conflicting local variable - private Collection initAndHoistLocalVars(InsnList insnList) { - if (methodNode.localVariables == null || methodNode.localVariables.isEmpty()) { + private Collection initAndHoistLocalVars(MethodNode methodNode) { + int hoistingLevel = Config.get().getDynamicInstrumentationLocalVarHoistingLevel(); + if (hoistingLevel == 0 || language != JvmLanguage.JAVA) { + // for now, only hoist local vars for Java return Collections.emptyList(); } - Map localVarsByName = new HashMap<>(); - Map localVarsBySlot = new HashMap<>(); - Map> hoistableVarByName = new HashMap<>(); - for (LocalVariableNode localVar : methodNode.localVariables) { - int idx = localVar.index - localVarBaseOffset; - if (idx < argOffset) { - // this is an argument - continue; - } - checkHoistableLocalVar(localVar, localVarsByName, localVarsBySlot, hoistableVarByName); - } - removeDuplicatesFromArgs(hoistableVarByName, localVarsBySlotArray); - // hoist vars - List results = new ArrayList<>(); - for (Map.Entry> entry : hoistableVarByName.entrySet()) { - Set hoistableVars = entry.getValue(); - LocalVariableNode newVarNode; - if (hoistableVars.size() > 1) { - // merge variables - LocalVariableNode firstHoistableVar = hoistableVars.iterator().next(); - String name = firstHoistableVar.name; - String desc = firstHoistableVar.desc; - Type localVarType = getType(desc); - int newSlot = newVar(localVarType); // new slot for the local variable - newVarNode = new LocalVariableNode(name, desc, null, null, null, newSlot); - Set endLabels = new HashSet<>(); - for (LocalVariableNode localVar : hoistableVars) { - // rewrite each usage of the old var to the new slot - rewriteLocalVarInsn(localVar, localVar.index, newSlot); - endLabels.add(localVar.end); - } - hoistVar(insnList, newVarNode); - newVarNode.end = findLatestLabel(methodNode.instructions, endLabels); - // remove previous local variables - methodNode.localVariables.removeIf(hoistableVars::contains); - methodNode.localVariables.add(newVarNode); - } else { - // hoist the single variable and rewrite all its local var instructions - newVarNode = hoistableVars.iterator().next(); - int oldIndex = newVarNode.index; - newVarNode.index = newVar(getType(newVarNode.desc)); // new slot for the local variable - rewriteLocalVarInsn(newVarNode, oldIndex, newVarNode.index); - hoistVar(insnList, newVarNode); - } - results.add(newVarNode); - } - return results; - } - - private void removeDuplicatesFromArgs( - Map> hoistableVarByName, - LocalVariableNode[] localVarsBySlotArray) { - for (int idx = 0; idx < argOffset; idx++) { - LocalVariableNode localVar = localVarsBySlotArray[idx]; - if (localVar == null) { - continue; - } - // we are removing local variables that are arguments - hoistableVarByName.remove(localVar.name); - } - } - - private LabelNode findLatestLabel(InsnList instructions, Set endLabels) { - for (AbstractInsnNode insn = instructions.getLast(); insn != null; insn = insn.getPrevious()) { - if (insn instanceof LabelNode && endLabels.contains(insn)) { - return (LabelNode) insn; - } - } - return null; - } - - private void hoistVar(InsnList insnList, LocalVariableNode varNode) { - LabelNode labelNode = new LabelNode(); // new start label for the local variable - insnList.add(labelNode); - varNode.start = labelNode; // update the start label of the local variable - Type localVarType = getType(varNode.desc); - addStore0Insn(insnList, varNode, localVarType); - } - - private static void addStore0Insn( - InsnList insnList, LocalVariableNode localVar, Type localVarType) { - switch (localVarType.getSort()) { - case Type.BOOLEAN: - case Type.CHAR: - case Type.BYTE: - case Type.SHORT: - case Type.INT: - insnList.add(new InsnNode(Opcodes.ICONST_0)); - break; - case Type.LONG: - insnList.add(new InsnNode(Opcodes.LCONST_0)); - break; - case Type.FLOAT: - insnList.add(new InsnNode(Opcodes.FCONST_0)); - break; - case Type.DOUBLE: - insnList.add(new InsnNode(Opcodes.DCONST_0)); - break; - default: - insnList.add(new InsnNode(Opcodes.ACONST_NULL)); - break; - } - insnList.add(new VarInsnNode(localVarType.getOpcode(Opcodes.ISTORE), localVar.index)); - } - - private void checkHoistableLocalVar( - LocalVariableNode localVar, - Map localVarsByName, - Map localVarsBySlot, - Map> hoistableVarByName) { - LocalVariableNode previousVarBySlot = localVarsBySlot.putIfAbsent(localVar.index, localVar); - LocalVariableNode previousVarByName = localVarsByName.putIfAbsent(localVar.name, localVar); - if (previousVarBySlot != null) { - // there are multiple local variables with the same slot but different names - // by hoisting in a new slot, we can avoid the conflict - hoistableVarByName.computeIfAbsent(localVar.name, k -> new HashSet<>()).add(localVar); - } - if (previousVarByName != null) { - // there are multiple local variables with the same name - // checking type to see if they are compatible - Type previousType = getType(previousVarByName.desc); - Type currentType = getType(localVar.desc); - if (!ASMHelper.isStoreCompatibleType(previousType, currentType)) { - reportWarning( - "Local variable " - + localVar.name - + " has multiple definitions with incompatible types: " - + previousVarByName.desc - + " and " - + localVar.desc); - // remove name from hoistable - hoistableVarByName.remove(localVar.name); - return; - } - // Merge variables because compatible type - } - // by default, there is no conflict => hoistable - hoistableVarByName.computeIfAbsent(localVar.name, k -> new HashSet<>()).add(localVar); - } - - private void rewriteLocalVarInsn(LocalVariableNode localVar, int oldSlot, int newSlot) { - rewritePreviousStoreInsn(localVar, oldSlot, newSlot); - for (AbstractInsnNode insn = localVar.start; - insn != null && insn != localVar.end; - insn = insn.getNext()) { - if (insn instanceof VarInsnNode) { - VarInsnNode varInsnNode = (VarInsnNode) insn; - if (varInsnNode.var == oldSlot) { - varInsnNode.var = newSlot; - } - } - if (insn instanceof IincInsnNode) { - IincInsnNode iincInsnNode = (IincInsnNode) insn; - if (iincInsnNode.var == oldSlot) { - iincInsnNode.var = newSlot; - } - } - } - } - - // Previous insn(s) to local var range could be a store to index that need to be rewritten as well - // LocalVariableTable ranges starts after the init of the local var. - // ex: - // 9: iconst_0 - // 10: astore_1 - // 11: aload_1 - // range for slot 1 starts at 11 - // 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: - // Start Length Slot Name Signature - // 70 121 4 $i$f$map I - // - // 64: istore 4 - // 66: aload_2 - // 67: iconst_2 - // 68: iconst_1 - // 69: bastore - // 70: aload_3 - private static void rewritePreviousStoreInsn( - LocalVariableNode localVar, int oldSlot, int newSlot) { - AbstractInsnNode previous = localVar.start.getPrevious(); - 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 (isVarStoreForSlot(previous, oldSlot)) { - VarInsnNode varInsnNode = (VarInsnNode) previous; - if (varInsnNode.var == oldSlot) { - varInsnNode.var = newSlot; - } + if (methodNode.localVariables == null || methodNode.localVariables.isEmpty()) { + return Collections.emptyList(); } - } - - private static boolean isVarStoreForSlot(AbstractInsnNode node, int slotIdx) { - return node instanceof VarInsnNode - && isStore(node.getOpcode()) - && ((VarInsnNode) node).var == slotIdx; + LOGGER.debug( + "Hoisting local variables level={} for method: {}", hoistingLevel, methodNode.name); + return LocalVarHoisting.processMethod(methodNode, hoistingLevel); } private void createInProbeFinallyHandler(LabelNode inProbeStartLabel, LabelNode inProbeEndLabel) { @@ -916,12 +712,10 @@ private void collectLocalVariables(AbstractInsnNode location, InsnList insnList) return; } Collection localVarNodes; - boolean isLocalVarHoistingEnabled = - Config.get().isDynamicInstrumentationHoistLocalVarsEnabled(); - if (definition.isLineProbe() || !isLocalVarHoistingEnabled) { + if (definition.isLineProbe() || hoistedLocalVars.isEmpty()) { localVarNodes = methodNode.localVariables; } else { - localVarNodes = unscopedLocalVars; + localVarNodes = hoistedLocalVars; } List applicableVars = new ArrayList<>(); boolean isLineProbe = definition.isLineProbe(); @@ -929,7 +723,7 @@ private void collectLocalVariables(AbstractInsnNode location, InsnList insnList) int idx = variableNode.index - localVarBaseOffset; if (idx >= argOffset) { // var is local not arg - if (isLineProbe || !isLocalVarHoistingEnabled) { + if (isLineProbe || hoistedLocalVars.isEmpty()) { if (ASMHelper.isInScope(methodNode, variableNode, location)) { applicableVars.add(variableNode); } diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/LocalVarHoisting.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/LocalVarHoisting.java new file mode 100644 index 00000000000..d4701ad14fb --- /dev/null +++ b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/LocalVarHoisting.java @@ -0,0 +1,313 @@ +package com.datadog.debugger.instrumentation; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import org.objectweb.asm.Opcodes; +import org.objectweb.asm.Type; +import org.objectweb.asm.tree.AbstractInsnNode; +import org.objectweb.asm.tree.InsnList; +import org.objectweb.asm.tree.InsnNode; +import org.objectweb.asm.tree.LabelNode; +import org.objectweb.asm.tree.LocalVariableNode; +import org.objectweb.asm.tree.MethodNode; +import org.objectweb.asm.tree.VarInsnNode; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class LocalVarHoisting { + private static final Logger LOGGER = LoggerFactory.getLogger(LocalVarHoisting.class); + + /** + * Hoisting strategy interface to define how local variables should be hoisted. Implementations + * can provide different hoisting strategies, such as safe or aggressive hoisting. + */ + private interface HoistingStrategy { + void hoist( + MethodNode method, + LocalVariableNode varNode, + Set forbiddenSlots, + Map countBySlot, + Map countByName, + SlotInfo slotInfo, + LabelNode methodEnterLabel, + LabelNode methodEndLabel, + Collection hoisted); + } + + public static Collection processMethod(MethodNode method, int hoistingLevel) { + Map slots = new HashMap<>(); + Set forbiddenSlots = new HashSet<>(); + scanInstructions(method.instructions, slots, forbiddenSlots); + Map countBySlot = new HashMap<>(); + Map countByName = new HashMap<>(); + scanLocalVariableTable(method.localVariables, countBySlot, countByName); + LabelNode methodEnterLabel = new LabelNode(); + method.instructions.insert(methodEnterLabel); + LabelNode methodEndLabel = new LabelNode(); + method.instructions.add(methodEndLabel); + Collection hoisted = new ArrayList<>(); + HoistingStrategy hoistingStrategy = getHoistingStrategy(hoistingLevel); + for (LocalVariableNode varNode : method.localVariables) { + if (isParameter(method, varNode.index)) { + // Skip parameters, they are not hoistable + // LOGGER.debug("Variable: {} at index: {} is a parameter and cannot be hoisted.", + // varNode.name, varNode.index); + continue; + } + SlotInfo slotInfo = slots.get(varNode.index); + if (slotInfo == null) { + // If the slot is not defined, we can skip it + LOGGER.debug("Variable: {} at index: {} has no slot info.", varNode.name, varNode.index); + continue; + } + hoistingStrategy.hoist( + method, + varNode, + forbiddenSlots, + countBySlot, + countByName, + slotInfo, + methodEnterLabel, + methodEndLabel, + hoisted); + } + return hoisted; + } + + private static HoistingStrategy getHoistingStrategy(int hoistingLevel) { + switch (hoistingLevel) { + case 0: + // No hoisting, return a no-op strategy + return LocalVarHoisting::noopHoisting; + case 1: + return LocalVarHoisting::safeHoisting; + case 2: + return LocalVarHoisting::aggressiveHoisting; + default: + LOGGER.warn("Unknown hoisting level: {}", hoistingLevel); + return LocalVarHoisting::noopHoisting; + } + } + + private static void noopHoisting( + MethodNode method, + LocalVariableNode varNode, + Set forbiddenSlots, + Map countBySlot, + Map countByName, + SlotInfo slotInfo, + LabelNode methodEnterLabel, + LabelNode methodEndLabel, + Collection hoisted) {} + + private static void safeHoisting( + MethodNode method, + LocalVariableNode varNode, + Set forbiddenSlots, + Map countBySlot, + Map countByName, + SlotInfo slotInfo, + LabelNode methodEnterLabel, + LabelNode methodEndLabel, + Collection hoisted) { + if (forbiddenSlots.contains(varNode.index)) { + // If the slot is forbidden, we can skip it + LOGGER.debug( + "Variable: {} at index: {} is in a forbidden slot.", varNode.name, varNode.index); + return; + } + int countSlot = countBySlot.get(varNode.index); + int countName = countByName.get(varNode.name); + boolean isOnlyOneType = slotInfo.isOnlyOneType(); + boolean isSingleSlotType = slotInfo.isSingleSlotType(); + if (countSlot == 1 && countName == 1 && isOnlyOneType && isSingleSlotType) { + // one local variable for this slot defined only for one type => safely hoistable + LOGGER.debug("Variable: {} at index: {} is safely hoistable.", varNode.name, varNode.index); + extendRange(varNode, methodEnterLabel, methodEndLabel); + InsnList init = new InsnList(); + addStore0Insn(init, varNode, Type.getType(varNode.desc)); + method.instructions.insert(methodEnterLabel, init); + hoisted.add(varNode); + } else { + // not safely hoistable, we can still try aggressive hoisting + LOGGER.debug( + "Variable: {} at index: {} is not safely hoistable, resons: " + + "countSlot={}, countName={}, isOnlyOneType={}, isSingleSlotType={}", + varNode.name, + varNode.index, + countSlot, + countName, + isOnlyOneType, + isSingleSlotType); + } + } + + private static void aggressiveHoisting( + MethodNode method, + LocalVariableNode varNode, + Set forbiddenSlots, + Map countBySlot, + Map countByName, + SlotInfo slotInfo, + LabelNode methodEnterLabel, + LabelNode methodEndLabel, + Collection hoisted) { + throw new RuntimeException("Aggressive hoisting not implemented yet."); + } + + private static boolean isParameter(MethodNode method, int slot) { + return slot < getParameterSlotCount(method); + } + + private static int getParameterSlotCount(MethodNode method) { + Type[] argTypes = Type.getArgumentTypes(method.desc); + int count = 0; + if ((method.access & Opcodes.ACC_STATIC) == 0) { + count = 1; // 'this' parameter + } + for (Type type : argTypes) { + count += type.getSize(); + } + return count; + } + + private static void addStore0Insn( + InsnList insnList, LocalVariableNode localVar, Type localVarType) { + switch (localVarType.getSort()) { + case Type.BOOLEAN: + case Type.CHAR: + case Type.BYTE: + case Type.SHORT: + case Type.INT: + insnList.add(new InsnNode(Opcodes.ICONST_0)); + break; + case Type.LONG: + insnList.add(new InsnNode(Opcodes.LCONST_0)); + break; + case Type.FLOAT: + insnList.add(new InsnNode(Opcodes.FCONST_0)); + break; + case Type.DOUBLE: + insnList.add(new InsnNode(Opcodes.DCONST_0)); + break; + default: + insnList.add(new InsnNode(Opcodes.ACONST_NULL)); + break; + } + insnList.add(new VarInsnNode(localVarType.getOpcode(Opcodes.ISTORE), localVar.index)); + } + + private static void extendRange(LocalVariableNode varNode, LabelNode first, LabelNode last) { + varNode.start = first; // Set the start of the variable to the first instruction + varNode.end = last; // Set the end of the variable to the last instruction + } + + private static void scanLocalVariableTable( + List localVariables, + Map countBySlot, + Map countByName) { + if (localVariables == null) { + return; // No local variables to process + } + for (LocalVariableNode varNode : localVariables) { + int slot = varNode.index; + countBySlot.compute(slot, (k, v) -> (v == null) ? 1 : v + 1); + countByName.compute(varNode.name, (k, v) -> (v == null) ? 1 : v + 1); + } + } + + private static void scanInstructions( + InsnList instructions, Map slots, Set forbiddenSlots) { + for (int i = 0; i < instructions.size(); i++) { + AbstractInsnNode insn = instructions.get(i); + if (insn instanceof VarInsnNode) { + VarInsnNode varInsn = (VarInsnNode) insn; + int slot = varInsn.var; + SlotInfo slotInfo = slots.computeIfAbsent(slot, k -> new SlotInfo(slot)); + if (isStoreOperation(varInsn.getOpcode())) { + // define var + int opCode = varInsn.getOpcode(); + slotInfo.addDefinition(i, opCode); + if (opCode == Opcodes.LSTORE || opCode == Opcodes.DSTORE) { + // Long and Double occupy two slots, so we need to skip the next slot + // to avoid double counting + if (forbiddenSlots != null) { + forbiddenSlots.add(slot + 1); + } + } + } + if (isLoadOperation(varInsn.getOpcode())) { + // use var + slotInfo.addUse(i, varInsn.getOpcode()); + // assume forbidden slots are already fill with definitions + } + } + } + } + + private static boolean isLoadOperation(int opcode) { + return opcode >= Opcodes.ILOAD && opcode <= Opcodes.ALOAD; + } + + private static boolean isStoreOperation(int opcode) { + return opcode >= Opcodes.ISTORE && opcode <= Opcodes.ASTORE; + } + + private static class SlotInfo { + final int slot; + final List definitions = new ArrayList<>(); + final List uses = new ArrayList<>(); + + SlotInfo(int slot) { + this.slot = slot; + } + + void addDefinition(int index, int type) { + definitions.add(new Definition(index, type)); + } + + void addUse(int index, int type) { + uses.add(new Use(index, type)); + } + + public int getDefinitionCount() { + return definitions.size(); + } + + public boolean isOnlyOneType() { + return definitions.stream().map(d -> d.type).distinct().count() == 1; + } + + public boolean isSingleSlotType() { + return definitions.stream() + .map(d -> d.type) + .distinct() + .allMatch(type -> type != Opcodes.DSTORE && type != Opcodes.LSTORE); + } + } + + private static class Definition { + final int index; // ASM instruction index + final int type; // type of the variable based on OpCode (e.g., ISORE, ASTORe, etc.) + + Definition(int index, int type) { + this.index = index; + this.type = type; + } + } + + private static class Use { + final int index; // ASM instruction index + final int type; // type of the variable based on OpCode (e.g., ILOAD, ALOAD, etc.) + + Use(int index, int type) { + this.index = index; + this.type = type; + } + } +} diff --git a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/CapturedSnapshotTest.java b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/CapturedSnapshotTest.java index 3ff29998125..4a953082f45 100644 --- a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/CapturedSnapshotTest.java +++ b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/CapturedSnapshotTest.java @@ -1866,38 +1866,37 @@ public void evaluateAtExitFalse() throws IOException, URISyntaxException { value = "datadog.trace.api.Platform#isJ9", disabledReason = "we cannot get local variable debug info") public void uncaughtExceptionConditionLocalVar() throws IOException, URISyntaxException { - setFieldInConfig(Config.get(), "dynamicInstrumentationHoistLocalVarsEnabled", true); + if (Config.get().getDynamicInstrumentationLocalVarHoistingLevel() < 2) { + // this test requires local var hoisting level 2 (aggressive) + // because we need to hoist long local variables (2 slots) + return; + } + final String CLASS_NAME = "CapturedSnapshot05"; + LogProbe probe = + createProbeBuilder(PROBE_ID, CLASS_NAME, "main", "(String)") + .when( + new ProbeCondition(DSL.when(DSL.ge(DSL.ref("after"), DSL.value(0))), "after >= 0")) + .evaluateAt(MethodLocation.EXIT) + .build(); + TestSnapshotListener listener = installProbes(probe); + Class testClass = compileAndLoadClass(CLASS_NAME); try { - final String CLASS_NAME = "CapturedSnapshot05"; - LogProbe probe = - createProbeBuilder(PROBE_ID, CLASS_NAME, "main", "(String)") - .when( - new ProbeCondition( - DSL.when(DSL.ge(DSL.ref("after"), DSL.value(0))), "after >= 0")) - .evaluateAt(MethodLocation.EXIT) - .build(); - TestSnapshotListener listener = installProbes(probe); - Class testClass = compileAndLoadClass(CLASS_NAME); - try { - Reflect.onClass(testClass).call("main", "triggerUncaughtException").get(); - Assertions.fail("should not reach this code"); - } catch (ReflectException ex) { - assertEquals("oops", ex.getCause().getCause().getMessage()); - } - Snapshot snapshot = assertOneSnapshot(listener); - assertCaptureThrowable( - snapshot.getCaptures().getReturn(), - "CapturedSnapshot05$CustomException", - "oops", - "CapturedSnapshot05.triggerUncaughtException", - 8); - assertNull(snapshot.getEvaluationErrors()); - // after is 0 because the exception is thrown before the assignment and local var initialized - // at the beginning of the method by instrumentation - assertCaptureLocals(snapshot.getCaptures().getReturn(), "after", "long", "0"); - } finally { - setFieldInConfig(Config.get(), "dynamicInstrumentationHoistLocalVarsEnabled", false); + Reflect.onClass(testClass).call("main", "triggerUncaughtException").get(); + Assertions.fail("should not reach this code"); + } catch (ReflectException ex) { + assertEquals("oops", ex.getCause().getCause().getMessage()); } + Snapshot snapshot = assertOneSnapshot(listener); + assertCaptureThrowable( + snapshot.getCaptures().getReturn(), + "CapturedSnapshot05$CustomException", + "oops", + "CapturedSnapshot05.triggerUncaughtException", + 8); + assertNull(snapshot.getEvaluationErrors()); + // after is 0 because the exception is thrown before the assignment and local var initialized + // at the beginning of the method by instrumentation + assertCaptureLocals(snapshot.getCaptures().getReturn(), "after", "long", "0"); } @Test @@ -1921,7 +1920,7 @@ public void uncaughtExceptionCaptureLocalVars() throws IOException, URISyntaxExc "java.lang.RuntimeException", "oops", "com.datadog.debugger.CapturedSnapshot31.uncaughtException", - 30); + 51); } @Test @@ -1929,20 +1928,16 @@ public void uncaughtExceptionCaptureLocalVars() throws IOException, URISyntaxExc value = "datadog.trace.api.Platform#isJ9", disabledReason = "we cannot get local variable debug info") public void methodProbeLocalVarsLocalScopes() throws IOException, URISyntaxException { - setFieldInConfig(Config.get(), "dynamicInstrumentationHoistLocalVarsEnabled", true); - try { - final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot31"; - LogProbe probe = createMethodProbeAtExit(PROBE_ID, CLASS_NAME, "localScopes", "(String)"); - TestSnapshotListener listener = installProbes(probe); - Class testClass = compileAndLoadClass(CLASS_NAME); - int result = Reflect.onClass(testClass).call("main", "localScopes").get(); - assertEquals(42, result); - Snapshot snapshot = assertOneSnapshot(listener); - assertEquals(1, snapshot.getCaptures().getReturn().getLocals().size()); - assertCaptureLocals(snapshot.getCaptures().getReturn(), "@return", "int", "42"); - } finally { - setFieldInConfig(Config.get(), "dynamicInstrumentationHoistLocalVarsEnabled", false); - } + final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot31"; + LogProbe probe = createMethodProbeAtExit(PROBE_ID, CLASS_NAME, "localScopes", "(String)"); + TestSnapshotListener listener = installProbes(probe); + Class testClass = compileAndLoadClass(CLASS_NAME); + int result = Reflect.onClass(testClass).call("main", "localScopes").get(); + assertEquals(42, result); + Snapshot snapshot = assertOneSnapshot(listener); + assertEquals(2, snapshot.getCaptures().getReturn().getLocals().size()); + assertCaptureLocals(snapshot.getCaptures().getReturn(), "@return", "int", "42"); + assertCaptureLocals(snapshot.getCaptures().getReturn(), "localVar", "int", "42"); } @Test @@ -1969,12 +1964,12 @@ public void methodProbeLocalVarsDeepScopes() throws IOException, URISyntaxExcept // 0 79 0 this Lcom/datadog/debugger/CapturedSnapshot31; // 0 79 1 arg Ljava/lang/String; // 2 77 2 localVarL0 I - assertEquals(6, snapshot.getCaptures().getReturn().getLocals().size()); + // localVarL4 cannot be hoisted in safe mode because of the same name and slot + assertEquals(5, snapshot.getCaptures().getReturn().getLocals().size()); assertCaptureLocals(snapshot.getCaptures().getReturn(), "localVarL0", "int", "0"); assertCaptureLocals(snapshot.getCaptures().getReturn(), "localVarL1", "int", "1"); assertCaptureLocals(snapshot.getCaptures().getReturn(), "localVarL2", "int", "2"); assertCaptureLocals(snapshot.getCaptures().getReturn(), "localVarL3", "int", "3"); - assertCaptureLocals(snapshot.getCaptures().getReturn(), "localVarL4", "int", "4"); } @Test @@ -2033,21 +2028,120 @@ public void overlappingLocalVarSlot() throws IOException, URISyntaxException { value = "datadog.trace.api.Platform#isJ9", disabledReason = "we cannot get local variable debug info") public void duplicateLocalDifferentScope() throws IOException, URISyntaxException { - setFieldInConfig(Config.get(), "dynamicInstrumentationHoistLocalVarsEnabled", true); - try { - final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot31"; - LogProbe probe = - createMethodProbeAtExit(PROBE_ID, CLASS_NAME, "duplicateLocalDifferentScope", "(String)"); - TestSnapshotListener listener = installProbes(probe); - Class testClass = compileAndLoadClass(CLASS_NAME); - int result = Reflect.onClass(testClass).call("main", "duplicateLocalDifferentScope").get(); - assertEquals(28, result); - Snapshot snapshot = assertOneSnapshot(listener); - assertCaptureLocals( - snapshot.getCaptures().getReturn(), "ch", Character.TYPE.getTypeName(), "e"); - } finally { - setFieldInConfig(Config.get(), "dynamicInstrumentationHoistLocalVarsEnabled", false); - } + final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot31"; + LogProbe probe = + createMethodProbeAtExit(PROBE_ID, CLASS_NAME, "duplicateLocalDifferentScope", "(String)"); + TestSnapshotListener listener = installProbes(probe); + Class testClass = compileAndLoadClass(CLASS_NAME); + int result = Reflect.onClass(testClass).call("main", "duplicateLocalDifferentScope").get(); + assertEquals(28, result); + Snapshot snapshot = assertOneSnapshot(listener); + assertEquals(1, snapshot.getCaptures().getReturn().getLocals().size()); + // ch cannot be hoisted in safe mode, because it has the duplicate name with different slot + assertCaptureLocals( + snapshot.getCaptures().getReturn(), "@return", Integer.TYPE.getTypeName(), "28"); + } + + @Test + @DisabledIf( + value = "datadog.trace.api.Platform#isJ9", + disabledReason = "we cannot get local variable debug info") + public void mixingIntAndLongWhenHoisting() throws IOException, URISyntaxException { + final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot31"; + LogProbe probe = createMethodProbeAtExit(PROBE_ID, CLASS_NAME, "mixingIntAndLong", "(String)"); + TestSnapshotListener listener = installProbes(probe); + Class testClass = compileAndLoadClass(CLASS_NAME); + int result = Reflect.onClass(testClass).call("main", "mixingIntAndLong").get(); + assertEquals(1626, result); + Snapshot snapshot = assertOneSnapshot(listener); + assertEquals(2, snapshot.getCaptures().getReturn().getLocals().size()); + // no hoisting in safe mode: long local variable and 2-slots type (forbidden slots) + assertCaptureLocals(snapshot.getCaptures().getReturn(), "l", Long.TYPE.getTypeName(), "1626"); + assertCaptureLocals( + snapshot.getCaptures().getReturn(), "@return", Integer.TYPE.getTypeName(), "1626"); + } + + @Test + @DisabledIf( + value = "datadog.trace.api.Platform#isJ9", + disabledReason = "we cannot get local variable debug info") + public void mixingIntAndCharWhenHoisting() throws IOException, URISyntaxException { + final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot31"; + LogProbe probe = createMethodProbeAtExit(PROBE_ID, CLASS_NAME, "mixingIntAndChar", "(String)"); + TestSnapshotListener listener = installProbes(probe); + Class testClass = compileAndLoadClass(CLASS_NAME); + int result = Reflect.onClass(testClass).call("main", "mixingIntAndChar").get(); + assertEquals(-327, result); + Snapshot snapshot = assertOneSnapshot(listener); + assertEquals(2, snapshot.getCaptures().getReturn().getLocals().size()); + assertCaptureLocals( + snapshot.getCaptures().getReturn(), "i", Integer.TYPE.getTypeName(), "-327"); + assertCaptureLocals( + snapshot.getCaptures().getReturn(), "@return", Integer.TYPE.getTypeName(), "-327"); + } + + @Test + @DisabledIf( + value = "datadog.trace.api.Platform#isJ9", + disabledReason = "we cannot get local variable debug info") + public void mixingIntAndRefTypeWhenHoisting() throws IOException, URISyntaxException { + final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot31"; + LogProbe probe = + createMethodProbeAtExit(PROBE_ID, CLASS_NAME, "mixingIntAndRefType", "(String)"); + TestSnapshotListener listener = installProbes(probe); + Class testClass = compileAndLoadClass(CLASS_NAME); + int result = Reflect.onClass(testClass).call("main", "mixingIntAndRefType").get(); + assertEquals(19, result); + Snapshot snapshot = assertOneSnapshot(listener); + assertEquals(2, snapshot.getCaptures().getReturn().getLocals().size()); + assertCaptureLocals(snapshot.getCaptures().getReturn(), "i", Integer.TYPE.getTypeName(), "19"); + assertCaptureLocals( + snapshot.getCaptures().getReturn(), "@return", Integer.TYPE.getTypeName(), "19"); + } + + @Test + @DisabledIf( + value = "datadog.trace.api.Platform#isJ9", + disabledReason = "we cannot get local variable debug info") + public void sameSlotAndTypeDifferentName() throws IOException, URISyntaxException { + final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot31"; + LogProbe probe = + createMethodProbeAtExit(PROBE_ID, CLASS_NAME, "sameSlotAndTypeDifferentName", "(String)"); + TestSnapshotListener listener = installProbes(probe); + Class testClass = compileAndLoadClass(CLASS_NAME); + int result = Reflect.onClass(testClass).call("main", "sameSlotAndTypeDifferentName").get(); + assertEquals(28, result); + Snapshot snapshot = assertOneSnapshot(listener); + // r and o cannot be hoisted in safe mode, because they have the same slot + assertEquals(2, snapshot.getCaptures().getReturn().getLocals().size()); + assertCaptureLocals( + snapshot.getCaptures().getReturn(), + "p", + String.class.getTypeName(), + "sameSlotAndTypeDifferentName"); + assertCaptureLocals( + snapshot.getCaptures().getReturn(), "@return", Integer.TYPE.getTypeName(), "28"); + } + + @Test + @DisabledIf( + value = "datadog.trace.api.Platform#isJ9", + disabledReason = "we cannot get local variable debug info") + public void sameSlotAndNameOneReturn() throws IOException, URISyntaxException { + final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot31"; + LogProbe probe = + createMethodProbeAtExit(PROBE_ID, CLASS_NAME, "sameSlotAndNameOneReturn", "(String)"); + TestSnapshotListener listener = installProbes(probe); + Class testClass = compileAndLoadClass(CLASS_NAME); + int result = Reflect.onClass(testClass).call("main", "sameSlotAndNameOneReturn").get(); + assertEquals(24, result); + Snapshot snapshot = assertOneSnapshot(listener); + // o is not hoisted in safe mode, because it has the same slot and name on different range + assertEquals(2, snapshot.getCaptures().getReturn().getLocals().size()); + assertCaptureLocals( + snapshot.getCaptures().getReturn(), "result", Integer.TYPE.getTypeName(), "24"); + assertCaptureLocals( + snapshot.getCaptures().getReturn(), "@return", Integer.TYPE.getTypeName(), "24"); } @Test diff --git a/dd-java-agent/agent-debugger/src/test/resources/com/datadog/debugger/CapturedSnapshot31.java b/dd-java-agent/agent-debugger/src/test/resources/com/datadog/debugger/CapturedSnapshot31.java index c1895a5802b..2180f6e0978 100644 --- a/dd-java-agent/agent-debugger/src/test/resources/com/datadog/debugger/CapturedSnapshot31.java +++ b/dd-java-agent/agent-debugger/src/test/resources/com/datadog/debugger/CapturedSnapshot31.java @@ -21,9 +21,30 @@ public static int main(String arg) throws Exception { if ("duplicateLocalDifferentScope".equals(arg)) { return new CapturedSnapshot31().duplicateLocalDifferentScope(arg); } + if ("mixingIntAndLong".equals(arg)) { + return new CapturedSnapshot31().mixingIntAndLong(arg); + } + if ("mixingIntAndChar".equals(arg)) { + return new CapturedSnapshot31().mixingIntAndChar(arg); + } + if ("sameSlotAndTypeDifferentName".equals(arg)) { + return new CapturedSnapshot31().sameSlotAndTypeDifferentName(arg); + } + if ("mixingIntAndRefType".equals(arg)) { + return new CapturedSnapshot31().mixingIntAndRefType(arg); + } + if ("sameSlotAndNameOneReturn".equals(arg)) { + return new CapturedSnapshot31().sameSlotAndNameOneReturn(arg); + } return 0; } + // LocalVariableTable: + // Start Length Slot Name Signature + // 0 46 0 this Lcom/datadog/debugger/CapturedSnapshot31; + // 0 46 1 arg Ljava/lang/String; + // 20 26 2 varStr Ljava/lang/String; + // 44 2 3 len I private int uncaughtException(String arg) { String varStr = arg + "foo"; if (varStr.endsWith("foo")) { @@ -70,6 +91,12 @@ private int deepScopes(String arg) { return localVarL0; } + // LocalVariableTable: + // Start Length Slot Name Signature + // 41 6 2 ex Ljava/lang/IllegalStateException; + // 48 6 2 ex Ljava/lang/IllegalArgumentException; + // 0 54 0 this Lcom/datadog/debugger/CapturedSnapshot31; + // 0 54 1 arg Ljava/lang/String; private int caughtException(String arg) { try { if ("illegalState".equals(arg)) { @@ -115,6 +142,12 @@ private int overlappingSlots(String arg) { return subStr.length(); } + // LocalVariableTable: + // Start Length Slot Name Signature + // 20 37 2 ch C + // 83 48 5 ch C + // 0 142 0 this Lcom/datadog/debugger/CapturedSnapshot31; + // 0 142 1 arg Ljava/lang/String; private int duplicateLocalDifferentScope(String arg) { if (arg == null) { return 0; @@ -134,4 +167,108 @@ private int duplicateLocalDifferentScope(String arg) { return arg.length(); } } + + // LocalVariableTable: +// Start Length Slot Name Signature +// 7 7 2 i I +// 10 4 3 j I +// 19 25 4 i I +// 16 31 2 l J +// 0 47 0 this Lcom/datadog/debugger/CapturedSnapshot31; +// 0 47 1 arg Ljava/lang/String; +// +// when hoisting j local var we will extend the range and then overlap to the l range. But longs +// are occupying two slots, so we need to consider l having slot 2 and 3 and prevent j to be hoisted + private int mixingIntAndLong(String arg) { + if (arg == null) { + int i = 42; + int j = 84; + return i + j; + } else { + long l = 0; + for (int i = 0; i < arg.length(); i++) { + l += arg.charAt(i); + } + return (int) l; + } + } + + // LocalVariableTable: + // Start Length Slot Name Signature + // 7 2 2 i I + // 12 2 2 c C + // 0 14 0 this Lcom/datadog/debugger/CapturedSnapshot31; + // 0 14 1 arg Ljava/lang/String; + // + // when hoisting c local var we will extend the range and then overlap to the i range. But chars + // are considered at bytecode level as ints. BUT chars are unsigned shorts and treating them as + // signed int can lead to unexpected results (cf Character.valueOf(char) with negative value) + private int mixingIntAndChar(String arg) { + if (arg != null) { + int i = -327; + return i; + } else { + char c = 'a'; + return (int)c; + } + } + + + // LocalVariableTable: + // Start Length Slot Name Signature + // 9 2 2 i I + // 19 5 2 o Ljava/lang/Object; + // 0 24 0 this Lcom/datadog/debugger/CapturedSnapshot31; + // 0 24 1 arg Ljava/lang/String; + private int mixingIntAndRefType(String arg) { + if (arg != null) { + int i = arg.length(); + return i; + } else { + Object o = new Object(); + return o.hashCode(); + } + } + + // LocalVariableTable: + // Start Length Slot Name Signature + // 13 5 2 p Ljava/lang/Object; + // 26 5 2 r Ljava/lang/Object; + // 39 5 2 o Ljava/lang/Object; + // 0 44 0 this Lcom/datadog/debugger/CapturedSnapshot31; + // 0 44 1 arg Ljava/lang/String; + private int sameSlotAndTypeDifferentName(String arg) { + if (arg != null) { + if (arg.length() > 0) { + Object p = arg; + return arg.length(); + } else { + Object r = new Object(); + return r.hashCode(); + } + } else { + Object o = new Object(); + return o.hashCode(); + } + } + + // LocalVariableTable: + // Start Length Slot Name Signature + // 6 5 3 o Ljava/lang/String; + // 11 3 2 result I + // 22 5 3 o Ljava/lang/Object; + // 0 29 0 this Lcom/datadog/debugger/CapturedSnapshot31; + // 0 29 1 arg Ljava/lang/String; + // 27 2 2 result I + private int sameSlotAndNameOneReturn(String arg) { + int result; + if (arg != null) { + String o = arg; + result = arg.length(); + } else { + Object o = new Object(); + result = o.hashCode(); + } + return result; + } } diff --git a/dd-smoke-tests/debugger-integration-tests/src/test/java/datadog/smoketest/LogProbesIntegrationTest.java b/dd-smoke-tests/debugger-integration-tests/src/test/java/datadog/smoketest/LogProbesIntegrationTest.java index 22a3c138d74..1e4824cfb60 100644 --- a/dd-smoke-tests/debugger-integration-tests/src/test/java/datadog/smoketest/LogProbesIntegrationTest.java +++ b/dd-smoke-tests/debugger-integration-tests/src/test/java/datadog/smoketest/LogProbesIntegrationTest.java @@ -79,8 +79,8 @@ void testFullMethod() throws Exception { "java.lang.String", "42, foobar, 3.42, {key1=val1, key2=val2, key3=val3}, var1,var2,var3"); assertNotNull(snapshot.getCaptures().getReturn().getLocals()); - // @return is the only locals - assertEquals(1, snapshot.getCaptures().getReturn().getLocals().size()); + // @return is the only locals but ex is safely hoisted in the method + assertEquals(2, snapshot.getCaptures().getReturn().getLocals().size()); assertNull(snapshot.getCaptures().getReturn().getCapturedThrowable()); snapshotReceived.set(true); }); diff --git a/dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java b/dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java index 7468bf0de96..a6d5888e76f 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java @@ -195,7 +195,7 @@ public final class ConfigDefaults { static final int DEFAULT_DYNAMIC_INSTRUMENTATION_MAX_PAYLOAD_SIZE = 1024; // KiB static final boolean DEFAULT_DYNAMIC_INSTRUMENTATION_VERIFY_BYTECODE = true; static final int DEFAULT_DYNAMIC_INSTRUMENTATION_CAPTURE_TIMEOUT = 100; // milliseconds - static final boolean DEFAULT_DYNAMIC_INSTRUMENTATION_HOIST_LOCALVARS_ENABLED = false; + static final int DEFAULT_DYNAMIC_INSTRUMENTATION_LOCALVAR_HOISTING_LEVEL = 1; static final boolean DEFAULT_SYMBOL_DATABASE_ENABLED = true; static final boolean DEFAULT_SYMBOL_DATABASE_FORCE_UPLOAD = false; static final int DEFAULT_SYMBOL_DATABASE_FLUSH_THRESHOLD = 100; // nb of classes diff --git a/dd-trace-api/src/main/java/datadog/trace/api/config/DebuggerConfig.java b/dd-trace-api/src/main/java/datadog/trace/api/config/DebuggerConfig.java index 0f7a418124d..c92d5705962 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/config/DebuggerConfig.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/config/DebuggerConfig.java @@ -40,8 +40,8 @@ public final class DebuggerConfig { "dynamic.instrumentation.redaction.excluded.identifiers"; public static final String DYNAMIC_INSTRUMENTATION_REDACTED_TYPES = "dynamic.instrumentation.redacted.types"; - public static final String DYNAMIC_INSTRUMENTATION_HOIST_LOCALVARS_ENABLED = - "dynamic.instrumentation.hoist.localvars.enabled"; + public static final String DYNAMIC_INSTRUMENTATION_LOCALVAR_HOISTING_LEVEL = + "dynamic.instrumentation.localvar.hoisting.level"; public static final String SYMBOL_DATABASE_ENABLED = "symbol.database.upload.enabled"; public static final String SYMBOL_DATABASE_FORCE_UPLOAD = "internal.force.symbol.database.upload"; public static final String SYMBOL_DATABASE_FLUSH_THRESHOLD = "symbol.database.flush.threshold"; diff --git a/internal-api/src/main/java/datadog/trace/api/Config.java b/internal-api/src/main/java/datadog/trace/api/Config.java index 8674954e210..b76ae0cfea8 100644 --- a/internal-api/src/main/java/datadog/trace/api/Config.java +++ b/internal-api/src/main/java/datadog/trace/api/Config.java @@ -427,7 +427,7 @@ public static String getHostName() { private final String dynamicInstrumentationRedactedIdentifiers; private final Set dynamicInstrumentationRedactionExcludedIdentifiers; private final String dynamicInstrumentationRedactedTypes; - private final boolean dynamicInstrumentationHoistLocalVarsEnabled; + private final int dynamicInstrumentationLocalVarHoistingLevel; private final boolean symbolDatabaseEnabled; private final boolean symbolDatabaseForceUpload; private final int symbolDatabaseFlushThreshold; @@ -1735,10 +1735,10 @@ PROFILING_DATADOG_PROFILER_ENABLED, isDatadogProfilerSafeInCurrentEnvironment()) configProvider.getList(DYNAMIC_INSTRUMENTATION_REDACTION_EXCLUDED_IDENTIFIERS)); dynamicInstrumentationRedactedTypes = configProvider.getString(DYNAMIC_INSTRUMENTATION_REDACTED_TYPES, null); - dynamicInstrumentationHoistLocalVarsEnabled = - configProvider.getBoolean( - DYNAMIC_INSTRUMENTATION_HOIST_LOCALVARS_ENABLED, - DEFAULT_DYNAMIC_INSTRUMENTATION_HOIST_LOCALVARS_ENABLED); + dynamicInstrumentationLocalVarHoistingLevel = + configProvider.getInteger( + DYNAMIC_INSTRUMENTATION_LOCALVAR_HOISTING_LEVEL, + DEFAULT_DYNAMIC_INSTRUMENTATION_LOCALVAR_HOISTING_LEVEL); symbolDatabaseEnabled = configProvider.getBoolean(SYMBOL_DATABASE_ENABLED, DEFAULT_SYMBOL_DATABASE_ENABLED); symbolDatabaseForceUpload = @@ -3426,8 +3426,8 @@ public String getDynamicInstrumentationRedactedTypes() { return dynamicInstrumentationRedactedTypes; } - public boolean isDynamicInstrumentationHoistLocalVarsEnabled() { - return dynamicInstrumentationHoistLocalVarsEnabled; + public int getDynamicInstrumentationLocalVarHoistingLevel() { + return dynamicInstrumentationLocalVarHoistingLevel; } public boolean isAwsPropagationEnabled() {