[agent/srcview] Parse inline sites for additional source lines. #3362
Conversation
Previously, inlined functions DO NOT get properly added to the source
lines because those records of line entries are recorded via an
S_INLINESITE symbol data, with more information located in the
inlinee debug subsection.
Required some major refactoring in pdbcache, and fundamentally, the
modoffs to lines will fundamentally be changed from a 1:1 relationship to a
1:n relationship, since there could be
(a) multiple lines that correspond to some modoff if that particular
modoff corresponds to a multiline statement.
(b) multiple lines from potentially different files due to the
possibly inlined functions.
|
Oops, totally forgot to join microsoft org before making this PR. Should I just create a new PR or can we still salvage this? |
wanghenry-msft
changed the title
Codecov Report
@@ Coverage Diff @@
## main #3362 +/- ##
==========================================
- Coverage 31.68% 30.53% -1.15%
==========================================
Files 308 133 -175
Lines 37626 14328 -23298
==========================================
- Hits 11921 4375 -7546
+ Misses 25705 9953 -15752 |
| // NOTE: BA_OP_ChangeCodeOffsetBase is actually wrong in pdb crate package | ||
| // See https://dev.azure.com/mseng/LLVM/_versionControl?path=%24/LLVM/pu/WinC/vctools/PDB/dia2/symcache.cpp&version=T&line=3453&lineEnd=3453&lineStartColumn=18&lineEndColumn=36&lineStyle=plain&_a=contents | ||
| // for what it should actually be. The value is actually an index into the | ||
| // nth S_SEPCODE entry which contains a section + offset value |
There was a problem hiding this comment.
Are we contributing a fix for this as well?
There was a problem hiding this comment.
Not sure tbh, since I'm not even sure that msvc actually emits BA_OP_ChangeCodeOffsetBase opcodes anyways in the PDB
No, that's not a problem. We do require that the code is rustfmt'd and clippy-clean though 🙂 |
| .flat_map(|sl| sl.map(|e| e.clone())) | ||
| .collect(); | ||
|
|
||
| // Generate our report, filtering on our example path |
There was a problem hiding this comment.
Small thing: just looking at Report::new, we could have it take an impl Iterator<Item = &SrcLine> which would avoid the need to instantiate a Vec here and also avoid cloning all the SrcLines. It looks a little cleaner as well:
// Convert our ModOffs to SrcLine so we can draw it
let coverage = modoffs
.into_iter()
.filter_map(|m| srcview.modoff(&m))
.flatten();
// Generate our report, filtering on our example path
let r = Report::new(coverage, &srcview, Some(r"E:\\1f\\coverage\\example")).unwrap();
Co-authored-by: George Pollard <porges@porg.es>
|
so apparently I also found out that cross module indexes are a thing and also need to be accounted for. |
|
|
||
| offset_to_line.push(srcloc.clone()); | ||
| symbol_to_lines.push(srcloc.clone()); | ||
| path_to_symbols.push(proc_name.to_string()); |
There was a problem hiding this comment.
| path_to_symbols.push(proc_name.to_string()); | |
| path_to_symbols.push(proc_name.to_string()); |
This line in particular could be hoisted out of the loop since it will be reinserting the same thing each time. It might be worth making path_to_symbols map to a BTreeSet?
|
This PR has been moved to an internal fork. And can be closed, as srcview is not maintained nor owned by onefuzz. |
4 participants
Summary of the Pull Request
Previously in srcview, the inlined functions DO NOT get properly added to the source lines because those records of line entries are recorded via an S_INLINESITE symbol data, with more information located in the inlinee debug subsection.
Required some major refactoring in pdbcache, and fundamentally, the modoffs to lines will fundamentally be changed from a 1:1 relationship to a 1:n relationship, since there could be:
PR Checklist
Info on Pull Request
What does this include?
Validation Steps Performed
How does someone test & validate?