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..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,12 +153,38 @@ 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. + * try/catch range. Other line labels must have an empty operand stack, see {@link + * #checkLineLabel(LabelNode, Map)}. */ private InsnList wrapLineTryCatch(InsnList insnList, LabelNode lineLabel) { InsnList wrapped = wrapTryCatch(insnList); @@ -410,13 +437,22 @@ private InstrumentationResult.Status addLineMetric(ClassFileLines classFileLines reportError("Missing line debug information."); return InstrumentationResult.Status.ERROR; } + // 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); @@ -433,6 +469,17 @@ private InstrumentationResult.Status addLineMetric(ClassFileLines classFileLines 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/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/agent/MetricProbesInstrumentationTest.java b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/MetricProbesInstrumentationTest.java index 96aad44f579..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 @@ -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 + + ": operand stack is not empty, line is 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/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); 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(); + } }