Merge SymResolver::find_code_info() into SymResolver::find_sym()

The SymResolver trait contains two methods used to fulfill symbolization
requests: find_sym() unearths basic information about the symbol such as
its name, address, and size, and find_code_info() reports additional
source code level information, if present.
This split is rather arbitrary and it can be the cause of
inefficiencies. Specifically, source code information may be co-located
with symbol information and by having this two-way split we may need to
perform the same lookup twice. This is obviously the case already in the
Breakpad resolver, but it is actually also happening in a more
convoluted way in the DWARF logic.
The only conceivable reason why such a split may have been desired in
the past in the first place, is to "refine" symbol information from one
source with that of another. For example, for kernel symbols one may use
kallsyms to report the symbol name, but then look at a potential DWARF
file for reporting source code information. While a valid use case, it
is niche and generally not all that necessary: if one were to fall back
to using *only* DWARF, there should be no issue in practice. This
arbitrary split also means that we need to stitch together the final Sym
object based on some IntSym and an optional AddrCodeInfo, which requires
the existence of those internal types in the first place.

This change merges these two methods into one. It does so in the dumbest
way possible: just adjusting the signature of find_sym() and then
reusing find_code_info() internally. This is making for some complex
logic, but this will get simplified down the line.

Note that, while conceptually a refactoring, this change introduces a
slight semantic change to the kernel resolver: where previously it may
consult two "resolvers" and stitch the result together, it now only ever
consults a single resolver.

Signed-off-by: Daniel Müller <[email protected]>

src/breakpad/resolver.rs

@@ -13,6 +13,7 @@ use crate::inspect::SymInfo;
 use crate::mmap::Mmap;
 use crate::symbolize::AddrCodeInfo;
 use crate::symbolize::CodeInfo;
+use crate::symbolize::FindSymOpts;
 use crate::symbolize::IntSym;
 use crate::symbolize::Reason;
 use crate::symbolize::SrcLang;
