From 5bc2998e5317bc5f95f297290d427fa99603a171 Mon Sep 17 00:00:00 2001 From: Laurence Tratt Date: Mon, 27 Mar 2023 18:01:40 +0100 Subject: [PATCH 1/2] Move from `Result` to `Option`. --- yktrace/src/hwt/mapper.rs | 1 + ykutil/src/addr.rs | 14 ++++++-------- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/yktrace/src/hwt/mapper.rs b/yktrace/src/hwt/mapper.rs index 718b508a9..186b87a5f 100644 --- a/yktrace/src/hwt/mapper.rs +++ b/yktrace/src/hwt/mapper.rs @@ -104,6 +104,7 @@ impl<'a> HWTMapper { // function's symbol. If the cache knows that block A and B are from the same // function, and a block X has a start address between blocks A and B, then X must // also belong to the same function and there's no need to query the linker. + // FIXME: Is this `unwrap` safe? let sio = vaddr_to_sym_and_obj(usize::try_from(block_vaddr).unwrap()).unwrap(); debug_assert_eq!( obj_name.to_str().unwrap(), diff --git a/ykutil/src/addr.rs b/ykutil/src/addr.rs index e2d734a06..250c34eee 100644 --- a/ykutil/src/addr.rs +++ b/ykutil/src/addr.rs @@ -130,14 +130,12 @@ pub fn off_to_vaddr(containing_obj: &Path, off: u64) -> Option { /// /// This function uses `dladdr()` internally, and thus inherits the same symbol visibility rules /// used there. For example, this function will not find unexported symbols. -pub fn vaddr_to_sym_and_obj(vaddr: usize) -> Result { - let info = dladdr(vaddr)?; +pub fn vaddr_to_sym_and_obj(vaddr: usize) -> Option { // `dladdr()` returns success if at least the virtual address could be mapped to an object // file, but here it is crucial that we can also find the symbol that the address belongs to. - if info.dli_sname().is_some() { - Ok(info) - } else { - Err(()) + match dladdr(vaddr) { + Ok(x) if x.dli_sname().is_some() => Some(x), + Ok(_) | Err(()) => None, } } @@ -211,13 +209,13 @@ mod tests { #[test] fn vaddr_to_sym_and_obj_cant_find_obj() { let func_vaddr = 1; // Obscure address unlikely to be in any loaded object. - assert!(vaddr_to_sym_and_obj(func_vaddr as usize).is_err()); + assert!(vaddr_to_sym_and_obj(func_vaddr as usize).is_none()); } #[test] fn vaddr_to_sym_and_obj_cant_find_sym() { // Address valid, but symbol not exported (test bin not built with `-Wl,--export-dynamic`). let func_vaddr = vaddr_to_sym_and_obj_cant_find_sym as *const fn(); - assert!(vaddr_to_sym_and_obj(func_vaddr as usize).is_err()); + assert!(vaddr_to_sym_and_obj(func_vaddr as usize).is_none()); } } From b7575b5bb8e3afcc3ac9a2373ae3821b3573e6bc Mon Sep 17 00:00:00 2001 From: Laurence Tratt Date: Mon, 27 Mar 2023 18:14:22 +0100 Subject: [PATCH 2/2] Various Clippy fixes. These are the last remaining Clippy fixes (well, at least until we introduce some more or Clippy becomes more stringent!). --- hwtracer/src/decode/ykpt/mod.rs | 6 ++---- hwtracer/src/lib.rs | 3 ++- ykcapi/src/lib.rs | 4 +++- ykfr/src/lib.rs | 2 ++ ykrt/src/location.rs | 1 + ykrt/src/mt.rs | 2 +- yksmp/src/lib.rs | 1 + yktrace/src/lib.rs | 4 +++- 8 files changed, 15 insertions(+), 8 deletions(-) diff --git a/hwtracer/src/decode/ykpt/mod.rs b/hwtracer/src/decode/ykpt/mod.rs index 1bb449564..73da5003b 100644 --- a/hwtracer/src/decode/ykpt/mod.rs +++ b/hwtracer/src/decode/ykpt/mod.rs @@ -703,7 +703,7 @@ impl<'t> YkPTBlockIterator<'t> { /// Fetch the next packet and update iterator state. fn packet(&mut self) -> Result { - let ret = if let Some(pkt_or_err) = self.parser.next() { + if let Some(pkt_or_err) = self.parser.next() { let mut pkt = pkt_or_err?; if pkt.kind() == PacketKind::OVF { @@ -778,9 +778,7 @@ impl<'t> YkPTBlockIterator<'t> { Ok(pkt) } else { Err(HWTracerError::NoMorePackets) - }; - - ret + } } } diff --git a/hwtracer/src/lib.rs b/hwtracer/src/lib.rs index b8874c327..0e0d31a79 100644 --- a/hwtracer/src/lib.rs +++ b/hwtracer/src/lib.rs @@ -1,5 +1,6 @@ -#![allow(clippy::upper_case_acronyms)] +#![allow(clippy::len_without_is_empty)] #![allow(clippy::new_without_default)] +#![allow(clippy::upper_case_acronyms)] #![feature(once_cell)] #![feature(ptr_sub_ptr)] diff --git a/ykcapi/src/lib.rs b/ykcapi/src/lib.rs index 625228e4a..af3515e8c 100644 --- a/ykcapi/src/lib.rs +++ b/ykcapi/src/lib.rs @@ -13,7 +13,6 @@ #[cfg(feature = "yk_testing")] mod testing; -use libc; use std::arch::asm; use std::convert::{TryFrom, TryInto}; use std::ffi::{c_char, c_void, CString}; @@ -25,6 +24,7 @@ use ykrt::{HotThreshold, Location, MT}; use yksmp::{Location as SMLocation, StackMapParser}; #[no_mangle] +#[allow(clippy::not_unsafe_ptr_arg_deref)] pub extern "C" fn yk_mt_new(err_msg: *mut *const c_char) -> *mut MT { match MT::new() { Ok(mt) => Box::into_raw(Box::new(mt)), @@ -45,6 +45,7 @@ pub extern "C" fn yk_mt_new(err_msg: *mut *const c_char) -> *mut MT { } #[no_mangle] +#[allow(clippy::not_unsafe_ptr_arg_deref)] pub extern "C" fn yk_mt_drop(mt: *mut MT) { unsafe { Box::from_raw(mt) }; } @@ -58,6 +59,7 @@ pub extern "C" fn yk_mt_control_point(_mt: *mut MT, _loc: *mut Location) { // The "real" control point, that is called once the interpreter has been patched by ykllvm. // Returns the address of a reconstructed stack or null if there wasn#t a guard failure. #[no_mangle] +#[allow(clippy::not_unsafe_ptr_arg_deref)] pub extern "C" fn __ykrt_control_point( mt: *mut MT, loc: *mut Location, diff --git a/ykfr/src/lib.rs b/ykfr/src/lib.rs index 5662e6a8f..1b8460142 100644 --- a/ykfr/src/lib.rs +++ b/ykfr/src/lib.rs @@ -1,4 +1,6 @@ #![feature(once_cell)] +#![allow(clippy::comparison_chain)] +#![allow(clippy::missing_safety_doc)] use llvm_sys::{core::*, target::LLVMABISizeOfType}; use object::{Object, ObjectSection}; diff --git a/ykrt/src/location.rs b/ykrt/src/location.rs index dfbb2229e..4f86544e5 100644 --- a/ykrt/src/location.rs +++ b/ykrt/src/location.rs @@ -373,6 +373,7 @@ impl LocationInner { /// If this `State` is not counting, return its `HotLocation`. It is undefined behaviour to /// call this function if this `State` is in the counting phase and/or if this `State` is not /// locked. + #[allow(clippy::mut_from_ref)] pub(super) unsafe fn hot_location(&self) -> &mut HotLocation { debug_assert!(!self.is_counting()); debug_assert!(self.is_locked()); diff --git a/ykrt/src/mt.rs b/ykrt/src/mt.rs index 0ddbc5b87..d709ec748 100644 --- a/ykrt/src/mt.rs +++ b/ykrt/src/mt.rs @@ -78,7 +78,7 @@ impl MT { max_worker_threads: AtomicUsize::new(cmp::max(1, num_cpus::get() - 1)), active_worker_threads: AtomicUsize::new(0), trace_decoder_kind: TraceDecoderKind::default_for_platform() - .ok_or_else(|| "Tracing is not supported on this platform")?, + .ok_or("Tracing is not supported on this platform")?, tracing_kind: TracingKind::default(), }) } diff --git a/yksmp/src/lib.rs b/yksmp/src/lib.rs index 1c7153d46..87faf13e3 100644 --- a/yksmp/src/lib.rs +++ b/yksmp/src/lib.rs @@ -1,3 +1,4 @@ +#![allow(clippy::len_without_is_empty)] #![allow(clippy::new_without_default)] //! Note that LLVM currently only supports stackmaps for 64 bit architectures. Once they support diff --git a/yktrace/src/lib.rs b/yktrace/src/lib.rs index d51f11ffc..319e545bf 100644 --- a/yktrace/src/lib.rs +++ b/yktrace/src/lib.rs @@ -2,7 +2,9 @@ #![feature(once_cell)] #![feature(naked_functions)] +#![allow(clippy::len_without_is_empty)] #![allow(clippy::new_without_default)] +#![allow(clippy::missing_safety_doc)] mod errors; use hwtracer::decode::TraceDecoderKind; @@ -229,7 +231,7 @@ impl IRTrace { di_tmpname_c, ) }; - if ret == ptr::null() { + if ret.is_null() { Err("Could not compile trace.".into()) } else { Ok((ret, di_tmp))