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

Streamline APK ELF build ID retrieval #227

Merged
merged 4 commits into from
Jun 15, 2023

Conversation

d-e-s-o
Copy link
Collaborator

@d-e-s-o d-e-s-o commented Jun 9, 2023

Please see individual commits for descriptions.

@codecov
Copy link

codecov bot commented Jun 9, 2023

Codecov Report

Patch coverage: 96.77% and project coverage change: +0.53 🎉

Comparison is base (52d0513) 84.28% compared to head (64954a6) 84.82%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #227      +/-   ##
==========================================
+ Coverage   84.28%   84.82%   +0.53%     
==========================================
  Files          26       26              
  Lines        3800     3783      -17     
==========================================
+ Hits         3203     3209       +6     
+ Misses        597      574      -23     
Impacted Files Coverage Δ
src/normalize/meta.rs 100.00% <ø> (ø)
src/symbolize/symbolizer.rs 62.19% <94.73%> (+6.63%) ⬆️
src/normalize/normalizer.rs 85.75% <100.00%> (+2.85%) ⬆️

... 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.

This change concludes the work for support APK ELF addresses by adding
symbolization support, in addition to the previously covered
normalization.

Closes: libbpf#175

Signed-off-by: Daniel Müller <[email protected]>
@d-e-s-o
Copy link
Collaborator Author

d-e-s-o commented Jun 9, 2023

Including the remaining pieces for APK ELF address symbolization now as well.

@d-e-s-o d-e-s-o force-pushed the topic/opt-apk-elf-norm branch 2 times, most recently from 696b5fc to e374fc6 Compare June 9, 2023 21:27
@d-e-s-o
Copy link
Collaborator Author

d-e-s-o commented Jun 9, 2023

Patch coverage: 96.77% and project coverage change: +0.54 tada

Comparison is base (e25f7d4) 84.61% compared to head (e374fc6) 85.15%.

85% code coverage!

This change optimizes the reading of build IDs from ELF files in APKs.
Specifically, we stop re-mmapping the entire archive and searching for the
ELF file in it by path and just work on the mmap we created initially.

Signed-off-by: Daniel Müller <[email protected]>
Now that we have eliminated the re-mapping of APK archives for retrieval
of the build ID of the contained ELF file, we no longer need the
BuildIdReader::read_build_id_from_apk_elf method. Remove it.

Signed-off-by: Daniel Müller <[email protected]>
We want APK ELF addresses to be normalized relative to the ELF file, as
that allows for easy symbolization if the ELF file is available. The
current code, however, report an address normalized to the containing
APK.
This change adjusts the logic and documents the property. We also start
adding more doc tests.

Signed-off-by: Daniel Müller <[email protected]>
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

src/normalize/meta.rs Outdated Show resolved Hide resolved
@danielocfb danielocfb merged commit 0313c9f into libbpf:main Jun 15, 2023
22 checks passed
@danielocfb danielocfb deleted the topic/opt-apk-elf-norm branch June 15, 2023 16:56
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