Add support to trace defer function calls under trace follow option #3978
Conversation
archanaravindar
force-pushed
the
tracedefer
branch
from
4d7a687 to
5dc7e54
Compare
There was a problem hiding this comment.
I don't really get what this PR is trying to achieve. Right now the trace with depth thing is determining statically from the call graph which functions to trace. This PR seems to change that so that deferred functions are added, at runtime, to the traced functions. Why are we doing this and why only deferred functions and not any other kind of dynamic dispatch?
archanaravindar
force-pushed
the
tracedefer
branch
from
2fa0860 to
ba6f2b3
Compare
|
Hi @aarzilli Thank you for reviewing the patch in detail and also suggesting improvements to optimize the code |
Defers are an explicit feature of Go just like function variables and interfaces, I'm not even sure that they really are the most common type of dynamic dispatch compared to the others (or by how much if they are), but even if they were I'm not sure it's enough to single them out. Or even how you would explain that to users, i.e. this changes the behavior of |
archanaravindar
force-pushed
the
tracedefer
branch
from
ba6f2b3 to
84335b4
Compare
Thank you for the detailed comment @aarzilli, it appears that with minimal modifications the code can trace functions that are dynamically passed as procedure parameters or returned from functions. Made changes to documentation and added couple of test cases for the same, kindly let us know your thoughts on this, Thanks again for your time on this! |
|
Sorry to not have reviewed this yet, but I have been busy this past month and I will continue to be busy for a while still. |
archanaravindar
force-pushed
the
tracedefer
branch
2 times, most recently
from
61e5bdf to
0ddb9bc
Compare
archanaravindar
force-pushed
the
tracedefer
branch
2 times, most recently
from
59936a7 to
57207b5
Compare
archanaravindar
force-pushed
the
tracedefer
branch
from
57207b5 to
e0de46e
Compare
…return first level only
more defer cases
…ntegration_test.go addressed compilation errors in debugger.go
…stination function address in CALL Reg instruction
archanaravindar
force-pushed
the
tracedefer
branch
from
e0de46e to
7241250
Compare
service/debugger/debugger.go
Outdated
| // createBreakpointForTracepoints creates a logical breakpoint specifically for tracepoints. | ||
| // It handles the BreakpointExistsError by silently ignoring it, as duplicate tracepoints | ||
| // are expected during dynamic function discovery. | ||
| func (d *Debugger) createBreakpointForTracepoints(lbp *proc.LogicalBreakpoint) error { |
There was a problem hiding this comment.
This is only ever called by the function below, so maybe we should just create a function variable in the below function like:
tryEnableBreakpoint := func(lbp *proc.LogicalBreakpoint) error { ... }
service/debugger/debugger.go
Outdated
| // createFnTracepoint is a way to create a trace point late in the cycle but just in time that it can be | ||
| // included in the trace output as in the case of dynamic functions, we get to know the functions that are | ||
| // being called from deferreturn quite late in execution | ||
| func createFnTracepoint(d *Debugger, fname string, rootstr string, followCalls int) (*api.Breakpoint, error) { |
There was a problem hiding this comment.
Sorry to be pedantic, but technically this creates multiple breakpoints (entry, exit), so maybe we rename this to createFunctionTracepoints (note plural).
There was a problem hiding this comment.
Also, it doesn't look like any of the callers of this function use the returned *api.Breakpoint so let's just return an error from this function. If callers did need the created breakpoint, it would be better to return all the breakpoints we created, I think (including return tracepoints).
3 participants
Is an extension of the basic support added to mention depth upto which functions and their descendants need to be traced #3594
The same option can now trace defer functions and their descendants
Fixes #3743