From 07a21cff9b7ed638d11491553e8323632c917ead Mon Sep 17 00:00:00 2001 From: Laurence Tratt Date: Mon, 16 Dec 2024 16:44:23 +0000 Subject: [PATCH] Percolate dladdr() errors upwards. The fundamental change here is to turn an `unwrap` in `vaddr_to_off` into an `Err` but this part of hwtracer is somewhat messy, so a seemingly simple change (`unwrap` -> `Err(...)`) requires lots of boilerplate. --- hwtracer/src/lib.rs | 12 +++++++++++- hwtracer/src/perf/collect.rs | 4 ++-- hwtracer/src/pt/ykpt/mod.rs | 14 +++++++++----- tests/src/hwtracer_ykpt.rs | 4 ++-- ykrt/src/trace/hwt/mapper.rs | 21 ++++++++++++++------- ykrt/src/trace/mod.rs | 3 +++ 6 files changed, 41 insertions(+), 17 deletions(-) diff --git a/hwtracer/src/lib.rs b/hwtracer/src/lib.rs index 0537582c8..5aa4c6710 100644 --- a/hwtracer/src/lib.rs +++ b/hwtracer/src/lib.rs @@ -17,6 +17,7 @@ pub use errors::{HWTracerError, TemporaryErrorKind}; #[cfg(test)] use std::time::SystemTime; use std::{fmt::Debug, sync::Arc}; +use thiserror::Error; /// A builder for [Tracer]s. By default, will attempt to use the most appropriate [Tracer] for your /// platform/configuration. This can be overridden with [TracerBuilder::tracer_kind] and @@ -93,7 +94,7 @@ pub trait Trace: Debug + Send { /// Iterate over the blocks of the trace. fn iter_blocks( self: Box, - ) -> Box> + Send>; + ) -> Box> + Send>; #[cfg(test)] fn bytes(&self) -> &[u8]; @@ -107,6 +108,15 @@ pub trait Trace: Debug + Send { fn len(&self) -> usize; } +#[derive(Debug, Error)] +pub enum BlockIteratorError { + #[cfg(ykpt)] + #[error("dladdr() cannot map vaddr")] + NoSuchVAddr, + #[error("HWTracerError: {0}")] + HWTracerError(HWTracerError), +} + /// A loop that does some work that we can use to build a trace. #[cfg(test)] #[inline(never)] diff --git a/hwtracer/src/perf/collect.rs b/hwtracer/src/perf/collect.rs index 6e0e7ebbc..8f7473b44 100644 --- a/hwtracer/src/perf/collect.rs +++ b/hwtracer/src/perf/collect.rs @@ -7,7 +7,7 @@ use crate::pt::c_errors::PerfPTCError; use crate::pt::ykpt::YkPTBlockIterator; use crate::{ errors::{HWTracerError, TemporaryErrorKind}, - Block, ThreadTracer, Trace, Tracer, + Block, BlockIteratorError, ThreadTracer, Trace, Tracer, }; use libc::{c_void, free, geteuid, malloc, size_t}; use std::{fs::read_to_string, sync::Arc}; @@ -182,7 +182,7 @@ impl Trace for PerfTrace { #[cfg(ykpt)] fn iter_blocks( mut self: Box, - ) -> Box> + Send> { + ) -> Box> + Send> { // We hand ownership for self.buf over to `YkPTBlockIterator` so we need to make sure that // we don't try and free it. let buf = std::mem::replace(&mut self.buf, PerfTraceBuf(std::ptr::null_mut())); diff --git a/hwtracer/src/pt/ykpt/mod.rs b/hwtracer/src/pt/ykpt/mod.rs index a733fc0df..e739e5f94 100644 --- a/hwtracer/src/pt/ykpt/mod.rs +++ b/hwtracer/src/pt/ykpt/mod.rs @@ -44,7 +44,7 @@ use crate::{ errors::{HWTracerError, TemporaryErrorKind}, llvm_blockmap::{BlockMapEntry, SuccessorKind, LLVM_BLOCK_MAP}, perf::collect::PerfTraceBuf, - Block, + Block, BlockIteratorError, }; use intervaltree::IntervalTree; use std::{ @@ -233,12 +233,12 @@ impl YkPTBlockIterator<'_> { /// Convert a file offset to a virtual address. fn off_to_vaddr(&self, obj: &Path, off: u64) -> Result { - Ok(ykaddr::addr::off_to_vaddr(obj, off).unwrap()) + ykaddr::addr::off_to_vaddr(obj, off).ok_or(IteratorError::NoSuchVAddr) } /// Convert a virtual address to a file offset. fn vaddr_to_off(&self, vaddr: usize) -> Result<(PathBuf, u64), IteratorError> { - Ok(ykaddr::addr::vaddr_to_obj_and_off(vaddr).unwrap()) + ykaddr::addr::vaddr_to_obj_and_off(vaddr).ok_or(IteratorError::NoSuchVAddr) } /// Looks up the blockmap entry for the given offset in the "main object binary". @@ -818,13 +818,14 @@ impl YkPTBlockIterator<'_> { } impl Iterator for YkPTBlockIterator<'_> { - type Item = Result; + type Item = Result; fn next(&mut self) -> Option { match self.do_next() { Ok(b) => Some(Ok(b)), Err(IteratorError::NoMorePackets) => None, - Err(IteratorError::HWTracerError(e)) => Some(Err(e)), + Err(IteratorError::NoSuchVAddr) => Some(Err(BlockIteratorError::NoSuchVAddr)), + Err(IteratorError::HWTracerError(e)) => Some(Err(BlockIteratorError::HWTracerError(e))), } } } @@ -844,6 +845,9 @@ impl Drop for YkPTBlockIterator<'_> { enum IteratorError { #[error("No more packets")] NoMorePackets, + #[cfg(ykpt)] + #[error("No such vaddr")] + NoSuchVAddr, #[error("HWTracerError: {0}")] HWTracerError(HWTracerError), } diff --git a/tests/src/hwtracer_ykpt.rs b/tests/src/hwtracer_ykpt.rs index e10a7731a..d58ed9e90 100644 --- a/tests/src/hwtracer_ykpt.rs +++ b/tests/src/hwtracer_ykpt.rs @@ -13,7 +13,7 @@ //! To that end, the test files in `tests/hwtracer_ykpt` are compiled into test binaries (as a //! langtester suite) and then they call into this file to have assertions checked in Rust code. -use hwtracer::{ThreadTracer, Trace, TracerBuilder}; +use hwtracer::{BlockIteratorError, ThreadTracer, Trace, TracerBuilder}; use std::ffi::c_void; #[no_mangle] @@ -52,7 +52,7 @@ pub extern "C" fn __hwykpt_decode_trace(trace: *mut Box) -> bool { for b in trace.iter_blocks() { match b { Ok(_) => (), - Err(HWTracerError::Temporary(_)) => return false, + Err(BlockIteratorError::HWTracerError(HWTracerError::Temporary(_))) => return false, Err(_) => panic!(), } } diff --git a/ykrt/src/trace/hwt/mapper.rs b/ykrt/src/trace/hwt/mapper.rs index 32f9adf26..0137f48f1 100644 --- a/ykrt/src/trace/hwt/mapper.rs +++ b/ykrt/src/trace/hwt/mapper.rs @@ -1,7 +1,10 @@ //! The mapper translates a hwtracer trace into an IR trace. use crate::trace::{AOTTraceIterator, AOTTraceIteratorError, TraceAction, TraceRecorderError}; -use hwtracer::{llvm_blockmap::LLVM_BLOCK_MAP, Block, HWTracerError, TemporaryErrorKind, Trace}; +use hwtracer::{ + llvm_blockmap::LLVM_BLOCK_MAP, Block, BlockIteratorError, HWTracerError, TemporaryErrorKind, + Trace, +}; use ykaddr::{ addr::{vaddr_to_obj_and_off, vaddr_to_sym_and_obj}, obj::SELF_BIN_PATH, @@ -13,7 +16,7 @@ use ykaddr::{ /// mapped LLVM IR block, or an unsuccessfully mapped "unmappable block" (an unknown region of /// code spanning at least one machine block). pub(crate) struct HWTTraceIterator { - hwt_iter: Box> + Send>, + hwt_iter: Box> + Send>, /// The next [TraceAction]`s we will produce when `next` is called. We need this intermediary /// to allow us to deduplicate mapped/unmapped basic blocks. This will be empty on the first /// iteration and from then on will always have at least one [TraceAction] in it at all times, @@ -195,10 +198,12 @@ impl Iterator for HWTTraceIterator { _ => panic!(), } } - Some(Err(HWTracerError::Temporary(TemporaryErrorKind::TraceBufferOverflow))) => { + Some(Err(BlockIteratorError::HWTracerError(HWTracerError::Temporary( + TemporaryErrorKind::TraceBufferOverflow, + )))) => { return Some(Err(AOTTraceIteratorError::TraceTooLong)); } - Some(Err(e)) => todo!("{e:?}"), + Some(Err(e)) => return Some(Err(AOTTraceIteratorError::Other(e.to_string()))), None => return Some(Err(AOTTraceIteratorError::PrematureEnd)), } debug_assert!(self.tas_generated > 0); @@ -212,15 +217,17 @@ impl Iterator for HWTTraceIterator { Some(Ok(x)) => { self.map_block(&x); } - Some(Err(HWTracerError::Unrecoverable(x))) + Some(Err(BlockIteratorError::HWTracerError(HWTracerError::Unrecoverable(x)))) if x == "longjmp within traces currently unsupported" => { return Some(Err(AOTTraceIteratorError::LongJmpEncountered)); } - Some(Err(HWTracerError::Temporary(TemporaryErrorKind::TraceBufferOverflow))) => { + Some(Err(BlockIteratorError::HWTracerError(HWTracerError::Temporary( + TemporaryErrorKind::TraceBufferOverflow, + )))) => { return Some(Err(AOTTraceIteratorError::TraceTooLong)); } - Some(Err(e)) => todo!("{e:?}"), + Some(Err(e)) => return Some(Err(AOTTraceIteratorError::Other(e.to_string()))), None => { // The last block should contains pointless unmappable code (the stop tracing call). match self.upcoming.pop() { diff --git a/ykrt/src/trace/mod.rs b/ykrt/src/trace/mod.rs index f3c9943f7..e784eb3a2 100644 --- a/ykrt/src/trace/mod.rs +++ b/ykrt/src/trace/mod.rs @@ -96,6 +96,9 @@ pub(crate) enum AOTTraceIteratorError { #[error("longjmp encountered")] #[allow(dead_code)] LongJmpEncountered, + #[error("{0}")] + #[allow(dead_code)] + Other(String), } /// A processed item from a trace.