Add FreeBSD support #63
Conversation
On some implementation of `dladdr`, the `dli_sname` and `dli_fname` can be null even if the `dladdr` itself succeed.
FreeBSD implemented DTrace fairly standardly in compare to Illumos.
1. build.rs
This patch enable FreeBSD to be treated as a "standard" implementation
with no linker supported in the usdt build.rs.
2. The values to issue ioctl to dtrace helper are different.
2.1. The cmd value on FreeBSD is derived the C macro
`_IOWR('z', 3, dot_helper_t)`; which is unfolded and the value
is inlined in this patch.
2.2 The struct dof_helper is also different, in FreeBSD there are
two extra fields: `dofhp_pid` and `dofhp_gen`.
3. Linker Issue
Like Illumos, without `FORCE_LOAD` line the linker will omit the symbols
(even with "-C link-dead-code"); therefore the line also needed to activate
on FreeBSD.
There are however still issues in linker that make the FreeBSD support
preliminary, on FreeBSD the crate consumer need to build with
"-C link-dead-code", or otherwise the probes will thrown away by the linker.
Provide a macro version of register_probes!() enables a explicit reference to the set_dtrace_probes section from the crate consumer, and prevent lld from gc the section. This trick along with the unstable "used(linker)" feature enable use on FreeBSD target with nightly toolchain without explicit "link-dead-code" Since the "used_with_arg" is available only on nightly, a patch is applied on usdt-impl/build.rs such that standard backend is enabled only for FreeBSD targets that are either on nightly or have link-dead-code explicitly passed to the linker.
|
Hi @michael-yuji, thanks for sending this patch! It's good to see broader platform support. In general the patch looks fine, however I have a few questions and concerns about the details. I'll comment on specific lines as appropriate, but overall it seems like there are several contortions required to prevent the FreeBSD linker from throwing the probe section into the trash. I think these can be simplified, but I don't have a FreeBSD system to test that on. Hopefully we can iterate together. Last, would you mind including some details on how you tested the code? Output showing some of the example or integration test probes firing would be excellent. Thanks! |
|
@bnaecker I have been testing mostly with a dummy project implemented some probes right now, however, I do just find out some issues regarding my patch, that when probes are defined outside the mod main lives, lld is still able to throw out the probes. Regarding of unit tests there are a few issue I am having, the main one is that unlike on Illumos, |
|
@bnaecker seems like you have the details well in hand, but I wanted to add that it would be good to add CI for FreeBSD if we can either using a built-in action or buildomat... somehow... |
Turns out we do not need all the hacks to get FreeBSD linker working. GNU and llvm both support the "R" flag for sections, which prevent them from gc. This make the ugly register_probes! hack obsolete so it can be removed. No one deserve to have it in their codebase. https://lists.freebsd.org/archives/freebsd-hackers/2022-June/001191.html
|
@ahl @bnaecker Turns out I am a joke and the fix for FreeBSD is quite trivial, seems like recent LLVM and GNU both added support for the "R" section flag, which prevents the section from GC. Therefore the horrible macro hack is not needed at all. Still need Reference: unit test results except |
|
Thanks for the testing notes @michael-yuji, that's very good to see things are mostly passing! And it's also great to hear that we can avoid the macro and public API change. Using the section flag and / or the unstable It's a bit surprising the I'm still surprised that the address cannot be resolved to a symbol, especially in an unoptimized, debug build. Do you actually see the expected function containing the probe in the symbol table of the binary? For reference, here's how I can see that on my machine: That's showing the full output of If the symbol name is in the table, then it's possible that the |
|
I can see the expected function in the symbol table, however I do realized something in comparing to Illumos. Parameter FreeBSD OmniOS: |
usdt-impl/src/no-linker.rs
Outdated
| let cmd: u64 = { | ||
| const DOFHELPER_SIZE: u64 = std::mem::size_of::<dof::dof_bindings::dof_helper>() as u64; | ||
| const IOCPARM_SHIFT: u64 = 13; | ||
| const IOCPARM_MASK: u64 = (1 << IOCPARM_SHIFT) - 1; | ||
| const IOC_INOUT: u64 = 0xc0000000; | ||
| IOC_INOUT | ((DOFHELPER_SIZE & IOCPARM_MASK) << 16) | 0x7a << 8 | 0x3 |
There was a problem hiding this comment.
let's either just have the constant as for illumos or let's change illumos to show its work as you have for freebsd -- i.e. let's not have two ways of doing the same thing
usdt-impl/src/record.rs
Outdated
| Some(CStr::from_ptr(info.dli_sname).to_string_lossy().to_string()), | ||
| Some(CStr::from_ptr(info.dli_fname).to_string_lossy().to_string()), | ||
| ) | ||
| unsafe fn to_str_if_not_null(ptr: *const std::os::raw::c_char) -> Option<String> { |
There was a problem hiding this comment.
deserves a comment; did you encounter this in the wild?
There was a problem hiding this comment.
deserves a comment; did you encounter this in the wild?
Yes, on FreeBSD, dli_sname can be NULL even when dladdr succeed, currently this is always the case when using USDT. dli_fname likely never be NULL and I probably should just remove the check.
@ahl I've not dug too deeply. There's nothing on GH itself, so we'd have to spin up a buildomat runner. I'll try to dig into that this week. |
|
@bnaecker I have found a workaround for the The root cause is that The bug report inspired me to look into how backtrace is implemented on FreeBSD. Turns out, Annoyingly, All tests passed now except for |
FreeBSD dladdr(3M) examine dynamic symbol table only hence never give use the mangled symbol. Instead, we use `backtrace_symbols_fmt` to give us the symbol. Handle backtrace_symbols_fmt and ffi a bit more carefully
|
Thanks for the ping @michael-yuji, and for your patience. I've triggered the CI run to make sure everything works on the main platforms we support (illumos, macOS, and the no-op implementation on Linux). Let's see how that all shakes out and go from there! |
There was a problem hiding this comment.
Hi @michael-yuji! Thanks again for your patience, I'm very sorry this has gotten away from us! I've added a few more comments here, but they're mostly nits. Overall, this looks more or less ready. Once we address these comments here and CI is satisfied, I'd be happy to merge this.
Thanks again!
usdt-impl/src/record.rs
Outdated
| Some(CStr::from_ptr(info.dli_fname).to_string_lossy().to_string()), | ||
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // On FreeBSD, dladdr(3M) only examines the dynamic symbol table. Which is pretty useless as it | ||
| // will always gives null dli_sname. To workaround this issue, we use `backtrace_symbols_fmt` from |
There was a problem hiding this comment.
Nit: typo here "will always gives" -> "always returns a"
usdt-impl/src/record.rs
Outdated
| Some(CStr::from_ptr(info.dli_fname).to_string_lossy().to_string()), | ||
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // On FreeBSD, dladdr(3M) only examines the dynamic symbol table. Which is pretty useless as it | ||
| // will always gives null dli_sname. To workaround this issue, we use `backtrace_symbols_fmt` from | ||
| // libexecinfo, which internally lookup in the executable to determine the symbol of the given |
There was a problem hiding this comment.
Nit: typo here "internally lookup in the" -> "internally looks in the"
usdt-impl/src/record.rs
Outdated
| let lines: Vec<_> = s.lines().collect(); | ||
| (Some(lines[0].to_string()), Some(lines[1].to_string())) |
There was a problem hiding this comment.
Style nit: You don't need to collect this to a Vec. You can keep it as the Lines iterator and call next(). Something like:
let lines = s.lines();
(Some(lines.next().unwrap().to_string()), Some(lines.next().unwrap().to_string()))
usdt-impl/src/record.rs
Outdated
| ) -> *mut *mut libc::c_char; | ||
| } | ||
|
|
||
| let addrs = [addr].as_ptr() as *const *mut c_void; |
There was a problem hiding this comment.
I'm surprised this works. [addr] creates a temporary array, which you then take the address of and store it to addrs. Does that temporary not get dropped right away, invalidating that address? I guess it might be okay, since it's on the stack, but I would feel much more comfortable with a let binding to that array:
let addr_arr = [addr];
let addrs = addr_arr.as_ptr() as *const *mut c_void;
usdt-impl/src/record.rs
Outdated
| #[cfg(target_os = "freebsd")] | ||
| pub(crate) fn addr_to_info(addr: u64) -> (Option<String>, Option<String>) { | ||
| unsafe { | ||
| // The libc crate does not have `backtrace_symbos_fmt` |
There was a problem hiding this comment.
I think this comment isn't needed, it's clear we're specifying the native library to link against.
usdt-impl/src/record.rs
Outdated
| @@ -71,14 +72,54 @@ pub(crate) fn addr_to_info(addr: u64) -> (Option<String>, Option<String>) { | |||
| if libc::dladdr(addr as *const c_void, &mut info as *mut _) == 0 { | |||
| (None, None) | |||
| } else { | |||
| // On some non Illumos platfroms dli_sname can be NULL | |||
There was a problem hiding this comment.
Two nits: "platforms" is misspelled, and there's an extra space following it.
|
The failures in CI are due to the tests around error messages we emit when the macros fail. They've always been a bit annoying to maintain. I believe the issue would be resolved by you rebasing on the latest |
|
@michael-yuji I meant an earlier commit actually, but I think was not right. During development, we built tests that verify the errors we produce when compilation fails, for example creating a probe with an unsupported data type. Those were useful initially, but have become a pain to maintain across many toolchain versions. I'm removing them in #67, as you noted. Once that merges, you should be able to rebase onto that. Thanks! |
|
@michael-yuji Thanks for your patience again. I resolved the CI issues causing the error-message checks to fail. You should now be able to rebase onto |
|
@bnaecker Done, thank you so much! |
|
@michael-yuji Friendly ping on this PR! I'd love to see all your hard work integrated. If you bring this up-to-date with |
|
@bnaecker Thanks for following up, I'm a bit busy this week, but I can probably bring it up to date this weekend. |
|
@bnaecker I have updated the patch and following are the test outputs. 🙌 uname The complete Output |
| @@ -16,6 +16,7 @@ | |||
|
|
|||
| #![cfg_attr(usdt_need_feat_asm, feature(asm))] | |||
| #![cfg_attr(usdt_need_feat_asm_sym, feature(asm_sym))] | |||
| #![cfg_attr(usdt_need_feat_used_with_arg, feature(used_with_arg))] | |||
There was a problem hiding this comment.
Hmm. I don't know if I realized it before, but this feature is not stable. We've recently been able to support a stable compiler on all platforms, and it would be very nice to keep that. I'm not sure that's possible, so FreeBSD might be the hold-out requiring nightly.
Are you certain that the link-dead-code linker-flag is not sufficient by itself to keep the FreeBSD linker from removing the start/stop symbols?
There was a problem hiding this comment.
With stable compiler and link-dead-code, it kinda works empirically under the limited cases I have tried but not that I can comfortably conclude it works (especially when it's failing tests on wrong mangled function name).
Apart from the correctness issue, It also require consumer of the crate to add the linker flag to make it useful.
😞
(edited: attaching the failed test case)
Running unittests src/main.rs (target/debug/deps/does_it_work-d0fe3ba398374246)
running 1 test
test tests::test_does_it_work ... FAILED
failures:
---- tests::test_does_it_work stdout ----
ID PROVIDER MODULE FUNCTION NAME
89108 doesit2045 does_it_work-d0fe3ba398374246 _ZN12does_it_work4main17hfd4a5f4c02c325b6E work
Probe Description Attributes
Identifier Names: Internal
Data Semantics: Internal
Dependency Class: Unknown
Argument Attributes
Identifier Names: Internal
Data Semantics: Internal
Dependency Class: Unknown
Argument Types
args[0]: uint8_t
args[1]: char *
thread 'tests::test_does_it_work' panicked at 'Mangled function name appears incorrect: _ZN12does_it_work4main17hfd4a5f4c02c325b6E', tests/does-it-work/src/main.rs:94:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
tests::test_does_it_work
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.26s
error: test failed, to rerun pass `-p does-it-work --bin does-it-work`
|
@michael-yuji @bnaecker Hi all, it would be cool to merge this PR one way or another, right? Do I understand correctly that the main issue now is that we either merge this but are forced to use nightly or we keep stable but force FreeBSD users to include If that's the case, then the latter option seems cool to me, if it counts for anything. Are there other options, maybe? Is there some alternative path for making it work under stable? |
This PR contains changes to support to FreeBSD.
* Add aregister_probes!()macro, which insert a noop call to a extern function, which argument referencing theset_dtrace_probessection, this prevents the probes from being gc by lldlink-dead-codeis set.nullcheck ondli_fnameanddli_snameas they can be nullon some platforms.
ioctldof_helperioctlvalueCurrently support FreeBSD targets with nightly toolchain, as well as stable toolchain if
link-dead-codeis set inRUSTFLAGSThere is a linker related issue that make the FreeBSD support preliminary
and limited that crate consumers on FreeBSD need to build binary with
-C link-dead-code=yesWithout this workaround , the linker will omit some symbols and complainabout
__start_set_dtrace_probesnot found.A nightly feature,#[used(linker)]is available to fix the issue of linkeromitting
__(start|stop)_set_dtrace_probes; It however does notprevent the linker throwing the probes generated by the provider macros
away. The
-C link-dead-code=yesis therefore currently necessaryregardless of the availability of
used_with_arg