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(dwarf): Resolve symbol names by exact address #510

Merged
merged 2 commits into from
Apr 11, 2022

Conversation

loewenheim
Copy link
Contributor

This changes the lookup of functions in the symbol table so that the symbol must have the same starting address as the function, rather than merely covering the entire function.

@loewenheim loewenheim requested a review from a team February 23, 2022 15:12
@codecov-commenter
Copy link

codecov-commenter commented Feb 23, 2022

Codecov Report

Merging #510 (ab86689) into master (7810fee) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

❗ Current head ab86689 differs from pull request most recent head 393b5f1. Consider uploading reports for the commit 393b5f1 to get more accurate results

@@            Coverage Diff             @@
##           master     #510      +/-   ##
==========================================
- Coverage   68.73%   68.66%   -0.08%     
==========================================
  Files          84       84              
  Lines       17500    17503       +3     
==========================================
- Hits        12029    12018      -11     
- Misses       5471     5485      +14     

@jan-auer
Copy link
Member

cc @mitsuhiko.

We found a case where we incorrectly matched the last (unbounded) symbol in the symbol table, but the start address was far off the DWARF function record. This made me question the approach, and I think this exact lookup is actually correct.

Do you remember which compilers were generating inferior DWARF name attributes? I'm looking for a regression test.

@mitsuhiko
Copy link
Member

Sorry for the late reply but I do not remember. There is also a non zero chance that we had to do this due to another bug somewhere else. That said, the easiest way would be to sample this on production data now.

@Swatinem Swatinem merged commit d0c89d3 into master Apr 11, 2022
@Swatinem Swatinem deleted the fix/resolve-symbol-names branch April 11, 2022 13:24
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

5 participants