From e7c99533d1bb964dc646a6cd12f2b89dbce6d2d1 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Bempel Date: Mon, 19 Oct 2026 11:06:29 +0200 Subject: [PATCH 1/3] Fix instrumenting metric on multi-line expressions On a multi-line expressions, we are now preventing instrumentation of a metric line probe. Example: ``` 1 var sb = new StringBuilder( 2 getName() 3 ) 4 .append("foo") 5 .build(); ``` with computeFrame we are detecting non-empty operand stack which means we are in the middle of an expression or statement. We are throwing an exception an return an ERROR for instrumentation result. --- .../debugger/instrumentation/ASMHelper.java | 2 +- .../instrumentation/MetricInstrumenter.java | 50 ++++++++++++++----- .../MetricProbesInstrumentationTest.java | 22 ++++++++ .../test/resources/CapturedSnapshot05.java | 6 +++ 4 files changed, 66 insertions(+), 14 deletions(-) diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/ASMHelper.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/ASMHelper.java index 973fd61d773..49f384bc9fc 100644 --- a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/ASMHelper.java +++ b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/ASMHelper.java @@ -486,7 +486,7 @@ protected static Map> computeFrames( methodNode.desc, ex); // when running tests, fails if an exception is thrown during analysis - // Gradle is running tests with assertions enbabled + // Gradle is running tests with assertions enabled assert false; break; } diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/MetricInstrumenter.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/MetricInstrumenter.java index 61e924fd047..8ad397c1b11 100644 --- a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/MetricInstrumenter.java +++ b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/MetricInstrumenter.java @@ -157,17 +157,31 @@ private void addFinallyHandler(LabelNode endLabel) { * the start of a catch handler, the caught exception is still on the operand stack. The exception * handler of the try/catch clears the stack, so the exception is saved in a temporary local * before the call and loaded back after it. This keeps the stack empty at both ends of the - * try/catch range. + * try/catch range. If the line label is in the middle of an expression, the operand stack is not + * empty and may hold uninitialized references that cannot be saved in locals, so the metric call + * is not wrapped: jumping from the exception handler back to the line with an empty stack would + * fail verification. If the operand stack state at the line label is unknown, the line cannot be + * instrumented safely and {@link IllegalStateException} is thrown. */ - private InsnList wrapLineTryCatch(InsnList insnList, LabelNode lineLabel) { - InsnList wrapped = wrapTryCatch(insnList); - if (wrapped == EMPTY_INSN_LIST) { - return wrapped; + private InsnList wrapLineTryCatch( + InsnList insnList, LabelNode lineLabel, Map> frames) { + if (insnList == null || insnList == EMPTY_INSN_LIST) { + return EMPTY_INSN_LIST; } TryCatchBlockNode catchHandler = findCatchHandler(lineLabel); if (catchHandler == null) { - return wrapped; + Frame frame = frames.get(lineLabel); + if (frame == null) { + throw new IllegalStateException("operand stack state cannot be computed"); + } + if (frame.getStackSize() > 0) { + // if stack is non-empty, means we are in the middle of a statement or expression + throw new IllegalStateException( + "Cannot instrument this line, probably in the middle of a statement or expression"); + } + return wrapTryCatch(insnList); } + InsnList wrapped = wrapTryCatch(insnList); int exceptionVar = newVar(OBJECT_TYPE); // stack [exception] wrapped.insert(new VarInsnNode(Opcodes.ASTORE, exceptionVar)); @@ -410,6 +424,9 @@ private InstrumentationResult.Status addLineMetric(ClassFileLines classFileLines reportError("Missing line debug information."); return InstrumentationResult.Status.ERROR; } + // computed before any insertion: frames are keyed by the original line labels + Map> frames = + ASMHelper.computeFrames(classNode.name, methodNode); for (Where.SourceLine sourceLine : targetLines) { int from = sourceLine.getFrom(); int till = sourceLine.getTill(); @@ -421,13 +438,20 @@ private InstrumentationResult.Status addLineMetric(ClassFileLines classFileLines reportError( "No line info for " + (sourceLine.isSingleLine() ? "line " : "range ") + sourceLine); } - if (beforeLabel != null) { - InsnList insnList = wrapLineTryCatch(callMetric(metricProbe, beforeLabel), beforeLabel); - methodNode.instructions.insertBefore(beforeLabel.getNext(), insnList); - } - if (afterLabel != null && !sourceLine.isSingleLine()) { - InsnList insnList = wrapLineTryCatch(callMetric(metricProbe, afterLabel), afterLabel); - methodNode.instructions.insert(afterLabel, insnList); + try { + if (beforeLabel != null) { + InsnList insnList = + wrapLineTryCatch(callMetric(metricProbe, beforeLabel), beforeLabel, frames); + methodNode.instructions.insertBefore(beforeLabel.getNext(), insnList); + } + if (afterLabel != null && !sourceLine.isSingleLine()) { + InsnList insnList = + wrapLineTryCatch(callMetric(metricProbe, afterLabel), afterLabel, frames); + methodNode.instructions.insert(afterLabel, insnList); + } + } catch (IllegalStateException ex) { + reportError("Cannot instrument line " + sourceLine + ": " + ex.getMessage()); + return InstrumentationResult.Status.ERROR; } } return InstrumentationResult.Status.INSTALLED; diff --git a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/MetricProbesInstrumentationTest.java b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/MetricProbesInstrumentationTest.java index 96aad44f579..c60a482b1ed 100644 --- a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/MetricProbesInstrumentationTest.java +++ b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/MetricProbesInstrumentationTest.java @@ -546,6 +546,28 @@ public void lineCatchHandlerCountMetric() throws IOException, URISyntaxException verify(probeStatusSink, times(0)).addError(any(), anyString()); } + @Test + public void lineMultiLineExpressionCountMetric() throws IOException, URISyntaxException { + final String CLASS_NAME = "CapturedSnapshot05"; + String METRIC_NAME = "expr_count"; + // line in the middle of a constructor call: operand stack holds uninitialized references + int line = getLineForLineProbe(CLASS_NAME, LINE_METRIC_ID4); + MetricProbe metricProbe = + createLineMetric(LINE_METRIC_ID4, METRIC_NAME, COUNT, CLASS_NAME, null, line); + MetricForwarderListener listener = installMetricProbes(metricProbe); + Class testClass = compileAndLoadClass(CLASS_NAME); + String result = Reflect.on(testClass).call("multiLineExpression", "foo").get(); + assertEquals("f", result); + assertFalse(listener.counters.containsKey(METRIC_NAME)); + verify(probeStatusSink) + .addError( + eq(LINE_METRIC_ID4), + eq( + "Cannot instrument line " + + line + + ": Cannot instrument this line, probably in the middle of a statement or expression")); + } + @Test public void invalidNameArgumentRefValueCountMetric() throws IOException, URISyntaxException { final String CLASS_NAME = "CapturedSnapshot03"; diff --git a/dd-java-agent/agent-debugger/src/test/resources/CapturedSnapshot05.java b/dd-java-agent/agent-debugger/src/test/resources/CapturedSnapshot05.java index 470d22b1ba4..47442942699 100644 --- a/dd-java-agent/agent-debugger/src/test/resources/CapturedSnapshot05.java +++ b/dd-java-agent/agent-debugger/src/test/resources/CapturedSnapshot05.java @@ -59,4 +59,10 @@ public CustomException(String message, String additionalMsg) { this.additionalMsg = additionalMsg; } } + + static String multiLineExpression(String arg) { + StringBuilder sb = new StringBuilder( + arg.substring(0, 1)); // beae1817-f3b0-4ea8-a74f-000000000004 + return sb.toString(); + } } From ea6d6ed6a61e7d46e259e74709163923e00fc330 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Bempel Date: Mon, 19 Oct 2026 15:22:30 +0200 Subject: [PATCH 2/3] check line label before modifying it --- .../instrumentation/MetricInstrumenter.java | 111 +++++++++++------- .../MetricProbesInstrumentationTest.java | 2 +- 2 files changed, 68 insertions(+), 45 deletions(-) diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/MetricInstrumenter.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/MetricInstrumenter.java index 8ad397c1b11..9b41db1dd39 100644 --- a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/MetricInstrumenter.java +++ b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/MetricInstrumenter.java @@ -1,5 +1,6 @@ package com.datadog.debugger.instrumentation; +import static com.datadog.debugger.instrumentation.ASMHelper.computeFrames; import static com.datadog.debugger.instrumentation.ASMHelper.decodeSignature; import static com.datadog.debugger.instrumentation.ASMHelper.emitReflectiveCall; import static com.datadog.debugger.instrumentation.ASMHelper.ensureSafeClassLoad; @@ -124,7 +125,7 @@ public InstrumentationResult.Status instrument() { case EXIT: { Map> frames = - ASMHelper.computeFrames(classNode.name, methodNode); + computeFrames(classNode.name, methodNode); processInstructions(frames); addFinallyHandler(returnHandlerLabel); installFinallyBlocks(); @@ -152,36 +153,48 @@ private void addFinallyHandler(LabelNode endLabel) { createFinallyHandler(durationStartLabel, endLabel, wrapTryCatch(insnList)); } + /** + * Checks that a metric call can be inserted at the line label. If the line label is in the middle + * of an expression, the operand stack is not empty and may hold uninitialized references that + * cannot be saved in locals: jumping from the exception handler of {@link + * #wrapLineTryCatch(InsnList, LabelNode)} back to the line with an empty stack would fail + * verification. The start of a catch handler is accepted, the caught exception on the stack is + * handled by {@link #wrapLineTryCatch(InsnList, LabelNode)}. + * + * @return the reason why the line cannot be instrumented, or null if it can + */ + private String checkLineLabel( + LabelNode lineLabel, Map> frames) { + if (lineLabel == null || findCatchHandler(lineLabel) != null) { + return null; + } + Frame frame = frames.get(lineLabel); + if (frame == null) { + return "operand stack state cannot be computed"; + } + if (frame.getStackSize() > 0) { + return "operand stack is not empty, line is probably in the middle of a statement or expression"; + } + return null; + } + /** * Wraps the metric call in a try/catch like {@link #wrapTryCatch(InsnList)}. If the line label is * the start of a catch handler, the caught exception is still on the operand stack. The exception * handler of the try/catch clears the stack, so the exception is saved in a temporary local * before the call and loaded back after it. This keeps the stack empty at both ends of the - * try/catch range. If the line label is in the middle of an expression, the operand stack is not - * empty and may hold uninitialized references that cannot be saved in locals, so the metric call - * is not wrapped: jumping from the exception handler back to the line with an empty stack would - * fail verification. If the operand stack state at the line label is unknown, the line cannot be - * instrumented safely and {@link IllegalStateException} is thrown. + * try/catch range. Other line labels must have an empty operand stack, see {@link + * #checkLineLabel(LabelNode, Map)}. */ - private InsnList wrapLineTryCatch( - InsnList insnList, LabelNode lineLabel, Map> frames) { - if (insnList == null || insnList == EMPTY_INSN_LIST) { - return EMPTY_INSN_LIST; + private InsnList wrapLineTryCatch(InsnList insnList, LabelNode lineLabel) { + InsnList wrapped = wrapTryCatch(insnList); + if (wrapped == EMPTY_INSN_LIST) { + return wrapped; } TryCatchBlockNode catchHandler = findCatchHandler(lineLabel); if (catchHandler == null) { - Frame frame = frames.get(lineLabel); - if (frame == null) { - throw new IllegalStateException("operand stack state cannot be computed"); - } - if (frame.getStackSize() > 0) { - // if stack is non-empty, means we are in the middle of a statement or expression - throw new IllegalStateException( - "Cannot instrument this line, probably in the middle of a statement or expression"); - } - return wrapTryCatch(insnList); + return wrapped; } - InsnList wrapped = wrapTryCatch(insnList); int exceptionVar = newVar(OBJECT_TYPE); // stack [exception] wrapped.insert(new VarInsnNode(Opcodes.ASTORE, exceptionVar)); @@ -424,39 +437,49 @@ private InstrumentationResult.Status addLineMetric(ClassFileLines classFileLines reportError("Missing line debug information."); return InstrumentationResult.Status.ERROR; } - // computed before any insertion: frames are keyed by the original line labels - Map> frames = - ASMHelper.computeFrames(classNode.name, methodNode); + // all lines are checked before any insertion: frames are keyed by the original line labels + // and a rejected line must not leave the method partially instrumented + Map> frames = computeFrames(classNode.name, methodNode); for (Where.SourceLine sourceLine : targetLines) { - int from = sourceLine.getFrom(); - int till = sourceLine.getTill(); - LabelNode beforeLabel = classFileLines.getLineLabel(from); - // single line N capture translates to line range (N, N+1) - LabelNode afterLabel = - classFileLines.getLineLabel(till + (sourceLine.isSingleLine() ? 1 : 0)); + String error = checkLineLabel(getBeforeLabel(classFileLines, sourceLine), frames); + if (error == null && !sourceLine.isSingleLine()) { + error = checkLineLabel(getAfterLabel(classFileLines, sourceLine), frames); + } + if (error != null) { + reportError("Cannot instrument line " + sourceLine + ": " + error); + return InstrumentationResult.Status.ERROR; + } + } + for (Where.SourceLine sourceLine : targetLines) { + LabelNode beforeLabel = getBeforeLabel(classFileLines, sourceLine); + LabelNode afterLabel = getAfterLabel(classFileLines, sourceLine); if (beforeLabel == null && afterLabel == null) { reportError( "No line info for " + (sourceLine.isSingleLine() ? "line " : "range ") + sourceLine); } - try { - if (beforeLabel != null) { - InsnList insnList = - wrapLineTryCatch(callMetric(metricProbe, beforeLabel), beforeLabel, frames); - methodNode.instructions.insertBefore(beforeLabel.getNext(), insnList); - } - if (afterLabel != null && !sourceLine.isSingleLine()) { - InsnList insnList = - wrapLineTryCatch(callMetric(metricProbe, afterLabel), afterLabel, frames); - methodNode.instructions.insert(afterLabel, insnList); - } - } catch (IllegalStateException ex) { - reportError("Cannot instrument line " + sourceLine + ": " + ex.getMessage()); - return InstrumentationResult.Status.ERROR; + if (beforeLabel != null) { + InsnList insnList = wrapLineTryCatch(callMetric(metricProbe, beforeLabel), beforeLabel); + methodNode.instructions.insertBefore(beforeLabel.getNext(), insnList); + } + if (afterLabel != null && !sourceLine.isSingleLine()) { + InsnList insnList = wrapLineTryCatch(callMetric(metricProbe, afterLabel), afterLabel); + methodNode.instructions.insert(afterLabel, insnList); } } return InstrumentationResult.Status.INSTALLED; } + private static LabelNode getBeforeLabel( + ClassFileLines classFileLines, Where.SourceLine sourceLine) { + return classFileLines.getLineLabel(sourceLine.getFrom()); + } + + private static LabelNode getAfterLabel( + ClassFileLines classFileLines, Where.SourceLine sourceLine) { + // single line N capture translates to line range (N, N+1) + return classFileLines.getLineLabel(sourceLine.getTill() + (sourceLine.isSingleLine() ? 1 : 0)); + } + private static class VisitorResult { final ASMHelper.Type type; final InsnList insnList; diff --git a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/MetricProbesInstrumentationTest.java b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/MetricProbesInstrumentationTest.java index c60a482b1ed..783a4bd0347 100644 --- a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/MetricProbesInstrumentationTest.java +++ b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/MetricProbesInstrumentationTest.java @@ -565,7 +565,7 @@ public void lineMultiLineExpressionCountMetric() throws IOException, URISyntaxEx eq( "Cannot instrument line " + line - + ": Cannot instrument this line, probably in the middle of a statement or expression")); + + ": operand stack is not empty, line is probably in the middle of a statement or expression")); } @Test From 5dbc4e009cf60c12a6ac435b1fe5e04073a5f7d2 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Bempel Date: Tue, 20 Oct 2026 11:49:32 +0200 Subject: [PATCH 3/3] address AI comment fix an issue with probe status, use encoded Id instead of raw probe Id for addDiagnostics --- .../com/datadog/debugger/sink/ProbeStatusSink.java | 2 +- .../com/datadog/debugger/sink/ProbeStatusSinkTest.java | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/sink/ProbeStatusSink.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/sink/ProbeStatusSink.java index 6a17a522b0e..86f636694cb 100644 --- a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/sink/ProbeStatusSink.java +++ b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/sink/ProbeStatusSink.java @@ -186,7 +186,7 @@ private void addDiagnostics(ProbeStatus message) { return; } ProbeId probeId = message.getDiagnostics().getProbeId(); - TimedMessage current = probeStatuses.get(probeId.getId()); + TimedMessage current = probeStatuses.get(probeId.getEncodedId()); if (current == null || shouldOverwrite(current.getMessage(), message)) { TimedMessage newMessage = new TimedMessage(message); probeStatuses.put(probeId.getEncodedId(), newMessage); diff --git a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/sink/ProbeStatusSinkTest.java b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/sink/ProbeStatusSinkTest.java index 6a80fc6418b..aef920e08b4 100644 --- a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/sink/ProbeStatusSinkTest.java +++ b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/sink/ProbeStatusSinkTest.java @@ -264,6 +264,16 @@ void doNotReemitRemovedMessage() { assertEquals(Collections.emptyList(), probeStatusSink.getDiagnostics()); } + @Test + void doNotReemitSameStatusAfterSentIfIntervalHasNotPassed() { + probeStatusSink.addInstalled(PROBE_ID); + assertEquals( + Collections.singletonList(builder.installedMessage(PROBE_ID)), + probeStatusSink.getDiagnostics()); + probeStatusSink.addInstalled(PROBE_ID); + assertEquals(Collections.emptyList(), probeStatusSink.getDiagnostics()); + } + @Test void dropRepeatingDiagnostics() { probeStatusSink.addReceived(PROBE_ID);