Skip to content

Conversation

@not-matthias
Copy link
Member

No description provided.

@not-matthias not-matthias requested review from art049 and Copilot and removed request for Copilot October 13, 2025 09:07
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds the ability to save file/line debug information for symbols by implementing DWARF debug info parsing functionality. The feature extracts source file locations for symbols and saves them as JSON files alongside existing perf maps.

Key changes:

  • Added debug info extraction using DWARF parsing via addr2line and gimli crates
  • Modified symbol storage to separate load bias from symbol addresses for better debug info correlation
  • Updated test snapshots to reflect the new data structure changes

Reviewed Changes

Copilot reviewed 11 out of 16 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/run/runner/wall_time/perf/debug_info.rs New module implementing DWARF debug info parsing and file/line extraction for symbols
src/run/runner/wall_time/perf/perf_map.rs Modified ModuleSymbols to track load_bias separately and updated symbol address handling
src/run/runner/wall_time/perf/mod.rs Integrated debug info generation into the benchmark data saving process
Cargo.toml Added dependencies for DWARF parsing: addr2line, gimli, and wholesym
Various snapshot files Updated test snapshots to reflect new data structures and added new debug info snapshots
.gitattributes Added LFS tracking for new debug info snapshot files

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@not-matthias not-matthias force-pushed the cod-1310-add-fileline-for-parsing-node-origin-in-rustcpp branch from 4e3ee83 to d68b5f6 Compare October 14, 2025 13:08
Copy link
Member

@art049 art049 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall! (not a thorough review)
I'll let @GuillaumeLagrange do the final review and ask for some clarification if needed.

Copy link
Contributor

@GuillaumeLagrange GuillaumeLagrange left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will finish the review when the file format stuff is sorted out, but OLGTM, minus the fixup not fixed up

@not-matthias not-matthias force-pushed the cod-1310-add-fileline-for-parsing-node-origin-in-rustcpp branch 2 times, most recently from 573ef3b to 962d55a Compare October 17, 2025 10:33
@not-matthias not-matthias requested a review from Copilot October 17, 2025 10:36
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 16 out of 19 changed files in this pull request and generated 3 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@not-matthias not-matthias force-pushed the cod-1310-add-fileline-for-parsing-node-origin-in-rustcpp branch 2 times, most recently from 44cbc6a to a488351 Compare October 17, 2025 10:55
@art049
Copy link
Member

art049 commented Oct 20, 2025

You need to rebase on main to have the CI passing otherwise there is a chance perf will not be found before #132

@not-matthias not-matthias force-pushed the cod-1310-add-fileline-for-parsing-node-origin-in-rustcpp branch from a488351 to e193696 Compare October 20, 2025 10:33
@not-matthias not-matthias force-pushed the cod-1310-add-fileline-for-parsing-node-origin-in-rustcpp branch from e193696 to baf312a Compare October 20, 2025 10:51
@not-matthias not-matthias merged commit baf312a into main Oct 20, 2025
9 checks passed
@not-matthias not-matthias deleted the cod-1310-add-fileline-for-parsing-node-origin-in-rustcpp branch October 20, 2025 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants