feat: save file and line info for symbols #126
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements file and line information storage for symbols by integrating DWARF debug information. The changes add the ability to extract source code location data (file names and line numbers) from binary debug information and associate it with existing symbols.
Key changes:
- Added new
debug_infomodule with DWARF parsing capabilities usingaddr2lineandgimlicrates - Modified
ModuleSymbolsto storeload_biasseparately and provide accessor methods - Updated symbol processing workflow to generate debug info files alongside existing perf maps
Reviewed Changes
Copilot reviewed 11 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/run/runner/wall_time/perf/debug_info.rs | New module implementing DWARF debug info extraction and file/line lookup for symbols |
| src/run/runner/wall_time/perf/perf_map.rs | Refactored to separate load_bias storage and defer address adjustment to output time |
| src/run/runner/wall_time/perf/mod.rs | Integration of debug info generation into the benchmark data processing pipeline |
| Cargo.toml | Added dependencies for DWARF parsing: addr2line, gimli, and wholesym |
| .gitattributes | Added LFS tracking for new debug info snapshot test files |
| Various snapshot files | Updated test snapshots reflecting changes to ModuleSymbols structure |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| .filter_map(|symbol| { | ||
| let (file, line) = match ctx.find_location(symbol.addr) { | ||
| Ok(Some(location)) => { | ||
| let file = location.file.map(|f| f.to_string())?; |
There was a problem hiding this comment.
Using ? operator inside filter_map closure will cause the entire iteration to terminate on the first None result from location.file.map(). This should handle the None case explicitly and continue iteration instead of stopping.
| let file = location.file.map(|f| f.to_string())?; | |
| let file = match location.file { | |
| Some(f) => f.to_string(), | |
| None => return None, | |
| }; |
Copilot uses AI. Check for mistakes.
| Ok(Some(location)) => { | ||
| let file = location.file.map(|f| f.to_string())?; | ||
| (file, location.line) | ||
| } |
There was a problem hiding this comment.
[nitpick] The pattern matching could be simplified by handling the file extraction within the match arm more clearly. Consider using let Some(file) = location.file pattern or explicit None handling to avoid the ? operator issue.
| Ok(Some(location)) => { | |
| let file = location.file.map(|f| f.to_string())?; | |
| (file, location.line) | |
| } | |
| Ok(Some(location)) => match location.file { | |
| Some(f) => (f.to_string(), location.line), | |
| None => return None, | |
| }, |
Copilot uses AI. Check for mistakes.
|
MOving this to #128 , accidentally used the wrong branch name. |
1 participant
No description provided.