@@ -118,15 +119,25 @@ impl BreakpadResolver {
 
 impl SymResolver for BreakpadResolver {
     #[cfg_attr(feature = "tracing", crate::log::instrument(fields(addr = format_args!("{addr:#x}"))))]
-    fn find_sym(&self, addr: Addr) -> Result<Result<IntSym<'_>, Reason>> {
+    fn find_sym(
+        &self,
+        addr: Addr,
+        opts: &FindSymOpts,
+    ) -> Result<Result<(IntSym<'_>, Option<AddrCodeInfo<'_>>), Reason>> {
         if let Some(func) = self.symbol_file.find_function(addr) {
             let sym = IntSym {
                 name: &func.name,
                 addr: func.addr,
                 size: Some(func.size.try_into().unwrap_or(usize::MAX)),
                 lang: SrcLang::Unknown,
             };
-            Ok(Ok(sym))
+
+            let code_info = if opts.code_info() {
+                self.find_code_info(addr, opts.inlined_fns())?
+            } else {
+                None
+            };
+            Ok(Ok((sym, code_info)))
         } else {
             let reason = if self.symbol_file.functions.is_empty() {
                 Reason::MissingSyms
@@ -152,12 +163,11 @@ impl SymResolver for BreakpadResolver {
 
         Ok(syms)
     }
+}
 
+impl BreakpadResolver {
+    // TODO: Fold into `find_sym`.
     fn find_code_info(&self, addr: Addr, inlined_fns: bool) -> Result<Option<AddrCodeInfo<'_>>> {
-        // TODO: We really shouldn't be doing another `find_function`
-        //       binary search here, conceptually. Consider merging
-        //       `find_code_info` into `find_sym` at the `SymResolver`
-        //       level.
         if let Some(func) = self.symbol_file.find_function(addr) {
             if let Some(source_line) = func.find_line(addr) {
                 let (dir, file) = self.find_source_location(source_line.file)?;

src/dwarf/resolver.rs

@@ -19,6 +19,7 @@ use crate::inspect::FindAddrOpts;
 use crate::inspect::SymInfo;
 use crate::symbolize::AddrCodeInfo;
 use crate::symbolize::CodeInfo;
+use crate::symbolize::FindSymOpts;
 use crate::symbolize::IntSym;
 use crate::symbolize::SrcLang;
 use crate::Addr;
@@ -116,11 +117,7 @@ impl DwarfResolver {
     /// Find source code information of an address.
     ///
     /// `addr` is a normalized address.
-    pub fn find_code_info(
-        &self,
-        addr: Addr,
-        inlined_fns: bool,
-    ) -> Result<Option<AddrCodeInfo<'_>>> {
+    fn find_code_info(&self, addr: Addr, inlined_fns: bool) -> Result<Option<AddrCodeInfo<'_>>> {
         // TODO: This conditional logic is weird and potentially
         //       unnecessary. Consider removing it or moving it higher
         //       in the call chain.
@@ -142,6 +139,7 @@ impl DwarfResolver {
                 };
 
                 let inlined = if inlined_fns {
+                    // TODO: Should reuse `function` from caller here instead.
                     if let Some(inline_stack) = self.units.find_inlined_functions(addr)? {
                         let mut inlined = Vec::with_capacity(inline_stack.len());
                         for result in inline_stack {
@@ -198,24 +196,34 @@ impl DwarfResolver {
     }
 
     /// Lookup the symbol at an address.
-    pub(crate) fn find_sym(&self, addr: Addr) -> Result<Option<IntSym<'_>>, Error> {
+    pub(crate) fn find_sym(
+        &self,
+        addr: Addr,
+        opts: &FindSymOpts,
+    ) -> Result<Option<(IntSym<'_>, Option<AddrCodeInfo<'_>>)>> {
         if let Some((function, unit)) = self.units.find_function(addr)? {
             let name = function
                 .name
                 .map(|name| name.to_string())
                 .transpose()?
                 .unwrap_or("");
-            let addr = function.range.map(|range| range.begin).unwrap_or(0);
+            let fn_addr = function.range.map(|range| range.begin).unwrap_or(0);
             let size = function
                 .range
                 .map(|range| usize::try_from(range.end - range.begin).unwrap_or(usize::MAX));
             let sym = IntSym {
                 name,
-                addr,
+                addr: fn_addr,
                 size,
                 lang: unit.language().into(),
             };
-            Ok(Some(sym))
+
+            let code_info = if opts.code_info() {
+                self.find_code_info(addr, opts.inlined_fns())?
+            } else {
+                None
+            };
+            Ok(Some((sym, code_info)))
         } else {
             Ok(None)
         }

src/elf/resolver.rs

@@ -13,6 +13,7 @@ use crate::inspect::FindAddrOpts;
 use crate::inspect::SymInfo;
 use crate::once::OnceCell;
 use crate::symbolize::AddrCodeInfo;
+use crate::symbolize::FindSymOpts;
 use crate::symbolize::IntSym;
 use crate::symbolize::Reason;
 use crate::symbolize::SrcLang;
@@ -151,10 +152,14 @@ impl ElfResolver {
 
 impl SymResolver for ElfResolver {
     #[cfg_attr(feature = "tracing", crate::log::instrument(fields(addr = format_args!("{addr:#x}"))))]
-    fn find_sym(&self, addr: Addr) -> Result<Result<IntSym<'_>, Reason>> {
+    fn find_sym(
+        &self,
+        addr: Addr,
+        opts: &FindSymOpts,
+    ) -> Result<Result<(IntSym<'_>, Option<AddrCodeInfo<'_>>), Reason>> {
         #[cfg(feature = "dwarf")]
         if let ElfBackend::Dwarf(dwarf) = &self.backend {
-            if let Some(sym) = dwarf.find_sym(addr)? {
+            if let Some(sym) = dwarf.find_sym(addr, opts)? {
                 return Ok(Ok(sym))
             }
         }
@@ -175,7 +180,7 @@ impl SymResolver for ElfResolver {
                     size: Some(size),
                     lang,
                 };
-                sym
+                (sym, None)
             });
 
         Ok(result)
@@ -206,20 +211,6 @@ impl SymResolver for ElfResolver {
             .for_each(|sym| sym.obj_file_name = Some(Cow::Borrowed(&self.file_name)));
         Ok(syms)
     }
-
-    #[cfg(feature = "dwarf")]
-    fn find_code_info(&self, addr: Addr, inlined_fns: bool) -> Result<Option<AddrCodeInfo<'_>>> {
-        if let ElfBackend::Dwarf(dwarf) = &self.backend {
-            dwarf.find_code_info(addr, inlined_fns)
-        } else {
-            Ok(None)
-        }
-    }
-
-    #[cfg(not(feature = "dwarf"))]
-    fn find_code_info(&self, addr: Addr, inlined_fns: bool) -> Result<Option<AddrCodeInfo<'_>>> {
-        Ok(None)
-    }
 }
 
 impl Debug for ElfResolver {

src/gsym/resolver.rs

@@ -14,6 +14,7 @@ use crate::inspect::SymInfo;
 use crate::mmap::Mmap;
 use crate::symbolize::AddrCodeInfo;
 use crate::symbolize::CodeInfo;
+use crate::symbolize::FindSymOpts;
 use crate::symbolize::IntSym;
 use crate::symbolize::Reason;
 use crate::symbolize::SrcLang;
@@ -162,7 +163,11 @@ impl<'dat> GsymResolver<'dat> {
 }
 
 impl SymResolver for GsymResolver<'_> {
-    fn find_sym(&self, addr: Addr) -> Result<Result<IntSym<'_>, Reason>> {
+    fn find_sym(
+        &self,
+        addr: Addr,
+        opts: &FindSymOpts,
+    ) -> Result<Result<(IntSym<'_>, Option<AddrCodeInfo<'_>>), Reason>> {
         if let Some(idx) = self.ctx.find_addr(addr) {
             let sym_addr = self
                 .ctx
@@ -192,7 +197,13 @@ impl SymResolver for GsymResolver<'_> {
                 lang,
             };
 
-            Ok(Ok(sym))
+            let code_info = if opts.code_info() {
+                self.find_code_info(addr, opts.inlined_fns())?
+            } else {
+                None
+            };
+
+            Ok(Ok((sym, code_info)))
         } else {
             Ok(Err(Reason::UnknownAddr))
         }
@@ -209,7 +220,9 @@ impl SymResolver for GsymResolver<'_> {
             "Gsym resolver does not currently support lookup by name",
         ))
     }
+}
 
+impl GsymResolver<'_> {
     #[cfg_attr(feature = "tracing", crate::log::instrument(skip(self), fields(file = debug(&self.file_name))))]
     fn find_code_info(&self, addr: Addr, inlined_fns: bool) -> Result<Option<AddrCodeInfo<'_>>> {
         let idx = match self.ctx.find_addr(addr) {
@@ -398,10 +411,13 @@ mod tests {
         // always fall into the inlined region, no matter toolchain. If not, add
         // padding bytes/dummy instructions and adjust some more.
         let addr = 0x200020a;
-        let sym = resolver.find_sym(addr).unwrap().unwrap();
+        let (sym, info) = resolver
+            .find_sym(addr, &FindSymOpts::CodeInfoAndInlined)
+            .unwrap()
+            .unwrap();
         assert_eq!(sym.name, "factorial_inline_test");
 
-        let info = resolver.find_code_info(addr, true).unwrap().unwrap();
+        let info = info.unwrap();
         assert_eq!(info.direct.1.line, Some(34));
         assert_eq!(info.direct.1.file, OsStr::new("test-stable-addresses.c"));
         assert_eq!(info.inlined.len(), 2);

src/kernel.rs

@@ -9,6 +9,7 @@ use crate::inspect::FindAddrOpts;
 use crate::inspect::SymInfo;
 use crate::ksym::KSymResolver;
 use crate::symbolize::AddrCodeInfo;
+use crate::symbolize::FindSymOpts;
 use crate::symbolize::IntSym;
 use crate::symbolize::Reason;
 use crate::Addr;
@@ -41,11 +42,18 @@ impl KernelResolver {
 }
 
 impl SymResolver for KernelResolver {
-    fn find_sym(&self, addr: Addr) -> Result<Result<IntSym<'_>, Reason>> {
+    fn find_sym(
+        &self,
+        addr: Addr,
+        opts: &FindSymOpts,
+    ) -> Result<Result<(IntSym<'_>, Option<AddrCodeInfo<'_>>), Reason>> {
+        // TODO: If an `ElfResolver` is available we probably should give
+        //       preference to it, if for no other reason than the fact that it
+        //       may report source code location information.
         if let Some(ksym_resolver) = self.ksym_resolver.as_ref() {
-            ksym_resolver.find_sym(addr)
+            ksym_resolver.find_sym(addr, opts)
         } else {
-            self.elf_resolver.as_ref().unwrap().find_sym(addr)
+            self.elf_resolver.as_ref().unwrap().find_sym(addr, opts)
         }
     }
 
@@ -56,14 +64,6 @@ impl SymResolver for KernelResolver {
     ) -> Result<Vec<SymInfo<'slf>>> {
         Ok(Vec::new())
     }
-
-    fn find_code_info(&self, addr: Addr, inlined_fns: bool) -> Result<Option<AddrCodeInfo>> {
-        if let Some(resolver) = self.elf_resolver.as_ref() {
-            resolver.find_code_info(addr, inlined_fns)
-        } else {
-            Ok(None)
-        }
-    }
 }
 
 impl Debug for KernelResolver {

src/ksym.rs

@@ -12,6 +12,7 @@ use crate::inspect::FindAddrOpts;
 use crate::inspect::SymInfo;
 use crate::once::OnceCell;
 use crate::symbolize::AddrCodeInfo;
+use crate::symbolize::FindSymOpts;
 use crate::symbolize::IntSym;
 use crate::symbolize::Reason;
 use crate::symbolize::SrcLang;
@@ -120,8 +121,12 @@ impl KSymResolver {
 }
 
 impl SymResolver for KSymResolver {
-    fn find_sym(&self, addr: Addr) -> Result<Result<IntSym<'_>, Reason>> {
-        let sym = self.find_ksym(addr).map(IntSym::from);
+    fn find_sym(
+        &self,
+        addr: Addr,
+        _opts: &FindSymOpts,
+    ) -> Result<Result<(IntSym<'_>, Option<AddrCodeInfo<'_>>), Reason>> {
+        let sym = self.find_ksym(addr).map(|sym| (IntSym::from(sym), None));
         Ok(sym)
     }
 
@@ -165,10 +170,6 @@ impl SymResolver for KSymResolver {
         };
         Ok(syms)
     }
-
-    fn find_code_info(&self, _addr: Addr, _inlined_fns: bool) -> Result<Option<AddrCodeInfo>> {
-        Ok(None)
-    }
 }
 
 impl Debug for KSymResolver {
@@ -235,20 +236,29 @@ mod tests {
         // Find the address of the symbol placed at the middle
         let sym = &resolver.syms[resolver.syms.len() / 2];
         let addr = sym.addr;
-        let found = resolver.find_sym(addr).unwrap().unwrap();
+        let (found, _) = resolver
+            .find_sym(addr, &FindSymOpts::Basic)
+            .unwrap()
+            .unwrap();
         ensure_addr_for_name(found.name, addr);
 
         // 0 is an invalid address.  We remove all symbols with 0 as
         // their address from the list.
-        assert!(resolver.find_sym(0).unwrap().is_err());
+        assert!(resolver.find_sym(0, &FindSymOpts::Basic).unwrap().is_err());
 
         // Find the address of the last symbol
         let sym = &resolver.syms.last().unwrap();
         let addr = sym.addr;
-        let found = resolver.find_sym(addr).unwrap().unwrap();
+        let (found, _) = resolver
+            .find_sym(addr, &FindSymOpts::Basic)
+            .unwrap()
+            .unwrap();
         ensure_addr_for_name(found.name, addr);
 
-        let found = resolver.find_sym(addr + 1).unwrap().unwrap();
+        let (found, _) = resolver
+            .find_sym(addr + 1, &FindSymOpts::Basic)
+            .unwrap()
+            .unwrap();
         // Should still find the previous symbol, which is the last one.
         ensure_addr_for_name(found.name, addr);
     }

src/resolver.rs

@@ -3,6 +3,7 @@ use std::fmt::Debug;
 use crate::inspect::FindAddrOpts;
 use crate::inspect::SymInfo;
 use crate::symbolize::AddrCodeInfo;
+use crate::symbolize::FindSymOpts;
 use crate::symbolize::IntSym;
 use crate::symbolize::Reason;
 use crate::Addr;
@@ -18,15 +19,12 @@ where
     Self: Debug,
 {
     /// Find the symbol corresponding to the given address.
-    fn find_sym(&self, addr: Addr) -> Result<Result<IntSym<'_>, Reason>>;
+    fn find_sym(
+        &self,
+        addr: Addr,
+        opts: &FindSymOpts,
+    ) -> Result<Result<(IntSym<'_>, Option<AddrCodeInfo<'_>>), Reason>>;
+
     /// Find information about a symbol given its name.
     fn find_addr(&self, name: &str, opts: &FindAddrOpts) -> Result<Vec<SymInfo<'_>>>;
-    /// Finds the source code location for a given address.
-    ///
-    /// This function tries to find source code information for the given
-    /// address. If no such information was found, `None` will be returned. If
-    /// `inlined_fns` is true, information about inlined calls at the very
-    /// address will also be looked up and reported as the optional
-    /// [`AddrCodeInfo::inlined`] attribute.
-    fn find_code_info(&self, addr: Addr, inlined_fns: bool) -> Result<Option<AddrCodeInfo<'_>>>;
 }