Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ensure locals are in scope when generating metrics #7121

Merged
merged 1 commit into from
Jun 4, 2024
Merged

Ensure locals are in scope when generating metrics #7121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions ...va-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/ASMHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,15 @@
import org.objectweb.asm.Type;
import org.objectweb.asm.signature.SignatureReader;
import org.objectweb.asm.signature.SignatureVisitor;
import org.objectweb.asm.tree.AbstractInsnNode;
import org.objectweb.asm.tree.FieldInsnNode;
import org.objectweb.asm.tree.FieldNode;
import org.objectweb.asm.tree.InsnList;
import org.objectweb.asm.tree.InsnNode;
import org.objectweb.asm.tree.LdcInsnNode;
import org.objectweb.asm.tree.LocalVariableNode;
import org.objectweb.asm.tree.MethodInsnNode;
import org.objectweb.asm.tree.MethodNode;
import org.objectweb.asm.tree.TypeInsnNode;

/** Helper class for bytecode generation */
Expand Down Expand Up @@ -274,6 +276,23 @@ public static void invokeConstructor(
// stack: []
}

/** Checks if the given variable is in scope at the given location */
public static boolean isInScope(
MethodNode methodNode, LocalVariableNode variableNode, AbstractInsnNode location) {
AbstractInsnNode startScope =
variableNode.start != null ? variableNode.start : methodNode.instructions.getFirst();
AbstractInsnNode endScope =
variableNode.end != null ? variableNode.end : methodNode.instructions.getLast();
AbstractInsnNode insn = startScope;
while (insn != null && insn != endScope) {
if (insn == location) {
return true;
}
insn = insn.getNext();
}
return false;
}

