-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
libprobe: Add a probe! macro for static instrumentation #14031
Conversation
This adds a new libprobe with a probe! macro, for adding static instrumentation to any rust crate. Please see the docstrings for simple probe examples and how they might be used by tools. This initial version only implements SystemTap SDT, with a voided macro for other platforms, so probes can be freely written anywhere. I believe the macro is generic enough that a DTrace USDT probe! could be implemented for freebsd and macos too.
//! intermediate total. | ||
//! | ||
//! ```rust | ||
//! #![feature(phase)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this have to opt-in to the asm
feature as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mentioned that before, but this seems to work fine as-is. I've tried it in a separate file, and rustdoc --test
works too. Hopefully this is not just an accident, because it will be nicer if users don't need to know it's asm
underneath.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I forgot about #12122!
It's unclear how much of a bug that is, but this is an experimental crate so it's ok to be susceptible to breakage.
This looks good to me. Ideally, we should have tests, but even if this PR contained tests, before enabling them SystemTap would need to be installed to Linux buildbots etc. I am fine with adding tests later, as long as they are added. |
Do those buildbots have at least GDB 7.5? I see that src/test/debuginfo does a lot with GDB, and I could probably make similar tests for |
I've been thinking about this, and I'm wary to starting including a crate in the standard rust distribution that's only supported on linux/android, and also doesn't have many tests to prevent it from regressing. I think this is a fantastic crate and it's very well written, but it may not be the right time to include in the standard distribution. I would love to see this on the wiki page and rust-ci, however! |
It's sort of a weird case, because the "normal" behavior of a On the testing point, I was hoping for an answer whether GDB 7.5 can be relied on in the buildbots, then I'm happy to write tests targeting that. Or if only some buildbots have GDB 7.5+, maybe there's a way to mark the tests ignored on others. If GDB If you still think it's not suitable, then I'll gladly publish it separately, but I think there's more value to having this included in standard rust, and little-to-no downside. The worst case for rust is that it might produce SDT metadata that the tools can't understand, but rust programs will still work regardless. |
I agree that this shouldn't be blocked solely based on the fact that it's only implemented for linux, but this is also a library that hasn't had much time to bake in the community to see how it works out and how it gets used. This is a perfect candidate for a cargo package to be super easy to install, but cargo is still in the works at this time. For now, I think it's best to publish separately to get some experience with it getting used in the community. If this turns out to be the de-facto standard used throughout rust libraries, then I think it'd be a great fit for being part of the distribution. Also, sorry, I thought I commented (bad memory), but the bots don't have GDB 7.5 right now. They're all on 7.4 or below. We could update them if necessary, however. If it's ok with you, I'm going to close this for now, but I highly encourage you to publish in your own repo. This is a high-quality library which definitely deserves attention! |
@cuviper did you look into publishing it as your own repo? |
This adds a new libprobe with a probe! macro, for adding static
instrumentation to any rust crate. Please see the docstrings for simple
probe examples and how they might be used by tools.
This initial version only implements SystemTap SDT, with a voided macro
for other platforms, so probes can be freely written anywhere. I
believe the macro is generic enough that a DTrace USDT probe! could be
implemented for freebsd and macos too.
This is related to issue #6816 requesting DTrace USDT, although I only implemented SystemTap SDT. I enabled this for both Linux and Android target_os, although I only tested the former. Since it's simply void everywhere else, it's safe to insert probes in any code.
Please let me know if anything needs to change, and I'm also happy to answer questions about debugging with SDT. Thanks!