Skip to content

Fix incorrect argument name for already traced methods#4630

Merged
jpbempel merged 1 commit into
masterfrom
jpbempel/fix-localvar-names-with-tracer
Feb 1, 2023
Merged

Fix incorrect argument name for already traced methods#4630
jpbempel merged 1 commit into
masterfrom
jpbempel/fix-localvar-names-with-tracer

Conversation

@jpbempel

@jpbempel jpbempel commented Jan 31, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

What Does This Do

The fix consists to reassigned local var information to arg slot.
Added smoke tests for this case.

Motivation

When instrumenting a method with a debugger probe that contains also tracer instrumentation (using @trace annotation or trace.methods property) argument names was not mapped correctly and p0, p1, appears in the snapshot.
This is due to ByteBuddy transformation messing up the LocalVariableTable that helps to maps local vars with names. Args are also local vars and following the JVM spec, the first local vars must match the methods args.
LocalVariableTable is optional and only used for debugger so it does not prevent the actual execution of the method.

Additional Notes

Example with petclinic:

VetController.processWithArg method:

      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0       8     0  this   Lorg/springframework/samples/petclinic/vet/VetController;
            0       8     1   obj   Ljava/lang/Object;

When we start with -Ddd.trace.methods=org.springframework.samples.petclinic.vet.VetController[processWithArg]

      LocalVariableTable:
        Start  Length  Slot  Name   Signature
          186      11     0  this   Lorg/springframework/samples/petclinic/vet/VetController;
          186      11     4   obj   Ljava/lang/Object;

This case works beside the JVM specification, because the slot 1 is still assigned to the parameter, instrumented bytcode do some dance with it to conform with the new LocalVariableTable:

       220: aload_1
       221: astore        4
       223: nop
       224: getstatic     #16                 // Field java/lang/System.out:Ljava/io/PrintStream;
       227: aload         4
       229: invokevirtual #52                 // Method java/io/PrintStream.println:(Ljava/lang/Object;)V

Before calling the original code System.out.println(obj)
we load from slot 1 the actual parameter of the method (obj) into the stack and store it back into slot 4 to match the LocalVariableTable

@jpbempel
Fix incorrect arg name for already traced methods
ba23134 
When instrumenting a method with a debugger probe that contains also
tracer instrumentation (using @trace annotation or
`trace.methods` property) argument names was not mapped correctly and
p0, p1, appears in the snapshot.
This is due to ByteBuddy transformation messing up the
LocalVariableTable that helps to maps local vars with names. Args
are also local vars and following the JVM spec, the first local vars
must match the methods args.
LocalVariableTable is optional and only used for debugger so it does
not prevent the actual execution of the method.
The fix consists to reassigned local var information to arg slot.
Added smoke tests for this case.
@jpbempel
jpbempel requested a review from a team as a code owner January 31, 2023 17:14
@jpbempel
jpbempel requested review from cimi and removed request for a team January 31, 2023 17:14
@jpbempel jpbempel added the comp: debugger Dynamic Instrumentation label Jan 31, 2023
shatzi
shatzi approved these changes Jan 31, 2023
View reviewed changes

@shatzi shatzi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome stuff JP.
Maybe worth creating a ticket of dealing with variables sharing slots as it would be different name depend on the location of the method.

Comment thread ...va-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/Instrumentor.java
LocalVariableNode[] localVars = new LocalVariableNode[maxIndex + 1];
localVarBaseOffset = sortedLocalVars.get(0).index;
for (LocalVariableNode localVariableNode : sortedLocalVars) {
localVars[localVariableNode.index] = localVariableNode;

@shatzi shatzi Jan 31, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if multiple localVariables uses the same slot (I think you showed me an example for that)?

Maybe we should also look at the variable range so get variable name would be correct depend on the line (offset) we are instrumenting.

@jpbempel jpbempel Jan 31, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear the localVarBySlot address only the problematic of args. which does not share slot with other since they are scoped for the entire method.
It replaces the argumentNames field previously

for pure local vars see

dd-trace-java/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/LogInstrumentor.java

Lines 326 to 332 in c42d248

List<LocalVariableNode> applicableVars = new ArrayList<>();
for (LocalVariableNode variableNode : methodNode.localVariables) {
int idx = variableNode.index - localVarBaseOffset;
if (idx >= argOffset && isInScope(variableNode, location)) {
applicableVars.add(variableNode);
}
}

and isInScope method:

dd-trace-java/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/LogInstrumentor.java

Lines 610 to 624 in c42d248

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;
}

@shatzi shatzi Jan 31, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome. thanks for clearing this out. looks good.

@jpbempel
jpbempel merged commit 5dbeb68 into master Feb 1, 2023
@jpbempel
jpbempel deleted the jpbempel/fix-localvar-names-with-tracer branch February 1, 2023 15:18
@jpbempel jpbempel changed the title Fix incorrect arg name for already traced methods Fix incorrect argument name for already traced methods Feb 1, 2023
@github-actions github-actions Bot added this to the 1.7.0 milestone Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cimi cimi Awaiting requested review from cimi cimi is a code owner automatically assigned from DataDog/debugger-java
1 more reviewer
@shatzi shatzi shatzi approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

comp: debugger Dynamic Instrumentation

Projects

None yet

Milestone

1.7.0

Development

Successfully merging this pull request may close these issues.

2 participants

@jpbempel @shatzi