Add inline function probing #3095

ttreyer · 2024-03-28T19:47:48Z

Inserting a probe before the stack frame has been established can lead to issues when collecting the stack.
There might be missing frames. See the runtime test call.ustack_elf_symtable where test2 is missing.

When inserting the probe after the prologue of the function, the stack walker has a better context and won't miss an entry.

Checklist

Language changes are updated in man/adoc/bpftrace.adoc
User-visible and non-trivial changes updated in CHANGELOG.md
The new behaviour is covered by tests

ttreyer · 2024-03-30T14:50:54Z

I had to change the ARG0_STACK from 8 to 16 for x86_64, since the probe stops after the push rbp instruction.
See System V ABI, page 21.

I still need to check the offset for other archs:

danobi · 2024-04-01T17:26:47Z

Sweet! This is an excellent fix. Haven't had a chance to look at the code yet; hope to look at the base PR today/tomorrow

tests/testprogs/CMakeLists.txt

tests/runtime/dwarf

src/dwarf_parser.h

src/bpftrace.cpp

ttreyer · 2024-04-05T13:29:50Z

Clang and GCC don't emit code with the same ABI: Godbolt. Looking at the dwarfdump, Clang's stack args are at the stack pointer + 16, but GCC stack args are at the stack pointer + 0. So I can't make the stack args test consistent between the two compilers. Is there an option or a function attribute that might force GCC to set the offset to 16?

We also discussed about sargX with @ajor and concluded it typically won't return the correct argument for other languages like Go. We also noticed that kprobe/kfunc have a maximum of 6 parameters, which fits in the AMD64 registers and the arguments never need to go on the stack. So we were wondering if sarg should remain in its current form.

In a later PR, we could ensure correct access to stack arguments by using the location expression emitted for the debugger.

viktormalik · 2024-04-08T13:18:17Z

We also discussed about sargX with @ajor and concluded it typically won't return the correct argument for other languages like Go.

Yes, Go uses their own calling conventions and ignores System V ABI (on x86). I'm not sure what we can do about it other than detecting that a uprobe is inside a Go program and issuing a warning or disabling thesargX builtin completely for such a case.

We also noticed that kprobe/kfunc have a maximum of 6 parameters, which fits in the AMD64 registers and the arguments never need to go on the stack. So we were wondering if sarg should remain in its current form.

FYI, kernel fprobes (i.e. our kfunc) now support up to 12 arguments so sargX could make sense for those (though they can by accessed by name).

viktormalik · 2024-04-26T12:47:27Z

But, if the user doesn't have DWARF symbols for his binary, knows the ABI, and wants to see values of stack-passed args, why not let him do it, especially since we already have the feature implementated?

Because I think it's going to get broken by this PR and we have no way of testing it across architectures 😅

Ah, I somehow missed that. In that case, I'm all in for dropping the feature :-) Sorry for the confusion and thanks for clearing it up!

And even if it did work correctly, I don't think it's a particularly useful feature because of how much knowledge of the application's ABI it requires in order to use (it was originally intended for Golang were sarg0 corresponded to the first argument). Extending arg to read stack arguments after the register arguments might be more useful than sarg.

That sounds like a reasonable approach. Maybe we could open an issue to track that.

ttreyer · 2024-04-29T18:11:51Z

Ah, I somehow missed that. In that case, I'm all in for dropping the feature :-) Sorry for the confusion and thanks for clearing it up!

Following that, I marked sarg as deprecated and removed its tests, since this PR breaks them by skipping the function prologue.

danobi

lgtm!

danobi · 2024-05-04T18:07:43Z

tests/runtime/builtin

Maybe it'd be good to leave at least one or two of these in the new form to document/check that stack accesses don't regress?

CHANGELOG.md

tests/runtime/dwarf

ajor

Just thought of one more changelog entry we should include: "Deprecated sarg".

Other than that, looks great and I think it's ready to merge!

CHANGELOG.md

viktormalik

I'd still prefer if we could avoid waiting the entire timeout for the few runtime tests but it's not a hard blocker for me.

LGTM otherwise, nice work!

tests/runtime/dwarf

viktormalik · 2024-05-13T07:24:09Z

@danobi @jordalgo do you want to have one more look? Otherwise we can merge.

jordalgo · 2024-05-13T12:27:42Z

tests/runtime/dwarf

+PROG config = { probe_inline = 0 }
+     uprobe:./testprogs/inline_function:square { @count++ }
+     uretprobe:./testprogs/inline_function:main { exit() }
+EXPECT @count: 1


I guess bpftrace initializes this map value to 1 even though it never gets called?

No, it should be initialized to 0. The test checks that only one (non-inlined) instance of square is hit (see the inline_function.c source).

Ah it's because the last call to square is not inlined. Is that worth a comment in this file as well to clear up possible confusion?

tests/testprogs/inline_function.c

tests/semantic_analyser.cpp

Use the DebugInfo (DWARF) to locate every inlined instances of a uprobe function and attach to all of them. This behaviour is controled by the `probe_inline` config key. Note that using this config disables access to the arguments, because we cannot guarantee inlined function instances follow the System V ABI. Also attach to uprobe functions after their prologue, once the stack frame has been set, which makes collecting stack traces more reliable. This behaviour is the new default for all binaries with DWARF.

jordalgo

LGTM!

ajor · 2024-05-14T13:49:05Z

Let's go! 🚀

