feat(difs): unwind info in dif candidates (#324)

This adds the unwind info to dif candidates. For this the cficache actor also exposes DIF info and the symbolication actor now correctly merges the various DIF candidates info together.
getsentry · Jan 5, 2021 · 0ea1924 · 0ea1924
1 parent d59691d
commit 0ea1924
Show file tree

Hide file tree

Showing 9 changed files with 357 additions and 97 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,7 +4,7 @@
 
 ### Features
 
-- Symbolication responses include download information about all DIF object files which were looked up on the available object sources. ([#309](https://github.com/getsentry/symbolicator/pull/309) [#316](https://github.com/getsentry/symbolicator/pull/316))
+- Symbolication responses include download, debug and unwind information about all DIF object files which were looked up on the available object sources. ([#309](https://github.com/getsentry/symbolicator/pull/309) [#316](https://github.com/getsentry/symbolicator/pull/316) [#324](https://github.com/getsentry/symbolicator/pull/324))
 
 ### Bug Fixes
 

diff --git a/src/actors/cficaches.rs b/src/actors/cficaches.rs
@@ -5,8 +5,7 @@ use std::sync::Arc;
 use std::time::Duration;
 
 use futures::compat::Future01CompatExt;
-use futures::future::{self, Either};
-use futures::{FutureExt, TryFutureExt};
+use futures::prelude::*;
 use sentry::{configure_scope, Hub, SentryFutureExt};
 use symbolic::{
     common::ByteView,
@@ -20,7 +19,9 @@ use crate::actors::objects::{
 };
 use crate::cache::{Cache, CacheKey, CacheStatus};
 use crate::sources::{FileType, SourceConfig};
-use crate::types::{ObjectFeatures, ObjectId, ObjectType, Scope};
+use crate::types::{
+    AllObjectCandidates, ObjectFeatures, ObjectId, ObjectType, ObjectUseInfo, Scope,
+};
 use crate::utils::futures::{BoxedFuture, ThreadPool};
 use crate::utils::sentry::WriteSentryScope;
 
@@ -72,6 +73,7 @@ pub struct CfiCacheFile {
     features: ObjectFeatures,
     status: CacheStatus,
     path: CachePath,
+    candidates: AllObjectCandidates,
 }
 
 impl CfiCacheFile {
@@ -89,13 +91,19 @@ impl CfiCacheFile {
     pub fn path(&self) -> &Path {
         self.path.as_ref()
     }
+
+    /// Returns all the DIF object candidates.
+    pub fn candidates(&self) -> &AllObjectCandidates {
+        &self.candidates
+    }
 }
 
 #[derive(Clone, Debug)]
 struct FetchCfiCacheInternal {
     request: FetchCfiCache,
     objects_actor: ObjectsActor,
-    object_meta: Arc<ObjectMetaHandle>,
+    meta_handle: Arc<ObjectMetaHandle>,
+    candidates: AllObjectCandidates,
     threadpool: ThreadPool,
 }
 
@@ -104,7 +112,7 @@ impl CacheItemRequest for FetchCfiCacheInternal {
     type Error = CfiCacheError;
 
     fn get_cache_key(&self) -> CacheKey {
-        self.object_meta.cache_key()
+        self.meta_handle.cache_key()
     }
 
     /// Extracts the Call Frame Information (CFI) from an object file.
@@ -115,12 +123,12 @@ impl CacheItemRequest for FetchCfiCacheInternal {
         let path = path.to_owned();
         let object = self
             .objects_actor
-            .fetch(self.object_meta.clone())
+            .fetch(self.meta_handle.clone())
             .map_err(CfiCacheError::Fetching);
 
         let threadpool = self.threadpool.clone();
         let result = object.and_then(move |object| {
-            let future = future::lazy(move |_| {
+            let future = async move {
                 if object.status() != CacheStatus::Positive {
                     return Ok(object.status());
                 }
@@ -135,7 +143,7 @@ impl CacheItemRequest for FetchCfiCacheInternal {
                 };
 
                 Ok(status)
-            });
+            };
 
             threadpool
                 .spawn_handle(future.bind_hub(Hub::current()))
@@ -168,14 +176,22 @@ impl CacheItemRequest for FetchCfiCacheInternal {
         data: ByteView<'static>,
         path: CachePath,
     ) -> Self::Item {
+        let mut candidates = self.candidates.clone();
+        candidates.set_unwind(
+            self.meta_handle.source().clone(),
+            self.meta_handle.location(),
+            ObjectUseInfo::from_derived_status(status, self.meta_handle.status()),
+        );
+
         CfiCacheFile {
             object_type: self.request.object_type,
             identifier: self.request.identifier.clone(),
             scope,
             data,
-            features: self.object_meta.features(),
+            features: self.meta_handle.features(),
             status,
             path,
+            candidates,
         }
     }
 }
@@ -196,11 +212,11 @@ impl CfiCacheActor {
     /// debug filename (the basename).  To do this it looks in the existing cache with the
     /// given scope and if it does not yet exist in cached form will fetch the required DIFs
     /// and compute the required CFI cache file.
-    pub fn fetch(
+    pub async fn fetch(
         &self,
         request: FetchCfiCache,
-    ) -> BoxedFuture<Result<Arc<CfiCacheFile>, Arc<CfiCacheError>>> {
-        let object = self
+    ) -> Result<Arc<CfiCacheFile>, Arc<CfiCacheError>> {
+        let found_result = self
             .objects
             .clone()
             .find(FindObject {
@@ -210,41 +226,32 @@ impl CfiCacheActor {
                 scope: request.scope.clone(),
                 purpose: ObjectPurpose::Unwind,
             })
-            .map_err(|e| Arc::new(CfiCacheError::Fetching(e)));
-
-        let cficaches = self.cficaches.clone();
-        let threadpool = self.threadpool.clone();
-        let objects = self.objects.clone();
-
-        let object_type = request.object_type;
-        let identifier = request.identifier.clone();
-        let scope = request.scope.clone();
-
-        object
-            .and_then(move |object| {
-                object
-                    .meta
-                    .map(move |object_meta| {
-                        Either::Left(cficaches.compute_memoized(FetchCfiCacheInternal {
-                            request,
-                            objects_actor: objects,
-                            object_meta,
-                            threadpool,
-                        }))
-                    })
-                    .unwrap_or_else(move || {
-                        Either::Right(future::ok(Arc::new(CfiCacheFile {
-                            object_type,
-                            identifier,
-                            scope,
-                            data: ByteView::from_slice(b""),
-                            features: ObjectFeatures::default(),
-                            status: CacheStatus::Negative,
-                            path: CachePath::new(),
-                        })))
+            .map_err(|e| Arc::new(CfiCacheError::Fetching(e)))
+            .await?;
+
+        match found_result.meta {
+            Some(meta_handle) => {
+                self.cficaches
+                    .compute_memoized(FetchCfiCacheInternal {
+                        request,
+                        objects_actor: self.objects.clone(),
+                        meta_handle,
+                        threadpool: self.threadpool.clone(),
+                        candidates: found_result.candidates,
                     })
-            })
-            .boxed_local()
+                    .await
+            }
+            None => Ok(Arc::new(CfiCacheFile {
+                object_type: request.object_type,
+                identifier: request.identifier,
+                scope: request.scope,
+                data: ByteView::from_slice(b""),
+                features: ObjectFeatures::default(),
+                status: CacheStatus::Negative,
+                path: CachePath::new(),
+                candidates: found_result.candidates,
+            })),
+        }
     }
 }
 

diff --git a/src/actors/common/cache.rs b/src/actors/common/cache.rs
@@ -16,7 +16,7 @@ use crate::types::Scope;
 use crate::utils::futures::{BoxedFuture, CallOnDrop};
 
 /// Result from [`Cacher::compute_memoized`].
-type CacheResult<T, E> = BoxedFuture<Result<Arc<T>, Arc<E>>>;
+type CacheResultFuture<T, E> = BoxedFuture<Result<Arc<T>, Arc<E>>>;
 
 // Inner result necessary because `futures::Shared` won't give us `Arc`s but its own custom
 // newtype around it.
@@ -311,7 +311,7 @@ impl<T: CacheItemRequest> Cacher<T> {
     /// occurs the error result is returned, **however** in this case nothing is written
     /// into the cache and the next call to the same cache item will attempt to re-compute
     /// the cache.
-    pub fn compute_memoized(&self, request: T) -> CacheResult<T::Item, T::Error> {
+    pub fn compute_memoized(&self, request: T) -> CacheResultFuture<T::Item, T::Error> {
         let key = request.get_cache_key();
         let name = self.config.name();
 

diff --git a/src/actors/snapshots/symbolicator__actors__symbolication__tests__minidump_windows.snap b/src/actors/snapshots/symbolicator__actors__symbolication__tests__minidump_windows.snap
@@ -190,6 +190,8 @@ modules:
             has_unwind_info: true
             has_symbols: true
             has_sources: false
+        unwind:
+          status: ok
         debug:
           status: ok
       - source: local
@@ -256,6 +258,27 @@ modules:
     debug_file: dbgcore.pdb
     image_addr: 0x70b70000
     image_size: 151552
+    candidates:
+      - source: local
+        location: dbgcore.dll/57898DAB25000/dbgcore.dl_
+        download:
+          status: notfound
+      - source: local
+        location: dbgcore.dll/57898DAB25000/dbgcore.dll
+        download:
+          status: notfound
+      - source: local
+        location: dbgcore.pdb/AEC7EF2FDF4B4642A4714C3E5FE8760A1/dbgcore.pd_
+        download:
+          status: notfound
+      - source: local
+        location: dbgcore.pdb/AEC7EF2FDF4B4642A4714C3E5FE8760A1/dbgcore.pdb
+        download:
+          status: notfound
+      - source: local
+        location: dbgcore.pdb/AEC7EF2FDF4B4642A4714C3E5FE8760A1/dbgcore.sym
+        download:
+          status: notfound
   - debug_status: unused
     unwind_status: unused
     features:
@@ -412,6 +435,27 @@ modules:
     debug_file: wrpcrt4.pdb
     image_addr: 0x75810000
     image_size: 790528
+    candidates:
+      - source: local
+        location: rpcrt4.dll/5A49BB75c1000/rpcrt4.dl_
+        download:
+          status: notfound
+      - source: local
+        location: rpcrt4.dll/5A49BB75c1000/rpcrt4.dll
+        download:
+          status: notfound
+      - source: local
+        location: wrpcrt4.pdb/AE131C6727A74FA19916B5A4AEF411901/wrpcrt4.pd_
+        download:
+          status: notfound
+      - source: local
+        location: wrpcrt4.pdb/AE131C6727A74FA19916B5A4AEF411901/wrpcrt4.pdb
+        download:
+          status: notfound
+      - source: local
+        location: wrpcrt4.pdb/AE131C6727A74FA19916B5A4AEF411901/wrpcrt4.sym
+        download:
+          status: notfound
   - debug_status: unused
     unwind_status: unused
     features:

diff --git a/src/actors/symbolication.rs b/src/actors/symbolication.rs
@@ -686,8 +686,7 @@ impl SymCacheLookup {
                 if let Some(ref symcache) = symcache {
                     entry.object_info.arch = symcache.arch();
                     entry.object_info.features.merge(symcache.features());
-
-                    entry.object_info.candidates = symcache.candidates(); // TODO(flub): merge!
+                    entry.object_info.candidates.merge(symcache.candidates());
                 }
 
                 entry.symcache = symcache;
@@ -1414,22 +1413,28 @@ impl SymbolicationActor {
         requests: Vec<(CodeModuleId, RawObjectInfo)>,
         sources: Arc<[SourceConfig]>,
     ) -> Vec<CfiCacheResult> {
-        let cficaches = self.cficaches.clone();
+        let mut futures = Vec::with_capacity(requests.len());
 
-        let futures = requests
-            .into_iter()
-            .map(move |(code_module_id, object_info)| {
-                cficaches
+        for (code_id, object_info) in requests {
+            let sources = sources.clone();
+            let scope = scope.clone();
+
+            let fut = async move {
+                let result = self
+                    .cficaches
                     .fetch(FetchCfiCache {
                         object_type: object_info.ty,
                         identifier: object_id_from_object_info(&object_info),
-                        sources: sources.clone(),
-                        scope: scope.clone(),
+                        sources,
+                        scope,
                     })
-                    .then(move |result| future::ready((code_module_id, result)))
-                    // Clone hub because of join_all
-                    .bind_hub(Hub::new_from_top(Hub::current()))
-            });
+                    .await;
+                (code_id, result)
+            };
+
+            // Clone hub because of join_all concurrency.
+            futures.push(fut.bind_hub(Hub::new_from_top(Hub::current())));
+        }
 
         future::join_all(futures).await
     }
@@ -1463,14 +1468,18 @@ impl SymbolicationActor {
         let mut unwind_statuses = BTreeMap::new();
         let mut object_features = BTreeMap::new();
         let mut frame_info_map = BTreeMap::new();
+        let mut dif_candidates = BTreeMap::new();
 
         // Go through all the modules in the minidump and build a map of the modules with
         // missing or malformed CFI.  ObjectFileStatus::Found is only added when the file is
         // loaded as it could still be corrupted when reading from the cache.  Usable CFI
         // cache files are added to the frame_info_map.
         for (code_module_id, result) in &cfi_results {
             let cache_file = match result {
-                Ok(x) => x,
+                Ok(cfi_cache_file) => {
+                    dif_candidates.insert(*code_module_id, cfi_cache_file.candidates().clone());
+                    cfi_cache_file
+                }
                 Err(e) => {
                     log::debug!("Error while fetching cficache: {}", LogError(e.as_ref()));
                     unwind_statuses.insert(*code_module_id, (&**e).into());
@@ -1510,15 +1519,19 @@ impl SymbolicationActor {
                         unwind_statuses,
                         minidump.clone(),
                         spawn_time,
+                        procspawn::serde::Json(dif_candidates),
                     ),
                     |(
                         frame_info_map,
                         object_features,
                         mut unwind_statuses,
                         minidump,
                         spawn_time,
+                        dif_candidates,
                     )|
-                     -> Result<_, ProcessMinidumpError> {
+                        -> Result<_, ProcessMinidumpError> {
+                        let procspawn::serde::Json(mut dif_candidates) = dif_candidates;
+
                         if let Ok(duration) = spawn_time.elapsed() {
                             metric!(timer("minidump.stackwalk.spawn.duration") = duration);
                         }
@@ -1622,6 +1635,15 @@ impl SymbolicationActor {
 
                                 info.features.merge(features);
 
+                                if let Some(code_id) = module_id {
+                                    // If we have the same code module mapped into the
+                                    // memory in multiple regions we would only have the
+                                    // candidates filled in for the first one.
+                                    if let Some(candidates) = dif_candidates.remove(&code_id) {
+                                        info.candidates = candidates;
+                                    }
+                                }
+
                                 info
                             })
                             .collect::<CodeModulesBuilder>();