From c44ac69e4cad6685cee7d256b19b9c7a762e1dde Mon Sep 17 00:00:00 2001 From: jean-philippe bempel Date: Wed, 25 Sep 2024 09:35:49 +0200 Subject: [PATCH] Fix duplicated locals with arguments When hoisting local variables remove the locals that could be have the same name than arguments. This could happen resulting of the instrumentation by the tracer that reshuffle the local variable table and then reassign dedicated slots to those arguments --- .../CapturedContextInstrumentor.java | 18 ++++++++++++++++-- .../debugger/instrumentation/Instrumentor.java | 4 ++-- .../instrumentation/MetricInstrumentor.java | 4 ++-- .../TracerDebuggerIntegrationTest.java | 3 +++ 4 files changed, 23 insertions(+), 6 deletions(-) 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 af0bb92b2a8..dda1265b201 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 @@ -476,6 +476,7 @@ private Collection initAndHoistLocalVars(InsnList insnList) { } checkHoistableLocalVar(localVar, localVarsByName, localVarsBySlot, hoistableVarByName); } + removeDuplicatesFromArgs(hoistableVarByName, localVarsBySlotArray); // hoist vars List results = new ArrayList<>(); for (Map.Entry> entry : hoistableVarByName.entrySet()) { @@ -512,6 +513,19 @@ private Collection initAndHoistLocalVars(InsnList insnList) { 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)) { @@ -762,8 +776,8 @@ private void collectArguments(InsnList insnList, Snapshot.Kind kind) { int slot = isStatic ? 0 : 1; for (Type argType : argTypes) { String currentArgName = null; - if (slot < localVarsBySlot.length) { - LocalVariableNode localVarNode = localVarsBySlot[slot]; + if (slot < localVarsBySlotArray.length) { + LocalVariableNode localVarNode = localVarsBySlotArray[slot]; currentArgName = localVarNode != null ? localVarNode.name : null; } if (currentArgName == null) { diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/Instrumentor.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/Instrumentor.java index 0f81bf033d6..d16e2fda812 100644 --- a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/Instrumentor.java +++ b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/Instrumentor.java @@ -45,7 +45,7 @@ public abstract class Instrumentor { protected final LabelNode methodEnterLabel; protected int localVarBaseOffset; protected int argOffset; - protected final LocalVariableNode[] localVarsBySlot; + protected final LocalVariableNode[] localVarsBySlotArray; protected LabelNode returnHandlerLabel; protected final List finallyBlocks = new ArrayList<>(); @@ -68,7 +68,7 @@ public Instrumentor( for (Type t : argTypes) { argOffset += t.getSize(); } - localVarsBySlot = extractLocalVariables(argTypes); + localVarsBySlotArray = extractLocalVariables(argTypes); } public abstract InstrumentationResult.Status instrument(); diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/MetricInstrumentor.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/MetricInstrumentor.java index 1905d451f49..11c46673318 100644 --- a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/MetricInstrumentor.java +++ b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/MetricInstrumentor.java @@ -697,8 +697,8 @@ private ASMHelper.Type tryRetrieveArgument(String head, InsnList insnList) { int slot = instrumentor.isStatic ? 0 : 1; for (Type argType : argTypes) { String currentArgName = null; - if (instrumentor.localVarsBySlot.length > 0) { - LocalVariableNode localVarNode = instrumentor.localVarsBySlot[slot]; + if (instrumentor.localVarsBySlotArray.length > 0) { + LocalVariableNode localVarNode = instrumentor.localVarsBySlotArray[slot]; currentArgName = localVarNode != null ? localVarNode.name : null; } if (currentArgName == null) { diff --git a/dd-smoke-tests/debugger-integration-tests/src/test/java/datadog/smoketest/TracerDebuggerIntegrationTest.java b/dd-smoke-tests/debugger-integration-tests/src/test/java/datadog/smoketest/TracerDebuggerIntegrationTest.java index 4b1884dd4b9..4361a8e5573 100644 --- a/dd-smoke-tests/debugger-integration-tests/src/test/java/datadog/smoketest/TracerDebuggerIntegrationTest.java +++ b/dd-smoke-tests/debugger-integration-tests/src/test/java/datadog/smoketest/TracerDebuggerIntegrationTest.java @@ -4,6 +4,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import com.datadog.debugger.agent.JsonSnapshotSerializer; @@ -99,6 +100,8 @@ void testTracerSameMethod() throws Exception { Snapshot snapshot = request.getDebugger().getSnapshot(); assertEquals(PROBE_ID.getId(), snapshot.getProbe().getId()); assertEquals(42, snapshot.getCaptures().getEntry().getArguments().get("argInt").getValue()); + // no locals captured + assertNull(snapshot.getCaptures().getEntry().getLocals()); assertTrue(Pattern.matches("[0-9a-f]+", request.getTraceId())); assertTrue(Pattern.matches("\\d+", request.getSpanId())); }