From b950e0b73726579732b707e4afb5429ce9fdeae8 Mon Sep 17 00:00:00 2001 From: Javier Honduvilla Coto Date: Mon, 10 Jun 2024 14:15:38 +0100 Subject: [PATCH] Introduce custom executable id (#46) Before this commit the only unique identifier for an executable file or object file was the build id. As the GNU's build ID might not be present, we fall back to the sha256 hash of the `.text` section. This works fine but takes quite a bit of space as we refer to it in every mapping. By using a 64 bit id derived from the first 8 bytes of the sha256 hash of the code, we can save some memory, make comparisons and hash the IDs faster, and helps simplify the code. For example, there's no need for a separate ID in the native unwind state. There might be a bit more CPU and IO needed to hash the code, but this might be something we can cache in the future. This is also useful for future profile formats that require unique 64 bit IDs for object files. Also fixes other minor things, such as using the lower hexadecimal representation. Test Plan ========= Lots of manual tests, everything seems good. --- src/bpf/profiler.bpf.c | 6 +-- src/bpf/profiler.h | 4 +- src/collector.rs | 8 ++-- src/object.rs | 44 ++++++++++++++------ src/profile.rs | 91 +++++++++++++++++------------------------- src/profiler.rs | 60 ++++++++++++++-------------- src/util.rs | 15 +++++-- 7 files changed, 118 insertions(+), 110 deletions(-) diff --git a/src/bpf/profiler.bpf.c b/src/bpf/profiler.bpf.c index d18046b..785d6f5 100644 --- a/src/bpf/profiler.bpf.c +++ b/src/bpf/profiler.bpf.c @@ -52,7 +52,7 @@ struct { struct { __uint(type, BPF_MAP_TYPE_HASH); __uint(max_entries, 5 * 1000); - __type(key, u32); + __type(key, u64); __type(value, unwind_info_chunks_t); } unwind_info_chunks SEC(".maps"); @@ -116,9 +116,9 @@ static __always_inline u64 find_offset_for_pc(stack_unwind_table_t *table, u64 p // address. static __always_inline chunk_info_t* find_chunk(mapping_t *mapping, u64 object_relative_pc) { - u32 executable_id = mapping->executable_id; + u64 executable_id = mapping->executable_id; - LOG("~about to check chunks, executable_id=%d", executable_id); + LOG("~about to check chunks, executable_id=%lld", executable_id); // Find the chunk where this unwind table lives. // Each chunk maps to exactly one shard. unwind_info_chunks_t *chunks = diff --git a/src/bpf/profiler.h b/src/bpf/profiler.h index 6d78ebf..848dd4f 100644 --- a/src/bpf/profiler.h +++ b/src/bpf/profiler.h @@ -125,11 +125,11 @@ typedef struct { // Represents an executable mapping. typedef struct { - u32 executable_id; - u32 type; + u64 executable_id; u64 load_address; u64 begin; u64 end; + u32 type; } mapping_t; // Key for the longest prefix matching. This is defined diff --git a/src/collector.rs b/src/collector.rs index 7341dd3..c2d5d0d 100644 --- a/src/collector.rs +++ b/src/collector.rs @@ -2,7 +2,7 @@ use std::collections::HashMap; use std::sync::{Arc, Mutex}; use tracing::{debug, span, Level}; -use crate::object::BuildId; +use crate::object::ExecutableId; use crate::profile::symbolize_profile; use crate::profiler::ObjectFileInfo; use crate::profiler::ProcessInfo; @@ -12,7 +12,7 @@ use crate::profiler::SymbolizedAggregatedProfile; pub struct Collector { profiles: Vec, procs: HashMap, - objs: HashMap, + objs: HashMap, } type ThreadSafeCollector = Arc>; @@ -30,7 +30,7 @@ impl Collector { &mut self, profile: RawAggregatedProfile, procs: &HashMap, - objs: &HashMap, + objs: &HashMap, ) { self.profiles.push(profile); @@ -40,7 +40,7 @@ impl Collector { for (k, v) in objs { self.objs.insert( - k.clone(), + *k, ObjectFileInfo { file: std::fs::File::open(v.path.clone()).unwrap(), path: v.path.clone(), diff --git a/src/object.rs b/src/object.rs index 87a1289..01e3059 100644 --- a/src/object.rs +++ b/src/object.rs @@ -3,7 +3,7 @@ use std::io::Read; use std::path::PathBuf; use anyhow::{anyhow, Result}; -use data_encoding::HEXUPPER; +use data_encoding::HEXLOWER; use memmap2; use ring::digest::{Context, Digest, SHA256}; @@ -16,6 +16,8 @@ use object::Object; use object::ObjectKind; use object::ObjectSection; +pub type ExecutableId = u64; + #[derive(Hash, Eq, PartialEq, Clone, Debug)] pub enum BuildId { Gnu(String), @@ -32,6 +34,7 @@ pub struct ElfLoad { pub struct ObjectFile<'a> { leaked_mmap_ptr: *const memmap2::Mmap, object: object::File<'a>, + code_hash: Digest, } impl Drop for ObjectFile<'_> { @@ -49,13 +52,26 @@ impl ObjectFile<'_> { let mmap = Box::new(mmap); let leaked = Box::leak(mmap); let object = object::File::parse(&**leaked)?; + let Some(code_hash) = code_hash(&object) else { + return Err(anyhow!("code hash is None")); + }; Ok(ObjectFile { leaked_mmap_ptr: leaked as *const memmap2::Mmap, object, + code_hash, }) } + /// Returns an identifier for the executable using the first 8 bytes of the Sha256 of the code section. + pub fn id(&self) -> Result { + let mut buffer = [0; 8]; + let _ = self.code_hash.as_ref().read(&mut buffer)?; + Ok(u64::from_ne_bytes(buffer)) + } + + /// Returns the executable build ID if present. If no GNU build ID and no Go build ID + /// are found it returns the hash of the text section. pub fn build_id(&self) -> anyhow::Result { let object = &self.object; let build_id = object.build_id()?; @@ -85,18 +101,8 @@ impl ObjectFile<'_> { } } - // No build id (rust, some other libraries). - for section in object.sections() { - if section.name().unwrap() == ".text" { - if let Ok(section) = section.data() { - return Ok(BuildId::Sha256( - HEXUPPER.encode(sha256_digest(section).as_ref()), - )); - } - } - } - - unreachable!("A build id should always be returned"); + // No build id (Rust, some compilers and Linux distributions). + return Ok(BuildId::Sha256(HEXLOWER.encode(self.code_hash.as_ref()))); } pub fn is_dynamic(&self) -> bool { @@ -160,6 +166,18 @@ impl ObjectFile<'_> { } } +pub fn code_hash(object: &object::File) -> Option { + for section in object.sections() { + if section.name().unwrap() == ".text" { + if let Ok(section) = section.data() { + return Some(sha256_digest(section)); + } + } + } + + None +} + fn sha256_digest(mut reader: R) -> Digest { let mut context = Context::new(&SHA256); let mut buffer = [0; 1024]; diff --git a/src/profile.rs b/src/profile.rs index 6a5b0d6..0400f49 100644 --- a/src/profile.rs +++ b/src/profile.rs @@ -1,10 +1,10 @@ use std::collections::HashMap; use std::path::PathBuf; -use tracing::{debug, span, Level}; +use tracing::{debug, error, span, Level}; use crate::bpf::profiler_bindings::native_stack_t; use crate::ksym::KsymIter; -use crate::object::BuildId; +use crate::object::ExecutableId; use crate::profiler::ExecutableMapping; use crate::profiler::ObjectFileInfo; use crate::profiler::ProcessInfo; @@ -16,7 +16,7 @@ use crate::usym::symbolize_native_stack_blaze; pub fn symbolize_profile( profile: &RawAggregatedProfile, procs: &HashMap, - objs: &HashMap, + objs: &HashMap, ) -> SymbolizedAggregatedProfile { let _span = span!(Level::DEBUG, "symbolize_profile").entered(); let mut r = SymbolizedAggregatedProfile::new(); @@ -61,8 +61,6 @@ pub fn symbolize_profile( symbolized_sample.kstack.push(le_symbol.symbol_name); } }; - // debug!("--- symbolized sample: {}", symbolized_sample); - r.push(symbolized_sample); } @@ -82,7 +80,7 @@ fn find_mapping(mappings: &[ExecutableMapping], addr: u64) -> Option, - objs: &HashMap, + objs: &HashMap, ) -> HashMap>> { let mut addresses_per_sample: HashMap>> = HashMap::new(); @@ -104,28 +102,21 @@ fn fetch_symbols_for_profile( continue; //return Err(anyhow!("could not find mapping")); }; - match &mapping.build_id { - Some(build_id) => { - match objs.get(build_id) { - Some(obj) => { - // We need the normalized address for normal object files - // and might need the absolute addresses for JITs - let normalized_addr = addr - mapping.start_addr + mapping.offset - - obj.load_offset - + obj.load_vaddr; - - let key = obj.path.clone(); - let addrs = addresses_per_sample.entry(key).or_default(); - addrs.insert(normalized_addr, vec!["".to_string()]); - // <- default value is a bit janky - } - None => { - println!("\t\t - [no build id found]"); - } - } + match objs.get(&mapping.executable_id) { + Some(obj) => { + // We need the normalized address for normal object files + // and might need the absolute addresses for JITs + let normalized_addr = addr - mapping.start_addr + mapping.offset + - obj.load_offset + + obj.load_vaddr; + + let key = obj.path.clone(); + let addrs = addresses_per_sample.entry(key).or_default(); + addrs.insert(normalized_addr, vec!["".to_string()]); + // <- default value is a bit janky } None => { - println!("\t\t - mapping is not backed by a file, could be a JIT segment"); + error!("executable with id {} not found", mapping.executable_id); } } } @@ -146,7 +137,7 @@ fn fetch_symbols_for_profile( fn symbolize_native_stack( addresses_per_sample: &HashMap>>, procs: &HashMap, - objs: &HashMap, + objs: &HashMap, pid: i32, native_stack: &native_stack_t, ) -> Vec { @@ -168,39 +159,31 @@ fn symbolize_native_stack( }; // finally - match &mapping.build_id { - Some(build_id) => match objs.get(build_id) { - Some(obj) => { - let failed_to_fetch_symbol = - vec!["".to_string()]; - let failed_to_symbolize = vec!["".to_string()]; - - let normalized_addr = addr - mapping.start_addr + mapping.offset - - obj.load_offset - + obj.load_vaddr; - - let func_names = match addresses_per_sample.get(&obj.path) { - Some(value) => match value.get(&normalized_addr) { - Some(v) => v, - None => &failed_to_fetch_symbol, - }, - None => &failed_to_symbolize, - }; + match objs.get(&mapping.executable_id) { + Some(obj) => { + let failed_to_fetch_symbol = vec!["".to_string()]; + let failed_to_symbolize = vec!["".to_string()]; + + let normalized_addr = + addr - mapping.start_addr + mapping.offset - obj.load_offset + obj.load_vaddr; + + let func_names = match addresses_per_sample.get(&obj.path) { + Some(value) => match value.get(&normalized_addr) { + Some(v) => v, + None => &failed_to_fetch_symbol, + }, + None => &failed_to_symbolize, + }; - for func_name in func_names { - r.push(func_name.clone()); - } + for func_name in func_names { + r.push(func_name.clone()); } - None => { - debug!("\t\t - [no build id found]"); - } - }, + } None => { - debug!("\t\t - mapping is not backed by a file, could be a JIT segment"); + debug!("build id not found"); } } } r - // Ok(()) } diff --git a/src/profiler.rs b/src/profiler.rs index 48572d8..bc5dfd3 100644 --- a/src/profiler.rs +++ b/src/profiler.rs @@ -1,5 +1,5 @@ use std::collections::hash_map::Entry; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::fmt; use std::fs; use std::os::fd::{AsFd, AsRawFd}; @@ -21,7 +21,7 @@ use crate::bpf::profiler_skel::{ProfilerSkel, ProfilerSkelBuilder}; use crate::bpf::tracers_bindings::*; use crate::bpf::tracers_skel::{TracersSkel, TracersSkelBuilder}; use crate::collector::*; -use crate::object::{BuildId, ObjectFile}; +use crate::object::{BuildId, ExecutableId, ObjectFile}; use crate::perf_events::setup_perf_event; use crate::unwind_info::{in_memory_unwind_info, remove_redundant, remove_unnecesary_markers}; use crate::util::{get_online_cpus, summarize_address_range}; @@ -72,6 +72,7 @@ pub enum Unwinder { #[derive(Debug, Clone)] #[allow(dead_code)] pub struct ExecutableMapping { + pub executable_id: ExecutableId, // No build id means either JIT or that we could not fetch it. Change this. pub build_id: Option, pub kind: MappingType, @@ -86,7 +87,7 @@ pub struct ExecutableMapping { } impl ExecutableMapping { - fn mark_as_deleted(&mut self, object_files: &mut HashMap) { + fn mark_as_deleted(&mut self, object_files: &mut HashMap) { // Avoid decrementing the reference count logic more than once if called multiple times. if self.unmapped { return; @@ -94,11 +95,7 @@ impl ExecutableMapping { self.unmapped = true; - let Some(ref build_id) = self.build_id else { - return; - }; - - if let Some(el) = object_files.get_mut(build_id) { + if let Some(el) = object_files.get_mut(&self.executable_id) { el.references -= 1; debug_assert!( el.references >= 0, @@ -112,7 +109,7 @@ pub struct NativeUnwindState { dirty: bool, last_persisted: Instant, live_shard: Vec, - build_id_to_executable_id: HashMap, + known_executables: HashSet, shard_index: u64, } @@ -122,7 +119,7 @@ impl Default for NativeUnwindState { dirty: false, last_persisted: Instant::now() - Duration::from_secs(1_000), // old enough to trigger it the first time live_shard: Vec::with_capacity(SHARD_CAPACITY), - build_id_to_executable_id: HashMap::new(), + known_executables: HashSet::new(), shard_index: 0, } } @@ -135,7 +132,7 @@ pub struct Profiler<'bpf> { tracers: TracersSkel<'bpf>, // Profiler state procs: Arc>>, - object_files: Arc>>, + object_files: Arc>>, // Channel for new process events. chan_send: Arc>>, chan_receive: Arc>>, @@ -779,7 +776,7 @@ impl Profiler<'_> { let object_files = self.object_files.lock().unwrap(); // We might know about a mapping that failed to open for some reason. - let object_file_info = object_files.get(&mapping.build_id.clone().unwrap()); + let object_file_info = object_files.get(&mapping.executable_id); if object_file_info.is_none() { warn!("mapping not found"); continue; @@ -802,20 +799,18 @@ impl Profiler<'_> { // Avoid deadlock std::mem::drop(object_files); - let build_id = mapping.build_id.clone().unwrap(); match self .native_unwind_state - .build_id_to_executable_id - .get(&build_id) + .known_executables + .get(&mapping.executable_id) { - Some(executable_id) => { - // println!("==== in cache"); + Some(_) => { // == Add mapping mappings.push(mapping_t { + executable_id: mapping.executable_id, load_address, begin: mapping.start_addr, end: mapping.end_addr, - executable_id: *executable_id, type_: 0, // normal i think }); debug!("unwind info CACHED for executable {:?}", obj_path); @@ -833,14 +828,13 @@ impl Profiler<'_> { load_address, begin: mapping.start_addr, end: mapping.end_addr, - executable_id: self.native_unwind_state.build_id_to_executable_id.len() as u32, + executable_id: mapping.executable_id, type_: 0, // normal i think }); - let build_id = mapping.build_id.clone().unwrap(); // This is not released (see note "deadlock") let first_mapping_ = self.object_files.lock().unwrap(); - let first_mapping = first_mapping_.get(&build_id).unwrap(); + let first_mapping = first_mapping_.get(&mapping.executable_id).unwrap(); // == Fetch unwind info, so far, this is in mem // todo, pass file handle @@ -883,7 +877,7 @@ impl Profiler<'_> { "======== Unwind rows for executable {}: {} with id {}", obj_path.display(), &found_unwind_info.len(), - self.native_unwind_state.build_id_to_executable_id.len(), + self.native_unwind_state.known_executables.len(), ); let first_pc = found_unwind_info[0].pc; @@ -1015,17 +1009,15 @@ impl Profiler<'_> { .maps() .unwind_info_chunks() .update( - &(self.native_unwind_state.build_id_to_executable_id.len() as u32) - .to_ne_bytes(), + &mapping.executable_id.to_ne_bytes(), unsafe { plain::as_bytes(&all_chunks) }, MapFlags::ANY, ) .unwrap(); - let executable_id = self.native_unwind_state.build_id_to_executable_id.len(); self.native_unwind_state - .build_id_to_executable_id - .insert(build_id, executable_id as u32); + .known_executables + .insert(mapping.executable_id); } // Added all mappings self.native_unwind_state.dirty = true; @@ -1098,8 +1090,8 @@ impl Profiler<'_> { abs_path.push("/root"); abs_path.push(path); - let file = fs::File::open(path)?; - let object_file = ObjectFile::new(path); + let file = fs::File::open(&abs_path)?; + let object_file = ObjectFile::new(&abs_path); if object_file.is_err() { warn!("object_file failed with {:?}", object_file); continue; @@ -1118,6 +1110,11 @@ impl Profiler<'_> { continue; }; + let Ok(executable_id) = object_file.id() else { + debug!("could not get id for object file: {:?}", abs_path); + continue; + }; + let load_address = || { for map2 in maps.iter() { if map2.pathname == map.pathname { @@ -1131,6 +1128,7 @@ impl Profiler<'_> { let main_exec = mappings.is_empty(); mappings.push(ExecutableMapping { + executable_id, build_id: Some(build_id.clone()), kind: MappingType::FileBacked, start_addr: map.address.0, @@ -1142,7 +1140,7 @@ impl Profiler<'_> { unmapped: false, }); - match object_files.entry(build_id) { + match object_files.entry(executable_id) { Entry::Vacant(entry) => match object_file.elf_load() { Ok(elf_load) => { entry.insert(ObjectFileInfo { @@ -1165,6 +1163,7 @@ impl Profiler<'_> { } procfs::process::MMapPath::Anonymous => { mappings.push(ExecutableMapping { + executable_id: 0, // Placeholder for JIT. build_id: None, kind: MappingType::Anonymous, start_addr: map.address.0, @@ -1181,6 +1180,7 @@ impl Profiler<'_> { | procfs::process::MMapPath::Vsys(_) | procfs::process::MMapPath::Vvar => { mappings.push(ExecutableMapping { + executable_id: 0, // Placeholder for vDSO. build_id: None, kind: MappingType::Vdso, start_addr: map.address.0, diff --git a/src/util.rs b/src/util.rs index 5af4fd2..4419404 100644 --- a/src/util.rs +++ b/src/util.rs @@ -121,23 +121,30 @@ mod tests { ..Default::default() }; - let map = - MapHandle::create(MapType::LpmTrie, Some("lpm_test_map"), 16, 32, 1024, &opts).unwrap(); + let map = MapHandle::create( + MapType::LpmTrie, + Some("lpm_test_map"), + std::mem::size_of::() as u32, + std::mem::size_of::() as u32, + 1024, + &opts, + ) + .unwrap(); let mapping1 = mapping_t { executable_id: 1111, - type_: 1, load_address: 1111, begin: 0x7f7428ea8000, end: 0x7f7428f50000, + type_: 1, }; let mapping2 = mapping_t { executable_id: 2222, - type_: 2, load_address: 2222, begin: 0x7f7428f85000, end: 0x7f74290e5000, + type_: 2, }; assert!(mapping1.begin < mapping1.end);