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

Introduce Reason enumeration to accompany Symbolized::Unknown variant #446

Merged
merged 3 commits into from
Dec 18, 2023

Conversation

d-e-s-o
Copy link
Collaborator

@d-e-s-o d-e-s-o commented Dec 15, 2023

Many paths can lead to an unsuccessful symbolization, and it may be hard for callers to figure out why symbolization didn't pan out. For example, a user may have provided a stripped binary, without being aware of that fact.

With this change we introduce the symbolize::Reason enumeration, which now accompanies the Symbolized::Unknown variant and aims to provide a best guess as to why symbolization was not successful.

This change adjusts the internally used Handler trait with an additional
generic argument that can be used for providing additional data to the
Handler::handle_unknown_addr() method call.
The ability to provide this data will be necessary with upcoming
changes.

Signed-off-by: Daniel Müller <deso@posteo.net>
Many paths can lead to an unsuccessful symbolization, and it may be hard
for callers to figure out why symbolization didn't pan out. For example,
a user may have provided a stripped binary, without being aware of that
fact.
With this change we introduce the symbolize::Reason enumeration, which
now accompanies the Symbolized::Unknown variant and aims to provide a
best guess as to why symbolization was not successful.

Signed-off-by: Daniel Müller <deso@posteo.net>
@d-e-s-o d-e-s-o force-pushed the topic/unknown-reason branch from a049e1f to 628fdb0 Compare December 15, 2023 00:36
Copy link

codecov bot commented Dec 15, 2023

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (3c11f30) 92.26% compared to head (4c30ba9) 92.30%.
Report is 1 commits behind head on main.

Files Patch % Lines
src/symbolize/symbolizer.rs 66.66% 4 Missing ⚠️
src/symbolize/mod.rs 84.21% 3 Missing ⚠️
src/gsym/resolver.rs 50.00% 2 Missing ⚠️
src/kernel.rs 0.00% 1 Missing ⚠️
src/ksym.rs 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #446      +/-   ##
==========================================
+ Coverage   92.26%   92.30%   +0.04%     
==========================================
  Files          41       41              
  Lines        6347     6397      +50     
==========================================
+ Hits         5856     5905      +49     
- Misses        491      492       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@d-e-s-o d-e-s-o force-pushed the topic/unknown-reason branch from 628fdb0 to e7d9eb4 Compare December 15, 2023 23:08
@d-e-s-o d-e-s-o changed the title Topic/unknown reason Introduce Reason enumeration to accompany Symbolized::Unknown variant Dec 15, 2023
@d-e-s-o d-e-s-o requested a review from anakryiko December 15, 2023 23:14
@d-e-s-o d-e-s-o marked this pull request as ready for review December 15, 2023 23:14
@danielocfb
Copy link
Collaborator

Should be ready for review now.

Copy link
Member

@anakryiko anakryiko left a comment

Choose a reason for hiding this comment

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

LGTM, minor naming suggestion, but up to you.

src/symbolize/mod.rs Outdated Show resolved Hide resolved
tests/blazesym.rs Show resolved Hide resolved
This change adds a test attempting symbolization on a stripped binary.
The test makes sure that the reason we report for the unsuccessful
symbolization is the fact that symbols are missing.

Signed-off-by: Daniel Müller <deso@posteo.net>
@d-e-s-o d-e-s-o force-pushed the topic/unknown-reason branch from e7d9eb4 to 4c30ba9 Compare December 18, 2023 15:40
@d-e-s-o d-e-s-o merged commit cdde707 into libbpf:main Dec 18, 2023
29 checks passed
@d-e-s-o d-e-s-o deleted the topic/unknown-reason branch December 18, 2023 15:43
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.

4 participants