Explicitly put numbers into an array on the stack instead of relying on architecture-specific calling conventions for stack-passed parameters. This removes the dependency on the stack pointer's value (which changed as of bpftrace#3095) and makes the tests architecture independent.

Explicitly put numbers into an array on the stack instead of relying on architecture-specific calling conventions for stack-passed parameters. This removes the dependency on the stack pointer's value (which changed as of #3095) and makes the tests architecture independent.

ttreyer requested review from ajor, viktormalik, danobi and fbs as code owners March 28, 2024 19:47

ttreyer force-pushed the skip-prologue branch 2 times, most recently from fcef4fa to 974b393 Compare March 30, 2024 14:36

ttreyer force-pushed the skip-prologue branch from 974b393 to b82fd29 Compare March 30, 2024 16:20

ttreyer force-pushed the skip-prologue branch from b82fd29 to 30db54c Compare April 2, 2024 17:25

ttreyer changed the title ~~Skip function prologue for uprobes~~ Add inline function probing Apr 2, 2024

jordalgo reviewed Apr 2, 2024

View reviewed changes

tests/testprogs/CMakeLists.txt Outdated Show resolved Hide resolved

jordalgo reviewed Apr 2, 2024

View reviewed changes

tests/runtime/dwarf Outdated Show resolved Hide resolved

jordalgo reviewed Apr 2, 2024

View reviewed changes

src/dwarf_parser.h Outdated Show resolved Hide resolved

jordalgo reviewed Apr 2, 2024

View reviewed changes

src/bpftrace.cpp Outdated Show resolved Hide resolved

ttreyer force-pushed the skip-prologue branch 6 times, most recently from 9ece794 to c2ee339 Compare April 4, 2024 17:08

ttreyer force-pushed the skip-prologue branch 6 times, most recently from 5288112 to b8e90ce Compare April 15, 2024 14:34

ttreyer marked this pull request as draft April 15, 2024 15:00

ttreyer force-pushed the skip-prologue branch 4 times, most recently from a80023e to 259688b Compare April 29, 2024 18:08

ttreyer force-pushed the skip-prologue branch from 259688b to e990c50 Compare April 30, 2024 14:53

danobi reviewed May 4, 2024

View reviewed changes

ttreyer force-pushed the skip-prologue branch from e990c50 to 74095b8 Compare May 8, 2024 13:43

ajor approved these changes May 8, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

viktormalik approved these changes May 9, 2024

View reviewed changes

tests/runtime/dwarf Outdated Show resolved Hide resolved

ttreyer force-pushed the skip-prologue branch 2 times, most recently from c202659 to a3caf64 Compare May 10, 2024 16:23

ttreyer commented May 10, 2024

View reviewed changes

tests/runtime/dwarf Show resolved Hide resolved

viktormalik approved these changes May 13, 2024

View reviewed changes

jordalgo reviewed May 13, 2024

View reviewed changes

tests/testprogs/inline_function.c Show resolved Hide resolved

jordalgo reviewed May 13, 2024

View reviewed changes

tests/semantic_analyser.cpp Show resolved Hide resolved

ttreyer force-pushed the skip-prologue branch from a3caf64 to 4267ede Compare May 13, 2024 15:34

jordalgo approved these changes May 13, 2024

View reviewed changes

ajor linked an issue May 13, 2024 that may be closed by this pull request

ustack missing stack frame second from top #646

Closed

ajor merged commit 742f0cf into bpftrace:master May 14, 2024
18 checks passed

ajor mentioned this pull request May 15, 2024

Fix intptrcast runtime tests #3166

Merged

viktormalik mentioned this pull request May 16, 2024

uprobe DWARF location attachment does not work with uprobe_multi #3173

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add inline function probing #3095

Add inline function probing #3095

ttreyer commented Mar 28, 2024 •

edited

Loading

ttreyer commented Mar 30, 2024 •

edited

Loading

danobi commented Apr 1, 2024

ttreyer commented Apr 5, 2024 •

edited

Loading

viktormalik commented Apr 8, 2024

viktormalik commented Apr 26, 2024

ttreyer commented Apr 29, 2024

danobi left a comment

danobi May 4, 2024

ajor left a comment

viktormalik left a comment

viktormalik commented May 13, 2024

jordalgo May 13, 2024

viktormalik May 13, 2024

jordalgo May 13, 2024

jordalgo left a comment

ajor commented May 14, 2024

Add inline function probing #3095

Add inline function probing #3095

Conversation

ttreyer commented Mar 28, 2024 • edited Loading

Checklist

ttreyer commented Mar 30, 2024 • edited Loading

danobi commented Apr 1, 2024

ttreyer commented Apr 5, 2024 • edited Loading

viktormalik commented Apr 8, 2024

viktormalik commented Apr 26, 2024

ttreyer commented Apr 29, 2024

danobi left a comment

Choose a reason for hiding this comment

danobi May 4, 2024

Choose a reason for hiding this comment

ajor left a comment

Choose a reason for hiding this comment

viktormalik left a comment

Choose a reason for hiding this comment

viktormalik commented May 13, 2024

jordalgo May 13, 2024

Choose a reason for hiding this comment

viktormalik May 13, 2024

Choose a reason for hiding this comment

jordalgo May 13, 2024

Choose a reason for hiding this comment

jordalgo left a comment

Choose a reason for hiding this comment

ajor commented May 14, 2024

ttreyer commented Mar 28, 2024 •

edited

Loading

ttreyer commented Mar 30, 2024 •

edited

Loading

ttreyer commented Apr 5, 2024 •

edited

Loading