/** Wraps ASM's {@link org.objectweb.asm.Type} with associated generic types */
public static class Type {
private final org.objectweb.asm.Type mainType;
Expand Down
27 changes: 9 additions & 18 deletions ...ugger/src/main/java/com/datadog/debugger/instrumentation/CapturedContextInstrumentor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,10 @@ private int addExceptionLocal(TryCatchBlockNode catchHandler, MethodNode methodN
|| current.getType() == AbstractInsnNode.LINE)) {
current = current.getNext();
}
if (current == null) {
reportWarning("Cannot add exception local variable to catch block - no instructions.");
return -1;
}
if (current.getOpcode() != Opcodes.ASTORE) {
reportWarning("Cannot add exception local variable to catch block - no store instruction.");
return -1;
Expand Down Expand Up @@ -673,8 +677,11 @@ private void collectLocalVariables(AbstractInsnNode location, InsnList insnList)
List<LocalVariableNode> applicableVars = new ArrayList<>();
for (LocalVariableNode variableNode : methodNode.localVariables) {
int idx = variableNode.index - localVarBaseOffset;
if (idx >= argOffset && isInScope(variableNode, location)) {
applicableVars.add(variableNode);
if (idx >= argOffset) {
// var is local not arg
if (ASMHelper.isInScope(methodNode, variableNode, location)) {
applicableVars.add(variableNode);
}
}
}

Expand Down Expand Up @@ -1072,22 +1079,6 @@ private static void addInheritedStaticFields(
}
}

private boolean isInScope(LocalVariableNode variableNode, AbstractInsnNode location) {
AbstractInsnNode startScope =
variableNode.start != null ? variableNode.start : methodNode.instructions.getFirst();
AbstractInsnNode endScope =
variableNode.end != null ? variableNode.end : methodNode.instructions.getLast();

AbstractInsnNode insn = startScope;
while (insn != null && insn != endScope) {
if (insn == location) {
return true;
}
insn = insn.getNext();
}
return false;
}

private int declareContextVar(InsnList insnList) {
int var = newVar(CAPTURED_CONTEXT_TYPE);
getStatic(insnList, CAPTURED_CONTEXT_TYPE, "EMPTY_CAPTURING_CONTEXT");
Expand Down
54 changes: 33 additions & 21 deletions ...agent-debugger/src/main/java/com/datadog/debugger/instrumentation/MetricInstrumentor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import static com.datadog.debugger.instrumentation.ASMHelper.invokeInterface;
import static com.datadog.debugger.instrumentation.ASMHelper.invokeStatic;
import static com.datadog.debugger.instrumentation.ASMHelper.invokeVirtual;
import static com.datadog.debugger.instrumentation.ASMHelper.isInScope;
import static com.datadog.debugger.instrumentation.ASMHelper.isStaticField;
import static com.datadog.debugger.instrumentation.ASMHelper.ldc;
import static com.datadog.debugger.instrumentation.Types.*;
Expand Down Expand Up @@ -101,7 +102,7 @@ public InstrumentationResult.Status instrument() {
case ENTRY:
case DEFAULT:
{
InsnList insnList = wrapTryCatch(callMetric(metricProbe));
InsnList insnList = wrapTryCatch(callMetric(metricProbe, null));
methodNode.instructions.insert(methodEnterLabel, insnList);
break;
}
Expand Down Expand Up @@ -159,7 +160,7 @@ protected InsnList getBeforeReturnInsnList(AbstractInsnNode node) {
switch (node.getOpcode()) {
case Opcodes.RET:
case Opcodes.RETURN:
return wrapTryCatch(callMetric(metricProbe));
return wrapTryCatch(callMetric(metricProbe, node));
case Opcodes.LRETURN:
storeOpCode = Opcodes.LSTORE;
loadOpCode = Opcodes.LLOAD;
Expand Down Expand Up @@ -192,7 +193,8 @@ protected InsnList getBeforeReturnInsnList(AbstractInsnNode node) {
}
int tmpIdx = newVar(size);
InsnList insnList =
wrapTryCatch(callMetric(metricProbe, new ReturnContext(tmpIdx, loadOpCode, returnType)));
wrapTryCatch(
callMetric(metricProbe, node, new ReturnContext(tmpIdx, loadOpCode, returnType)));
// store return value from the stack to local before wrapped call
insnList.insert(new VarInsnNode(storeOpCode, tmpIdx));
// restore return value to the stack after wrapped call
Expand All @@ -219,7 +221,8 @@ private void createFinallyHandler(LabelNode startLabel, LabelNode endLabel, Insn
finallyBlocks.add(new FinallyBlock(startLabel, endLabel, handlerLabel));
}

private InsnList callCount(MetricProbe metricProbe, ReturnContext returnContext) {
private InsnList callCount(
MetricProbe metricProbe, AbstractInsnNode targetLocation, ReturnContext returnContext) {
if (metricProbe.getValue() == null) {
InsnList insnList = new InsnList();
ldc(insnList, metricProbe.getProbeId().getEncodedId());
Expand All @@ -246,10 +249,11 @@ private InsnList callCount(MetricProbe metricProbe, ReturnContext returnContext)
// stack []
return insnList;
}
return internalCallMetric(metricProbe, returnContext);
return internalCallMetric(metricProbe, targetLocation, returnContext);
}

private InsnList internalCallMetric(MetricProbe metricProbe, ReturnContext returnContext) {
private InsnList internalCallMetric(
MetricProbe metricProbe, AbstractInsnNode targetLocation, ReturnContext returnContext) {
InsnList insnList = new InsnList();
InsnList nullBranch = new InsnList();
VisitorResult result;
Expand All @@ -259,7 +263,7 @@ private InsnList internalCallMetric(MetricProbe metricProbe, ReturnContext retur
metricProbe
.getValue()
.getExpr()
.accept(new MetricValueVisitor(this, nullBranch, returnContext));
.accept(new MetricValueVisitor(this, nullBranch, targetLocation, returnContext));
} catch (InvalidValueException | UnsupportedOperationException ex) {
reportError(ex.getMessage());
return EMPTY_INSN_LIST;
Expand Down Expand Up @@ -320,21 +324,22 @@ private Type convertIfRequired(Type currentType, InsnList insnList) {
}
}

private InsnList callMetric(MetricProbe metricProbe) {
return callMetric(metricProbe, null);
private InsnList callMetric(MetricProbe metricProbe, AbstractInsnNode targetLocation) {
return callMetric(metricProbe, targetLocation, null);
}

private InsnList callMetric(MetricProbe metricProbe, ReturnContext returnContext) {
private InsnList callMetric(
MetricProbe metricProbe, AbstractInsnNode targetLocation, ReturnContext returnContext) {
switch (metricProbe.getKind()) {
case COUNT:
return callCount(metricProbe, returnContext);
return callCount(metricProbe, targetLocation, returnContext);
case GAUGE:
case HISTOGRAM:
case DISTRIBUTION:
if (metricProbe.getValue() == null) {
return EMPTY_INSN_LIST;
}
return internalCallMetric(metricProbe, returnContext);
return internalCallMetric(metricProbe, targetLocation, returnContext);
default:
reportError(String.format("Unknown metric kind: %s", metricProbe.getKind()));
}
Expand Down Expand Up @@ -363,11 +368,11 @@ private InstrumentationResult.Status addLineMetric(ClassFileLines classFileLines
"No line info for " + (sourceLine.isSingleLine() ? "line " : "range ") + sourceLine);
}
if (beforeLabel != null) {
InsnList insnList = wrapTryCatch(callMetric(metricProbe));
InsnList insnList = wrapTryCatch(callMetric(metricProbe, beforeLabel));
methodNode.instructions.insertBefore(beforeLabel.getNext(), insnList);
}
if (afterLabel != null && !sourceLine.isSingleLine()) {
InsnList insnList = wrapTryCatch(callMetric(metricProbe));
InsnList insnList = wrapTryCatch(callMetric(metricProbe, afterLabel));
methodNode.instructions.insert(afterLabel, insnList);
}
}
Expand All @@ -387,12 +392,17 @@ public VisitorResult(ASMHelper.Type type, InsnList insnList) {
private class MetricValueVisitor implements Visitor<VisitorResult> {
private final MetricInstrumentor instrumentor;
private final InsnList nullBranch;
private final AbstractInsnNode targetLocation;
private final ReturnContext returnContext;

public MetricValueVisitor(
MetricInstrumentor instrumentor, InsnList nullBranch, ReturnContext returnContext) {
MetricInstrumentor instrumentor,
InsnList nullBranch,
AbstractInsnNode targetLocation,
ReturnContext returnContext) {
this.instrumentor = instrumentor;
this.nullBranch = nullBranch;
this.targetLocation = targetLocation;
this.returnContext = returnContext;
}

Expand Down Expand Up @@ -709,12 +719,14 @@ private ASMHelper.Type tryRetrieveArgument(String head, InsnList insnList) {
private ASMHelper.Type tryRetrieveLocalVar(String head, InsnList insnList) {
for (LocalVariableNode varNode : instrumentor.methodNode.localVariables) {
if (varNode.name.equals(head)) {
Type varType = Type.getType(varNode.desc);
VarInsnNode varInsnNode =
new VarInsnNode(varType.getOpcode(Opcodes.ILOAD), varNode.index);
insnList.add(varInsnNode);
// stack [local]
return new ASMHelper.Type(varType);
if (isInScope(instrumentor.methodNode, varNode, targetLocation)) {
Type varType = Type.getType(varNode.desc);
VarInsnNode varInsnNode =
new VarInsnNode(varType.getOpcode(Opcodes.ILOAD), varNode.index);
insnList.add(varInsnNode);
// stack [local]
return new ASMHelper.Type(varType);
}
}
}
return null;
Expand Down
24 changes: 24 additions & 0 deletions ...nt-debugger/src/test/java/com/datadog/debugger/agent/MetricProbesInstrumentationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import static org.junit.jupiter.api.Assertions.assertArrayEquals;
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.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;
import static org.mockito.ArgumentMatchers.any;
Expand All @@ -17,6 +18,7 @@
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static utils.InstrumentationTestHelper.compileAndLoadClass;
import static utils.InstrumentationTestHelper.loadClass;

import com.datadog.debugger.el.DSL;
import com.datadog.debugger.el.ValueScript;
Expand Down Expand Up @@ -1194,6 +1196,27 @@ public void primitivesFunction() throws IOException, URISyntaxException {
assertEquals(97, listener.counters.get(METRIC_NAME_CHAR));
}

@Test
public void localVarNotInScope() throws IOException, URISyntaxException {
final String METRIC_NAME = "lenstr";
final String CLASS_NAME = "com.datadog.debugger.jaxrs.MyResource";
MetricProbe metricProbe =
createMetricBuilder(METRIC_ID, METRIC_NAME, GAUGE)
.where(CLASS_NAME, "createResource", null)
.valueScript(new ValueScript(DSL.len(DSL.ref("varStr")), "len(varStr)"))
.build();
MetricForwarderListener listener = installMetricProbes(metricProbe);
Class<?> testClass =
loadClass(CLASS_NAME, getClass().getResource("/MyResource.class").getFile());
Object result =
Reflect.onClass(testClass)
.create()
.call("createResource", (Object) null, (Object) null, 1)
.get();
assertNotNull(result);
assertFalse(listener.gauges.containsKey(METRIC_NAME));
}

private MetricForwarderListener installSingleMetric(
String metricName,
MetricProbe.MetricKind metricKind,
Expand Down Expand Up @@ -1273,6 +1296,7 @@ private MetricForwarderListener installMetricProbes(Configuration configuration)
Config config = mock(Config.class);
when(config.isDebuggerEnabled()).thenReturn(true);
when(config.isDebuggerClassFileDumpEnabled()).thenReturn(true);
when(config.isDebuggerVerifyByteCode()).thenReturn(true);
when(config.getFinalDebuggerSnapshotUrl())
.thenReturn("http://localhost:8126/debugger/v1/input");
when(config.getFinalDebuggerSymDBUrl()).thenReturn("http://localhost:8126/symdb/v1/input");
Expand Down
Loading