Skip to content
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

Add FreeBSD support #63

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions dof/src/dof_bindings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,10 @@ pub struct dof_helper {
pub dofhp_mod: [::std::os::raw::c_char; 64usize],
pub dofhp_addr: u64,
pub dofhp_dof: u64,
#[cfg(target_os = "freebsd")]
pub dofhp_pid: i32,
bnaecker marked this conversation as resolved.
Show resolved Hide resolved
#[cfg(target_os = "freebsd")]
pub dofhp_gen: ::std::os::raw::c_int
}
impl Default for dof_helper {
fn default() -> Self {
Expand Down
41 changes: 41 additions & 0 deletions usdt-impl/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,20 +27,43 @@ enum Backend {
NoOp,
}

fn have_link_dead_code_check() -> bool {
match env::var_os("CARGO_ENCODED_RUSTFLAGS").as_deref() {
Some(rustflags) => {
let mut atoms = rustflags.to_str().unwrap_or("").split(' ');
let mut link_dead_code = false;
// check if the last link-dead-code is n or no
while let Some(atom) = atoms.next() {
if atom.starts_with("-C") && atom.contains("link-dead-code") {
link_dead_code = !atom.contains("link-dead-code=n")
}
}
link_dead_code
}
_ => false
}
}

fn main() {
println!("cargo:rerun-if-changed=build.rs");

// `asm` feature was stabilized in 1.59
let have_stable_asm = version_check::is_min_version("1.59").unwrap_or(false);
// XXX: `asm_sym` feature is not yet stable
let have_stable_asm_sym = false;
let have_stable_used_with_arg = false;

// Are we being built with a compiler which allows feature flags (nightly)
let is_nightly = version_check::is_feature_flaggable().unwrap_or(false);

let feat_asm = env::var_os("CARGO_FEATURE_ASM").is_some();
let feat_strict_asm = env::var_os("CARGO_FEATURE_STRICT_ASM").is_some();

// Check if upstream have enabled link-dead-code, which in those cases we can
// enable standard backend for FreeBSD. We check this by finding the last
// -C link-dead-code* flag, and check if it is a negation of link-dead-code
let have_link_dead_code = have_link_dead_code_check();

let backend = match env::var("CARGO_CFG_TARGET_OS").ok().as_deref() {
Some("macos") if feat_asm => {
if have_stable_asm && have_stable_asm_sym {
Expand All @@ -67,6 +90,24 @@ fn main() {
Backend::NoOp
}
}
Some("freebsd") if feat_asm => {
// FreeBSD require used(linker) to preserve __(start|stop)_set_dtrace_probes
// without explicit "link-dead-code" by consumer
if have_link_dead_code || have_stable_used_with_arg || is_nightly {
if !have_stable_used_with_arg && is_nightly {
println!("cargo:rustc-cfg=usdt_need_used_with_arg");
}
if have_stable_asm {
Backend::Standard
} else if feat_strict_asm || is_nightly {
Backend::Standard
} else {
Backend::NoOp
}
} else {
Backend::NoOp
}
}
_ => Backend::NoOp,
};

Expand Down
1 change: 1 addition & 0 deletions usdt-impl/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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))]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@michael-yuji michael-yuji Mar 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`


use serde::Deserialize;
use std::cell::RefCell;
Expand Down
19 changes: 17 additions & 2 deletions usdt-impl/src/no-linker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,9 +130,10 @@ fn extract_probe_records_from_section() -> Result<Section, crate::Error> {
// writable (to implement one-time registration), so an immutable variable here
bnaecker marked this conversation as resolved.
Show resolved Hide resolved
// leads to _two_ sections, one writable and one read-only. A mutable variable
// here ensures this ends up in a mutable section, the same as the probe records.
#[cfg(target_os = "illumos")]
#[cfg(any(target_os = "illumos", target_os = "freebsd"))]
#[link_section = "set_dtrace_probes"]
bnaecker marked this conversation as resolved.
Show resolved Hide resolved
#[used]
#[cfg_attr(usdt_need_feat_used_with_arg, used(linker))]
#[cfg_attr(not(usdt_need_feat_used_with_arg), used)]
static mut FORCE_LOAD: [u64; 0] = [];

let data = unsafe {
Expand Down Expand Up @@ -170,9 +171,23 @@ fn ioctl_section(buf: &[u8], modname: [std::os::raw::c_char; 64]) -> Result<(),
dofhp_mod: modname,
dofhp_addr: buf.as_ptr() as u64,
dofhp_dof: buf.as_ptr() as u64,
#[cfg(target_os = "freebsd")]
dofhp_pid: std::process::id() as i32,
#[cfg(target_os = "freebsd")]
dofhp_gen: 0
};
let data = &helper as *const _;
#[cfg(target_os = "illumos")]
let cmd: i32 = 0x64746803;
#[cfg(target_os = "freebsd")]
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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};

let file = OpenOptions::new()
.read(true)
.write(true)
Expand Down
16 changes: 12 additions & 4 deletions usdt-impl/src/record.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,15 @@ 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 {
(
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> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deserves a comment; did you encounter this in the wild?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if ptr == null() {
None
} else {
Some(CStr::from_ptr(ptr).to_string_lossy().to_string())
}
}

(to_str_if_not_null(info.dli_sname), to_str_if_not_null(info.dli_fname))
}
}
}
Expand Down Expand Up @@ -213,7 +218,10 @@ impl<'a> ReadCstrExt<'a> for &'a [u8] {
// Construct the ASM record for a probe. If `types` is `None`, then is is an is-enabled probe.
#[allow(dead_code)]
pub(crate) fn emit_probe_record(prov: &str, probe: &str, types: Option<&[DataType]>) -> String {
#[cfg(not(target_os = "freebsd"))]
let section_ident = r#"set_dtrace_probes,"aw","progbits""#;
#[cfg(target_os = "freebsd")]
let section_ident = r#"set_dtrace_probes,"awR","progbits""#;
bnaecker marked this conversation as resolved.
Show resolved Hide resolved
let is_enabled = types.is_none();
let n_args = types.map_or(0, |typ| typ.len());
let arguments = types.map_or_else(String::new, |types| {
Expand Down