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

Remove ResolverMap and more #162

Merged
merged 6 commits into from
May 13, 2023

Conversation

danielocfb
Copy link
Collaborator

Please see individual commits for descriptions.

With commit a03c15e ("Remove 'symbolize' feature") we simplified
the conditional compilation logic and that enables us to expose the
ElfBackend enum from the elf module without any conflicts/potentially
unresolved dependencies.
With this change we make use of that fact, by reworking the Inspector
internal find_addr logic to use an ElfResolver on all paths. Doing so
will make follow on changes more straight forward.

Signed-off-by: Daniel Müller <deso@posteo.net>
So far the Inspector type does not report file offset or object file
name reporting. With this change we hook up this logic (copying the
existing logic from the Symbolizer type).

Signed-off-by: Daniel Müller <deso@posteo.net>
By now the Inspector type captures the functionality that
Symbolizer::find_addrs provided. Hence, remove the latter and everything
associated with it, including ResolverMap (which was only used from this
functionality).

Signed-off-by: Daniel Müller <deso@posteo.net>
We initially organized benchmarks by source type (e.g., DWARF, Gsym,
etc.). However, by now we changed the overall organization of the crate
in such a way that different areas of functionality (symbolization,
inspection, normalization) are separated.
With this change we start organizing benchmarks similarly, by moving
the DWARF symbolization benchmark into benches/symbolize.

Signed-off-by: Daniel Müller <deso@posteo.net>
This change renames the benches/dwarf module to inspect, in continuation
of our effort of restructuring benchmarks.

Signed-off-by: Daniel Müller <deso@posteo.net>
@danielocfb danielocfb requested a review from anakryiko May 10, 2023 23:51
@codecov
Copy link

codecov bot commented May 10, 2023

Codecov Report

Patch coverage: 97.05% and project coverage change: +0.21 🎉

Comparison is base (35abf6e) 74.16% compared to head (2a7ed90) 74.38%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #162      +/-   ##
==========================================
+ Coverage   74.16%   74.38%   +0.21%     
==========================================
  Files          28       27       -1     
  Lines        4382     4345      -37     
==========================================
- Hits         3250     3232      -18     
+ Misses       1132     1113      -19     
Impacted Files Coverage Δ
src/elf/resolver.rs 80.00% <ø> (+3.00%) ⬆️
src/symbolize/symbolizer.rs 63.58% <ø> (-2.03%) ⬇️
src/inspect/inspector.rs 96.96% <96.55%> (-0.99%) ⬇️
src/inspect/source.rs 70.58% <100.00%> (+12.25%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

With this change we move the Gsym symbolization benchmark into
benches/symbolize, to co-locate it with other symbolization benchmarks
and to follow the organization of the rest of the crate.

Signed-off-by: Daniel Müller <deso@posteo.net>
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

let () = syms.iter_mut().for_each(|sym| {
if opts.offset_in_file {
if let Some(off) = resolver.addr_file_off(sym.address) {
sym.file_offset = off;
Copy link
Member

Choose a reason for hiding this comment

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

just thinking out loud: should we always calculate file_offset and not even have an opts.offset_in_file?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose we could. Will check how expensive addr_file_off is and whether configurability has merrit. But let's do that as a follow-on, as the change at hand is effectively only a code "move"/copy.

@d-e-s-o d-e-s-o merged commit 893efe7 into libbpf:main May 13, 2023
@d-e-s-o d-e-s-o deleted the topic/resolver-map-no-more branch May 13, 2023 20:50
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