Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(debuginfo): include more pdb symbols #641

Merged
merged 1 commit into from
Jul 29, 2022

Conversation

mstange
Copy link
Contributor

@mstange mstange commented Jul 28, 2022

Instead of checking symbol.function, check whether the symbol is contained
in an executable section. This matches what pdb-addr2line does, and is
closer to what dump_syms' handwritten PDB parsing does.

This change allows more symbols to be included. For example, it allows the
following symbols from basic-opt32.pdb to pass the check:

4f9d7 _rtinfpopse
4f9d9 _rtinfnpopse
4f9e9 _fFLN
4fa9e _rtinfpop
4faa0 _rtinfnpop
4fabd _ffexpm1

https://github.com/mozilla/dump_syms/blob/54f9e6240e34cf17fc7dc60ac4985772e2c20001/test_data/windows/basic-opt32.pdb

These symbols have symbol.function == false, so they were being filtered
out before.

The change also adds over 300 symbols for ntdll.pdb BD298DA990CD4BF9BE5CE4796D7924C61.

The handwritten PDB parsing in dump_syms currently uses this full check:
symbol.function || symbol.code || is_in_executable_section || is_in_executable_section_contribution

This seems like overkill; I've tested a few pdbs and haven't found any
for which this additional complexity would make a difference.

Instead of checking symbol.function, check whether the symbol is contained
in an executable section. This matches what pdb-addr2line does, and is
closer to what dump_syms' handwritten PDB parsing does.

This change allows more symbols to be included. For example, it allows the
following symbols from basic-opt32.pdb to pass the check:

```
4f9d7 _rtinfpopse
4f9d9 _rtinfnpopse
4f9e9 _fFLN
4fa9e _rtinfpop
4faa0 _rtinfnpop
4fabd _ffexpm1
```

https://github.com/mozilla/dump_syms/blob/54f9e6240e34cf17fc7dc60ac4985772e2c20001/test_data/windows/basic-opt32.pdb

These symbols have symbol.function == false, so they were being filtered
out before.

The change also adds over 300 symbols for ntdll.pdb BD298DA990CD4BF9BE5CE4796D7924C61.

The handwritten PDB parsing in dump_syms currently uses this full check:
`symbol.function || symbol.code || is_in_executable_section || is_in_executable_section_contribution`

This seems like overkill; I've tested a few pdbs and haven't found any
for which this additional complexity would make a difference.
@mstange mstange requested a review from a team July 28, 2022 20:26
@codecov-commenter
Copy link

Codecov Report

Merging #641 (a5430ed) into master (a0360d3) will increase coverage by 0.04%.
The diff coverage is 92.30%.

@@            Coverage Diff             @@
##           master     #641      +/-   ##
==========================================
+ Coverage   71.20%   71.25%   +0.04%     
==========================================
  Files          96       96              
  Lines       18408    18433      +25     
==========================================
+ Hits        13108    13134      +26     
+ Misses       5300     5299       -1     

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.

None yet

3 participants