add symbol and module names to StackFrame (#723)

This exposes the module_info and symbol name from debugger in the StackFrame.  This enables the stack minimization function work on function names.

src/agent/debugger/src/dbghelp.rs

@@ -407,6 +407,7 @@ impl ModuleInfo {
     }
 }
 
+#[derive(Clone, Debug, Hash, PartialEq)]
 pub struct SymInfo {
     symbol: String,
     address: u64,

src/agent/debugger/src/stack.rs

@@ -4,7 +4,6 @@
 use std::{
     fmt::{self, Display, Formatter},
     hash::{Hash, Hasher},
-    path::Path,
 };
 
 use anyhow::Result;
@@ -14,7 +13,7 @@ use serde::{Serialize, Serializer};
 use win_util::memory;
 use winapi::{shared::minwindef::DWORD, um::winnt::HANDLE};
 
-use crate::dbghelp::{self, DebugHelpGuard, ModuleInfo, SymLineInfo};
+use crate::dbghelp::{self, DebugHelpGuard, ModuleInfo, SymInfo, SymLineInfo};
 
 const UNKNOWN_MODULE: &str = "<UnknownModule>";
 
@@ -39,60 +38,30 @@ impl From<&SymLineInfo> for FileInfo {
     }
 }
 
-#[derive(Clone, Debug, Hash, PartialEq)]
-pub struct DebugFunctionLocation {
-    /// File/line if available
-    ///
-    /// Should be stable - ASLR and JIT should not change source position,
-    /// but some precision is lost.
-    ///
-    /// We mitigate this loss of precision by collecting multiple samples
-    /// for the same hash bucket.
-    pub file_info: Option<FileInfo>,
-
-    /// Offset if line information not available.
-    pub displacement: u64,
-}
-
-impl DebugFunctionLocation {
-    pub fn new(displacement: u64) -> Self {
-        DebugFunctionLocation {
-            displacement,
-            file_info: None,
-        }
-    }
-
-    pub fn new_with_file_info(displacement: u64, file_info: FileInfo) -> Self {
-        DebugFunctionLocation {
-            displacement,
-            file_info: Some(file_info),
-        }
-    }
-}
-
-impl Display for DebugFunctionLocation {
-    fn fmt(&self, formatter: &mut Formatter) -> fmt::Result {
-        if let Some(file_info) = &self.file_info {
-            write!(formatter, "{}", file_info)?;
-        } else {
-            write!(formatter, "0x{:x}", self.displacement)?;
-        }
-        Ok(())
-    }
-}
-
 #[derive(Clone, Debug, Hash, PartialEq)]
 pub enum DebugStackFrame {
     Frame {
-        function: String,
+        module_name: String,
-        location: DebugFunctionLocation,
+        module_offset: u64,
+        symbol: Option<SymInfo>,
+        file_info: Option<FileInfo>,
     },
     CorruptFrame,
 }
 
 impl DebugStackFrame {
-    pub fn new(function: String, location: DebugFunctionLocation) -> DebugStackFrame {
+    pub fn new(
-        DebugStackFrame::Frame { function, location }
+        module_name: String,
+        module_offset: u64,
+        symbol: Option<SymInfo>,
+        file_info: Option<FileInfo>,
+    ) -> DebugStackFrame {
+        DebugStackFrame::Frame {
+            module_name,
+            module_offset,
+            symbol,
+            file_info,
+        }
     }
 
     pub fn corrupt_frame() -> DebugStackFrame {
@@ -110,13 +79,31 @@ impl DebugStackFrame {
 impl Display for DebugStackFrame {
     fn fmt(&self, formatter: &mut Formatter) -> fmt::Result {
         match self {
-            DebugStackFrame::Frame { function, location } => {
+            DebugStackFrame::Frame {
-                if let Some(file_info) = &location.file_info {
+                module_name,
-                    write!(formatter, "{} {}", function, file_info)
+                module_offset,
-                } else {
+                symbol,
-                    write!(formatter, "{}+0x{:x}", function, location.displacement)
+                file_info,
+            } => match (symbol, file_info) {
+                (Some(symbol), Some(file_info)) => write!(
+                    formatter,
+                    "{}!{}+0x{:x} {}",
+                    module_name,
+                    symbol.symbol(),
+                    symbol.displacement(),
+                    file_info
+                ),
+                (Some(symbol), None) => write!(
+                    formatter,
+                    "{}!{}+0x{:x}",
+                    module_name,
+                    symbol.symbol(),
+                    symbol.displacement(),
+                ),
+                _ => {
+                    write!(formatter, "{}+0x{:x}", module_name, module_offset)
                 }
-            }
+            },
             DebugStackFrame::CorruptFrame => formatter.write_str("<corrupt frame(s)>"),
         }
     }
@@ -145,7 +132,7 @@ impl DebugStack {
         // Corrupted stacks and jit can result in stacks that vary from run to run, so we exclude
         // those frames and anything below them for a more stable hash.
         let first_unstable_frame = self.frames.iter().position(|f| match f {
-            DebugStackFrame::Frame { function, .. } => function == UNKNOWN_MODULE,
+            DebugStackFrame::Frame { module_name, .. } => module_name == UNKNOWN_MODULE,
             DebugStackFrame::CorruptFrame => true,
         });
 
@@ -182,34 +169,26 @@ fn get_function_location_in_module(
     program_counter: u64,
     inline_context: DWORD,
 ) -> DebugStackFrame {
+    let module_name = module_info.name().to_string_lossy().to_string();
+    let module_offset = program_counter - module_info.base_address();
+
     if let Ok(sym_info) =
         dbghlp.sym_from_inline_context(process_handle, program_counter, inline_context)
     {
-        let function = format!(
+        let file_info =
-            "{}!{}",
+            match dbghlp.sym_get_file_and_line(process_handle, program_counter, inline_context) {
-            Path::new(module_info.name()).display(),
+                // Don't use file/line for these magic line numbers.
-            sym_info.symbol()
+                Ok(ref sym_line_info) if !sym_line_info.is_fake_line_number() => {
-        );
+                    Some(sym_line_info.into())
+                }
+                _ => None,
+            };
 
-        let sym_line_info =
+        DebugStackFrame::new(module_name, module_offset, Some(sym_info), file_info)
-            dbghlp.sym_get_file_and_line(process_handle, program_counter, inline_context);
-
-        let displacement = sym_info.displacement();
-        let location = match sym_line_info {
-            // Don't use file/line for these magic line numbers.
-            Ok(ref sym_line_info) if !sym_line_info.is_fake_line_number() => {
-                DebugFunctionLocation::new_with_file_info(displacement, sym_line_info.into())
-            }
-
-            _ => DebugFunctionLocation::new(displacement),
-        };
-
-        DebugStackFrame::new(function, location)
     } else {
         // No function - assume we have an exe with no pdb (so no exports). This should be
         // common, so we won't report an error. We do want a nice(ish) location though.
-        let location = DebugFunctionLocation::new(program_counter - module_info.base_address());
+        DebugStackFrame::new(module_name, module_offset, None, None)
-        DebugStackFrame::new(module_info.name().to_string_lossy().into(), location)
     }
 }
 
@@ -222,12 +201,11 @@ fn get_frame_with_unknown_module(process_handle: HANDLE, program_counter: u64) -
     match memory::get_memory_info(process_handle, program_counter) {
         Ok(mi) => {
             if mi.is_executable() {
-                let offset = program_counter
+                let module_offset = program_counter
                     .checked_sub(mi.base_address())
                     .expect("logic error computing fake rva");
 
-                let location = DebugFunctionLocation::new(offset);
+                DebugStackFrame::new(UNKNOWN_MODULE.to_owned(), module_offset, None, None)
-                DebugStackFrame::new(UNKNOWN_MODULE.into(), location)
             } else {
                 DebugStackFrame::corrupt_frame()
             }
@@ -292,20 +270,19 @@ mod test {
     use super::*;
 
     macro_rules! frame {
-        ($name: expr, disp: $disp: expr) => {
+        ($module: expr, disp: $location: expr) => {
-            DebugStackFrame::new($name.to_string(), DebugFunctionLocation::new($disp))
+            DebugStackFrame::new($module.to_string(), $location, None, None)
         };
 
-        ($name: expr, disp: $disp: expr, line: ($file: expr, $line: expr)) => {
+        ($module: expr, disp: $location: expr, line: ($file: expr, $line: expr)) => {
             DebugStackFrame::new(
-                $name.to_string(),
+                $module.to_string(),
-                DebugFunctionLocation::new_with_file_info(
+                $location,
-                    $disp,
+                None,
-                    FileInfo {
+                Some(FileInfo {
-                        file: $file.to_string(),
+                    file: $file.to_string(),
-                        line: $line,
+                    line: $line,
-                    },
+                }),
-                ),
             )
         };
     }
@@ -321,7 +298,7 @@ mod test {
 
         // Hard coded hash constant is what we want to ensure
         // the hash function is relatively stable.
-        assert_eq!(stack.stable_hash(), 4643290346391834992);
+        assert_eq!(stack.stable_hash(), 3072338388009340488);
     }
 
     #[test]

src/agent/onefuzz/src/input_tester.rs

@@ -148,16 +148,24 @@ impl<'a> Tester<'a> {
                         line: f.to_string(),
                         ..Default::default()
                     },
-                    debugger::stack::DebugStackFrame::Frame { function, location } => StackEntry {
+                    debugger::stack::DebugStackFrame::Frame {
+                        module_name,
+                        module_offset,
+                        symbol,
+                        file_info,
+                    } => StackEntry {
                         line: f.to_string(),
-                        function_name: Some(function.to_owned()), // TODO: this includes both the module & symbol
+                        function_name: symbol.as_ref().map(|x| x.symbol().to_owned()),
-                        address: Some(location.displacement),
+                        function_offset: symbol.as_ref().map(|x| x.displacement()),
-                        module_offset: None,
+                        address: None,
-                        module_path: None,
+                        module_offset: Some(*module_offset),
-                        source_file_line: location.file_info.as_ref().map(|x| x.line.into()),
+                        module_path: Some(module_name.to_owned()),
-                        source_file_name: location.file_info.as_ref().map(|x| x.file.to_string()),
+                        source_file_line: file_info.as_ref().map(|x| x.line.into()),
-                        source_file_path: None,
+                        source_file_name: file_info
-                        function_offset: None,
+                            .as_ref()
+                            .map(|x| x.file.rsplit_terminator('\\').next().map(|x| x.to_owned()))
+                            .flatten(),
+                        source_file_path: file_info.as_ref().map(|x| x.file.to_string()),
                     },
                 })
                 .collect();