diff --git a/crates/symbolicator-service/src/services/cficaches.rs b/crates/symbolicator-service/src/services/cficaches.rs index 96a1afbfa..5de1a9ed0 100644 --- a/crates/symbolicator-service/src/services/cficaches.rs +++ b/crates/symbolicator-service/src/services/cficaches.rs @@ -4,10 +4,12 @@ use std::sync::Arc; use futures::future::BoxFuture; use minidump_unwind::SymbolFile; +use sentry::types::DebugId; use tempfile::NamedTempFile; use symbolic::cfi::CfiCache; use symbolic::common::ByteView; +use symbolic::debuginfo::breakpad::BreakpadModuleRecord; use symbolicator_sources::{FileType, ObjectId, ObjectType, SourceConfig}; use crate::caching::{ @@ -22,10 +24,15 @@ use crate::utils::sentry::ConfigureScope; use super::caches::versions::CFICACHE_VERSIONS; use super::derived::{derive_from_object_handle, DerivedCache}; -type CfiItem = (u32, Option>); +type CfiItem = Option)>>; + +pub struct CfiModuleInfo { + pub debug_id: DebugId, + pub debug_file: String, +} #[tracing::instrument(skip_all)] -fn parse_cfi_cache(bytes: ByteView<'static>) -> CacheEntry { +fn parse_cfi_cache(bytes: ByteView<'static>) -> CacheEntry<(u32, CfiItem)> { let weight = bytes.len().try_into().unwrap_or(u32::MAX); // NOTE: we estimate the in-memory structures to be ~8x as heavy in memory as on disk let weight = weight.saturating_mul(8); @@ -39,7 +46,23 @@ fn parse_cfi_cache(bytes: ByteView<'static>) -> CacheEntry { let symbol_file = SymbolFile::from_bytes(cfi_cache.as_slice()).map_err(CacheError::from_std_error)?; - Ok((weight, Some(Arc::new(symbol_file)))) + let first_line = cfi_cache + .as_slice() + .split(|b| *b == b'\n') + .next() + .unwrap_or_default(); + let cfi_module = BreakpadModuleRecord::parse(first_line) + .ok() + .and_then(|module| { + let debug_id = module.id.parse().ok()?; + let debug_file = module.name.into(); + Some(CfiModuleInfo { + debug_id, + debug_file, + }) + }); + + Ok((weight, Some(Arc::new((symbol_file, cfi_module))))) } #[derive(Clone, Debug)] @@ -79,7 +102,7 @@ async fn compute_cficache( } impl CacheItemRequest for FetchCfiCacheInternal { - type Item = CfiItem; + type Item = (u32, CfiItem); const VERSIONS: CacheVersions = CFICACHE_VERSIONS; @@ -109,7 +132,7 @@ pub struct FetchCfiCache { pub scope: Scope, } -pub type FetchedCfiCache = DerivedCache>>; +pub type FetchedCfiCache = DerivedCache; impl CfiCacheActor { /// Fetches the CFI cache file for a given code module. diff --git a/crates/symbolicator-service/src/services/symbolication/process_minidump.rs b/crates/symbolicator-service/src/services/symbolication/process_minidump.rs index 9b79dcb78..0b925efd0 100644 --- a/crates/symbolicator-service/src/services/symbolication/process_minidump.rs +++ b/crates/symbolicator-service/src/services/symbolication/process_minidump.rs @@ -20,7 +20,7 @@ use tempfile::TempPath; use symbolic::common::{Arch, ByteView, CodeId, DebugId}; use symbolicator_sources::{ObjectId, ObjectType, SourceConfig}; -use crate::services::cficaches::{CfiCacheActor, FetchCfiCache, FetchedCfiCache}; +use crate::services::cficaches::{CfiCacheActor, CfiModuleInfo, FetchCfiCache, FetchedCfiCache}; use crate::services::minidump::parse_stacktraces_from_minidump; use crate::services::module_lookup::object_file_status_from_cache_entry; use crate::types::{ @@ -212,7 +212,7 @@ impl SymbolicatorSymbolProvider { impl SymbolProvider for SymbolicatorSymbolProvider { async fn fill_symbol( &self, - _module: &(dyn Module + Sync), + module: &(dyn Module + Sync), _frame: &mut (dyn FrameSymbolizer + Send), ) -> Result<(), FillSymbolError> { // Always return an error here to signal that we have no useful symbol information to @@ -221,6 +221,14 @@ impl SymbolProvider for SymbolicatorSymbolProvider { // See https://github.com/rust-minidump/rust-minidump/blob/7eed71e4075e0a81696ccc307d6ac68920de5db5/minidump-processor/src/stackwalker/mod.rs#L295. // // TODO: implement this properly, i.e., use symbolic to actually fill in information. + + // This function is being called for every context frame, + // regardless if any stack walking happens. + // In contrast, `walk_frame` below will be skipped in case the minidump + // does not contain any actionable stack memory that would allow stack walking. + // Loading this module here means we will be able to backfill a possibly missing + // debug_id/file. + self.load_cfi_module(module).await; Err(FillSymbolError {}) } @@ -230,7 +238,7 @@ impl SymbolProvider for SymbolicatorSymbolProvider { walker: &mut (dyn FrameWalker + Send), ) -> Option<()> { let cfi_module = self.load_cfi_module(module).await; - cfi_module.cache.ok()??.walk_frame(module, walker) + cfi_module.cache.ok()??.0.walk_frame(module, walker) } async fn get_file_path( @@ -355,6 +363,15 @@ async fn stackwalk( // NOTE: minidump stackwalking is the first thing that happens to a request, // hence the current candidate list is empty. obj_info.candidates = cfi_module.candidates; + + // if the debug_id/file is empty, it might be possible to + // backfill that using the reference in the executable file. + if let Ok(Some(cfi_item)) = &cfi_module.cache { + if let Some(cfi_module_info) = &cfi_item.1 { + maybe_backfill_debugid(&mut obj_info.raw, cfi_module_info); + } + } + object_file_status_from_cache_entry(&cfi_module.cache) } }); @@ -376,6 +393,15 @@ async fn stackwalk( }) } +fn maybe_backfill_debugid(info: &mut RawObjectInfo, cfi_module: &CfiModuleInfo) { + if info.debug_id.is_none() { + info.debug_id = Some(cfi_module.debug_id.to_string()); + } + if info.debug_file.is_none() { + info.debug_file = Some(cfi_module.debug_file.clone()); + } +} + impl SymbolicationActor { /// Saves the given `minidump_file` in the diagnostics cache if configured to do so. fn maybe_persist_minidump(&self, minidump_file: TempPath) {