Page MenuHomeFreeBSD

kinst: add kinst_excluded()
ClosedPublic

Authored by christos on Mar 23 2023, 11:20 AM.
Tags
Referenced Files
Unknown Object (File)
Sat, Jun 8, 10:20 AM
Unknown Object (File)
Tue, Jun 4, 11:55 AM
Unknown Object (File)
Thu, May 23, 7:51 PM
Unknown Object (File)
Wed, May 22, 8:13 AM
Unknown Object (File)
Thu, May 16, 1:25 PM
Unknown Object (File)
Thu, May 16, 1:25 PM
Unknown Object (File)
Thu, May 16, 1:25 PM
Unknown Object (File)
Thu, May 16, 1:25 PM
Subscribers

Details

Reviewers
markj
Summary

Exclude functions that are not safe-to-trace.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 51707
Build 48598: arc lint + arc unit

Event Timeline

This revision is now accepted and ready to land.Mar 23 2023, 1:55 PM

No longer search for push %rbp, just exclude the exception handlers.
This way we can trace safe functions that do not push %rbp, such as
cpu_switch().

This revision now requires review to proceed.Mar 24 2023, 8:19 PM

Surround amd64 exception handlers in an ifdef.

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

Add RISC-V exception handlers.

Remove unused KINST_PUSHL_RBP define.

christos retitled this revision from kinst(4): add kinst_excluded() to kinst: add kinst_excluded().Apr 29 2023, 10:21 AM

Exclude unsafe arm64 exception handlers.

So we should certainly exclude dtrace_* like FBT does. Same for unwind_frame(). I'd suggest posting that as its own patch, since it's needed independent of whether we remove the push %rbp check.

Right now we still have the check for push %rbp at the beginning of each function, which covers most of the rest. Where exactly did we land on that? I know that there are traceable functions where push %rbp is not the first instruction, but perhaps there's another way to handle them?

Right now we still have the check for push %rbp at the beginning of each function, which covers most of the rest. Where exactly did we land on that? I know that there are traceable functions where push %rbp is not the first instruction, but perhaps there's another way to handle them?

I think it's better to not check for push %rbp at all, so we do not ignore safe to trace functions that just have push %rbp after the first instruction, or do not have it at all but are safe to trace, and instead exclude functions that are known to be unsafe-to-trace. I tested this patch with
quite a lot of functions that do not have push %rbp (https://github.com/christosmarg/omitrbp) and haven't gotten any crash so far with this exclusion list.

I'd suggest posting that as its own patch, since it's needed independent of whether we remove the push %rbp check.

I'm not sure these 2 changes are unrelated. Basically the push %rbp check was our "exclusion" mechanism, now replaced by kinst_excluded().

I'd suggest posting that as its own patch, since it's needed independent of whether we remove the push %rbp check.

I'm not sure these 2 changes are unrelated. Basically the push %rbp check was our "exclusion" mechanism, now replaced by kinst_excluded().

The "push %rbp" check isn't sufficient though. Today you can do this: dtrace -n 'kinst::dtrace_probe:0'. This doesn't work very well.

I would like to at least fix that problem before reconsidering the push %rbp check.

The "push %rbp" check isn't sufficient though. Today you can do this: dtrace -n 'kinst::dtrace_probe:0'. This doesn't work very well.

I would like to at least fix that problem before reconsidering the push %rbp check.

Okay, I can split it into two patches.

Get rid of the 'push rbp' removal in this patch.

Remove amd64 exception handlers.

Add kinst_md_excluded() as well.

sys/cddl/dev/kinst/kinst.c
87

This is handled by the push rbp check. Only amd64 has that check currently, but I think riscv should perform that as well, see my comments in that review.

christos marked an inline comment as done.

Remove cpu_switch() from kinst_excluded().

This revision is now accepted and ready to land.May 26 2023, 1:51 PM

Closed by commit 9c80ad6839cd30ecfeff2fb946d86888815da600, but had to do it manually because the commit message ate up the 'D' from 'Differential Revision'...