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

aya-obj: move code for object file loading and relocation into a separate crate #475

Merged
merged 8 commits into from
Jan 3, 2023

Conversation

yesh0
Copy link
Contributor

@yesh0 yesh0 commented Dec 28, 2022

This PR moves some platform-independent code into a separate aya-obj crate. Changes include:

  • aya::generated - Depended upon by aya::obj and moved into aya-obj as well.
    Actually, contents in aya/src/generated/linux_bindings_*.rs seem to be identical and
    platform-independent. Theses files were re-generated with rust-bindgen v0.63 with
    use_core.
  • aya::obj - The main part.
    • Documentation was added based on my (doubtful) understanding of Aya and libbpf.
    • Code relevant to maps was moved into a map module.
    • Members used by aya have their visibility changed to pub.
    • A no_std feature was added.
  • Others
    • aya::programs::CgroupSock*AttachType and some map definition types were moved.
    • aya::bpf::Features was split into Features and BtfFeatures with the latter moved.
    • An integration test using rbpf was added.

Closes #473

@netlify
Copy link

netlify bot commented Dec 28, 2022

Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 9c451a3
🔍 Latest deploy log https://app.netlify.com/sites/aya-rs-docs/deploys/63b3a77fbe91530008ac1941
😎 Deploy Preview https://deploy-preview-475--aya-rs-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@alessandrod
Copy link
Collaborator

Thanks for doing this! Just some high level comments, I'll do a proper review soon

This PR moves some platform-independent code into a separate aya-obj crate. Changes include:

* `aya::generated` - Depended upon by `aya::obj` and moved into `aya-obj` as well.
  Actually, contents in `aya/src/generated/linux_bindings_*.rs` seem to be identical and
  platform-independent. Theses files were re-generated with rust-bindgen v0.63 with
  `use_core`.

The files are identical today yes, given the bindings we generate. The input headers aren't arch independent tho, we don't control them, and we might add arch dependent bindings at some point in the future, so the best thing to do is to generate them separately.

* `aya::obj` - The main part.
  
  * Documentation was added based on my (doubtful) understanding of Aya and libbpf.

Haha thanks for this! I'll keep it in mind while reviewing.

  * Code relevant to maps was moved into a `map` module.
  * Members used by `aya` have their visibility changed to `pub`.
  * A `no_std` feature was added.

Amazing!

* Others
  
  * `aya::programs::CgroupSock*AttachType` and some map definition types were moved.

Skimming through the diff I couldn't immediately understand why this was needed? I'm assuming it has to do with dependencies on some generated stuff?

  * `aya::bpf::Features` was split into `Features` and `BtfFeatures` with the latter moved.

Cool

  * An integration test using rbpf was added.

Awesome!

@yesh0
Copy link
Contributor Author

yesh0 commented Dec 28, 2022

* Others
  
  * `aya::programs::CgroupSock*AttachType` and some map definition types were moved.

Skimming through the diff I couldn't immediately understand why this was needed? I'm assuming it has to do with dependencies on some generated stuff?

aya::programs::CgroupSock*AttachTypes are actually encoded in the section name strings, and aya-obj needs that for parsing ProgramSections. Those map types are bpf_map_def, BtfMapDef and PinningType, and they are what map definitions in the object files get decoded into.
So basically aya-obj depends on these types for parsing, and I just moved them over.

@alessandrod
Copy link
Collaborator

* Others
  
  * `aya::programs::CgroupSock*AttachType` and some map definition types were moved.

Skimming through the diff I couldn't immediately understand why this was needed? I'm assuming it has to do with dependencies on some generated stuff?

aya::programs::CgroupSock*AttachTypes are actually encoded in the section name strings, and aya-obj needs that for parsing ProgramSections.

I see, makes sense. Just as a note (mostly to self), it looks like those types are accidentally public but only used internally, we should remove the re-export but safer to do so in a separate PR.

Those map types are bpf_map_def, BtfMapDef and PinningType, and they are what map definitions in the object files get decoded into. So basically aya-obj depends on these types for parsing, and I just moved them over.

Yeah these are fine.

Well, this looks great! Thanks again for doing it, it's late here now but I'll do a proper review tomorrow.

aya-obj/README.md Outdated Show resolved Hide resolved
@alessandrod
Copy link
Collaborator

@dave-tucker tagging you in case you want to take a look too as iirc you mentioned doing something like this a while back. This looks good to me so I'm going to review and aim to merge ~soon unless you have any comments.

Copy link
Collaborator

@alessandrod alessandrod left a comment

Choose a reason for hiding this comment

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

This is a quality PR! Just a couple of minor comments then it's ready to go

aya-obj/Cargo.toml Outdated Show resolved Hide resolved
aya-obj/README.md Show resolved Hide resolved
Aya::obj depends on bindgen generated files, and we start
by migrating bindgen generated files.

This commit adds the new aya-obj crate to the workplace
and migrates generated files into the crate. We use core
instead of std in an effort to make the final crate no_std.

Bindgen was run against libbpf v1.0.1.

Refs: aya-rs#473
To split the crate into two, several changes were made:
1. Most `pub(crate)` are now `pub` to allow access from Aya;
2. Parts of BpfError are merged into, for example, RelocationError;
3. BTF part of Features is moved into the new crate;
4. `#![deny(missing_docs)]` is removed temporarily;
5. Some other code gets moved into the new crate, mainly:
   - aya::{bpf_map_def, BtfMapDef, PinningType},
   - aya::programs::{CgroupSock*AttachType},

The new crate is currenly allowing missing_docs. Member visibility
will be adjusted later to minimize exposure of implementation details.

Refs: aya-rs#473
Types relevant to maps are moved into aya_obj::maps.
Some members are marked `pub(crate)` again.

Refs: aya-rs#473
The crate has few libstd dependencies. Since it should be platform-
independent in principle, making it no_std like the object crate would
seem reasonable.

However, the feature `error_in_core` is not yet stabilized, and the
thiserror crate currently offers no no_std support. When the feature
no_std is selected, we enable the `error_in_core` feature, switch to
thiserror-core and replace the HashMap with the one in hashbrown.
This commit adds documentation on how program names are parsed from
section names, as is used by `aya_obj::Object.programs` as HashMap keys,
and updates the examples into using program names.
Copy link
Collaborator

@alessandrod alessandrod left a comment

Choose a reason for hiding this comment

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

See nit then this is ready to go :)

aya-obj/README.md Outdated Show resolved Hide resolved
- Set the version number of `aya-obj` to `0.1.0`.
- Update the description of the `aya-obj` crate.
- Add a section in README and rustdoc warning about the unstable API.
@alessandrod alessandrod merged commit 897957a into aya-rs:main Jan 3, 2023
@dave-tucker dave-tucker added aya-obj Relating to the aya-obj crate feature A PR that implements a new feature or enhancement test A PR that improves test cases or CI aya This is about aya (userspace) labels Feb 23, 2023
@qmonnet
Copy link

qmonnet commented Mar 10, 2023

Hey, I found this PR while looking at who's been using rbpf -- I just wanted to say that I'm super happy that you have some tests with rbpf in Aya! Thanks for this work! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aya This is about aya (userspace) aya-obj Relating to the aya-obj crate feature A PR that implements a new feature or enhancement test A PR that improves test cases or CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Make aya::obj API public to allow usage with different runtime
5 participants