Page MenuHomeFreeBSD

libdtrace: Implement inline function tracing for kinst
Needs RevisionPublic

Authored by christos on Feb 28 2023, 5:10 PM.
Tags
Referenced Files
F115416420: D38825.id126567.diff
Wed, Apr 23, 2:17 PM
F115413045: D38825.id118255.diff
Wed, Apr 23, 1:26 PM
F115374724: D38825.id119059.diff
Wed, Apr 23, 3:31 AM
F115374174: D38825.diff
Wed, Apr 23, 3:21 AM
Unknown Object (File)
Tue, Apr 22, 4:22 AM
Unknown Object (File)
Mon, Apr 21, 10:52 PM
Unknown Object (File)
Mon, Apr 21, 4:58 PM
Unknown Object (File)
Mon, Apr 21, 4:58 PM

Details

Summary

The following code implements inline tracing for the kinst DTrace
provider.

Briefly explained, what the code does is, for each probe of the form:

kinst::<inline_func>:<entry/return>

libdtrace checks to see if <inline_func> is indeed an inline function,
where in this case it finds all inline copies of it and creates new
probes for each one of them. Otherwise, it converts the probe to an FBT
one so that we don't duplicate FBT's functionality in kinst.

Notes:

Unlike FBT, leaving the probe name empty won't trace both entry and
return. kinst goes into "inline-tracing mode" only when it sees
entry/return in the probe name, otherwise it thinks it's a regular kinst
probe, in which case the dtrace(1) invocation will fail if we try to
trace an inline function with an empty probe name.

Sponsored by: The FreeBSD Foundation
MFC after: 1 week

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

update dtrace_kinst.4 man page to mention inline tracing

remove call to dwarf_next_types_section()

Handle leaf functions with their push %rbp ommitted.

christos edited the summary of this revision. (Show Details)

Got rid of deep copying, appending to the main clause is simpler.

Handle multiple dn_pdescs properly, move handling away from
dt_sugar_visit_all().

christos edited the summary of this revision. (Show Details)

Do not always go back one instruction.

Improve example in man page.

Get rid of push %rbp check (depends on D39166).

christos edited the summary of this revision. (Show Details)

Parse all loaded modules and their debug files instead of just kernel.
Mention new bug in summary.

christos edited the summary of this revision. (Show Details)

Fix bugs, surround amd64-specific code in an #ifdef, merge flags into
dtsp_flags, handle dt_sugar_elf_init() and dwarf_elf_init() errors, use
longjmp() for dt_alloc() failures.

Fix per-module parsing. Create an additional FBT probe if a non-inline definition is
found along an inline definition in the same module.

More bug fixes. I always keep finding them after I submit the patch.

Do not exclude inline copies that have DW_AT_subprogram instead of
DW_AT_inlined_subroutine.

Add tests. kinst_test_inline() needs modification so that it can be splitted
into DW_AT_ranges.

Implement dt_sugar_elf_verify_debuglink() to make sure the debug files we're
reading are up to date, and if they're not, skip them. Also stop leaking
elf_info structs in dt_sugar_do_kinst_inline().

Fix accidental dangling brace.

Surround amd64-specific code in an ifdef.

Use dtrace_instr_size() and get rid of machine-dependent code (depends on
D39489).

Fix Makefile conflict, check if dtracetest is already loaded before attempting to load it.

Add libdwarf in Makefile.inc1 (Thanks Mark).

Bail out if kinst doesn't exist (i.e the architecture doesn't support it).

Fix return offset handling.

Fix off->val type causing a build failure.

sys/cddl/dev/dtrace/dtrace_test.c
90 ↗(On Diff #127670)

@markj do you have a good idea for this?

domagoj.stolfa_gmail.com added inline comments.
cddl/contrib/opensolaris/lib/libdtrace/common/dt_sugar.c
874

I don't see any other uses of cont, perhaps this can be done without using a goto?

cddl/contrib/opensolaris/lib/libdtrace/common/dt_sugar.c
873–877

I don't see any other uses of cont, perhaps this can be done without using a goto?

I guess it could be rewritten like this.

christos retitled this revision from libdtrace: implement inline function tracing for kinst to libdtrace: Implement inline function tracing for kinst.Jan 8 2025, 2:04 PM
christos edited the summary of this revision. (Show Details)
christos marked an inline comment as done.
  • Remove unncessary goto.
  • Rebase on top of main.

tinderbox build on the cluster done with no errors.

After some testing, this generally works fine for me. As per our chat on IRC, one thing I'd quite like before this lands are two additional tests in dtrace_test, one for a simple tail call and one for tail recursion which is unrolled into a loop (both should be achievable via __attribute__((musttail)), though I'm not 100% clear on how to ensure that you get the desired loop unrolling behaviour for recursion. See https://clang.llvm.org/docs/AttributeReference.html#musttail for more details). Even if they don't necessarily pass, I think it's OK to land this and simply fix the tail calls later as the state of things with this change would still improve over what's currently there.

This revision now requires changes to proceed.Tue, Apr 15, 3:08 PM

I will spend some time testing this again within a week. My recollection from last time is that there were some edge cases where probes didn't fire when I expected. One thing I didn't test yet but am a bit suspicious will not work properly is stack unwinding -- did you test that the stack() action prints reasonable output when used within an inline function probe?

There is no documentation for the new functionality; dtrace_kinst.4 needs to be updated.

There is no documented "theory of operation" for this functionality, either in the commit log or in the comments (the latter would be much better). There should be a high-level description of how this actually works, as in, how do we actually find the entry and return instructions. As far as I understand, the algorithm is basically just a generalization of some examples drawn from the kernel. As such, I suspect that over time we'll find examples where it doesn't work, and have to add more complexity to handle them. That will be quite difficult without documentation and comprehensive test cases. That maintenance burden is part of why I'm quite hesitant about adding this feature as it is.

This functionality will be much more useful if we can get inline function arguments somehow, but I don't know how far away that is. What would be involved in adding support for that?