refactor(aya-ebpf): make btf_maps libbpf-compatible

[Brskt]

Summary

• Remove the btf_map_def! macro which wrapped fields in UnsafeCell, creating nested BTF structures that libbpf couldn't parse
• Convert Array, RingBuf, and SkStorage to use flat #[repr(C)] structs with direct pointer fields
• Add C BPF program and Rust test to verify BTF map definitions are compatible with libbpf's parser

Closes #1455

Test plan

• Existing integration tests pass (array, sk_storage)
• New btf_maps test verifies C program with BTF maps loads correctly
• BTF structure verified with bpftool btf dump - produces flat anonymous structs
• Verified libbpf (via bpftool) correctly parses the BTF map definitions

---

This change is 

[netlify]

✅ Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	1fa7ee2
🔍 Latest deploy log	https://app.netlify.com/projects/aya-rs-docs/deploys/6977fcd5a265db00086be95c
😎 Deploy Preview	https://deploy-preview-1457--aya-rs-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[vadorovsky]

Just one nit.

We can also remove this part of loader, since we don't have to traverse the UnsafeCell:

aya/aya-obj/src/obj.rs

Lines 1270 to 1311 in be872b1

	// In aya-ebpf, the BTF map definition types are not used directly in the
	// map variables. Instead, they are wrapped in two nested types:
	//
	// * A struct representing the map type (e.g., `Array`, `HashMap`) that
	// provides methods for interacting with the map type (e.g.
	// `HashMap::get`, `RingBuf::reserve`).
	// * It has a single field with name `__0`.
	// * An `UnsafeCell`, which informs the Rust compiler that the type is
	// thread-safe and can be safely mutated even as a global variable. The
	// kernel guarantees map operation safety.
	// * It has a single field with name `value`.
	//
	// Therefore, the traversal to the actual map definition looks like:
	//
	// HashMap -> __0 -> value
	let mut s = s;
	for (index, expected_field_name) in ["__0", "value"].into_iter().enumerate() {
	match s.members.as_slice() {
	[m] => {
	let field_name = btf.string_at(m.name_offset)?;
	if field_name.as_ref() != expected_field_name {
	return Err(BtfError::UnexpectedBtfMapWrapperLayout(s.clone()));
	}
	s = match btf.type_by_id(m.btf_type)? {
	BtfType::Struct(s) => s,
	_ => {
	return Err(BtfError::UnexpectedBtfType {
	type_id: m.btf_type,
	});
	}
	};
	}
	// If the first wrapper level is missing, use the original struct.
	_ => {
	if index == 0 {
	break;
	} else {
	return Err(BtfError::UnexpectedBtfMapWrapperLayout(s.clone()));
	}
	}
	}
	}

24890fa#diff-437eb49e9290dc56055a92b4278828b430176ce90c580ce8b5e660c09463868e

But up to you if you want to do it in this PR.

[Brskt]

Done, removed the UnsafeCell traversal code since it's no longer needed.

@Brskt made 2 comments.
Reviewable status: 0 of 12 files reviewed, 2 unresolved discussions (waiting on @tamird and @vadorovsky).

[tamird]

@tamird partially reviewed 8 files and made 3 comments.
Reviewable status: 3 of 12 files reviewed, 4 unresolved discussions (waiting on @Brskt and @vadorovsky).

---

ebpf/aya-ebpf/src/btf_maps/mod.rs line 27 at r4 (raw file):

/// compatible with both aya and libbpf loaders.
macro_rules! btf_map_def {
    // Variant for maps with <T, const M: usize, const F: usize> generics

these two variants are almost complete copies, this is classic LLM slop. can you get it down to a single variant please?

---

ebpf/aya-ebpf/src/btf_maps/mod.rs line 52 at r4 (raw file):

        impl<T, const M: usize, const F: usize> $name<T, M, F> {
            #[expect(

to be fair we could trivially implement Default by delegating to this method.

[tamird]

@tamird made 1 comment and resolved 1 discussion.
Reviewable status: 3 of 12 files reviewed, 4 unresolved discussions (waiting on @Brskt and @vadorovsky).

---

test/integration-test/src/tests/btf_maps.rs line 5 at r5 (raw file):

//! This test loads a C BPF program that uses BTF map definitions.
//! If this works, it proves the BTF structure is valid for libbpf.
//! Our Rust btf_maps should produce equivalent BTF structures.

I don't understand how this test is relevant to the change we're making in this PR. It's testing that we can load libbpf BTF maps, but the change is about making our BTF maps loadable by libbpf.

[Brskt]

@Brskt made 4 comments.
Reviewable status: 2 of 16 files reviewed, 4 unresolved discussions (waiting on @tamird and @vadorovsky).

---

ebpf/aya-ebpf/src/btf_maps/mod.rs line 27 at r4 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

these two variants are almost complete copies, this is classic LLM slop. can you get it down to a single variant please?

Done.

---

ebpf/aya-ebpf/src/btf_maps/mod.rs line 52 at r4 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

to be fair we could trivially implement Default by delegating to this method.

Done.

---

test/integration-test/src/tests/btf_maps.rs line 5 at r5 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

I don't understand how this test is relevant to the change we're making in this PR. It's testing that we can load libbpf BTF maps, but the change is about making our BTF maps loadable by libbpf.

Done. Replaced with a test that uses libbpf-rs to load a Rust eBPF program with btf_maps, which actually tests what this PR changes.

[Brskt]

The libbpf compatibility test requires libelf-dev to be installed. I made it optional (behind libbpf-test feature) so the CI passes for now. To enable the test in CI, libelf-dev would need to be added to the workflow. Is there another way to test libbpf compatibility?

[tamird]

We can add libelf-dev to the workflow.

@tamird reviewed 14 files and all commit messages, made 13 comments, and resolved 3 discussions.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @Brskt and @vadorovsky).

---

test/integration-test/src/tests/btf_maps.rs line 6 at r7 (raw file):

//! is compatible with libbpf's loader.
//!
//! This test requires the `libbpf-test` feature and system `libelf-dev` to be installed.

does it require libelf-dev just at build time or at run time as well?

---

test/integration-test/src/tests/btf_maps.rs line 22 at r7 (raw file):

    // Verify libbpf can see the BTF map
    let maps: Vec<_> = obj.maps().collect();

how about collecting the names and asserting it includes BTF_ARRAY; and if it doesn't, emit the list of names.

---

test/integration-test/Cargo.toml line 17 at r7 (raw file):

[features]
libbpf-test = ["dep:libbpf-rs"]

it'd be great to remove this feature, provided we can reliably install what's needed. i suspect cross compilation on the mac VM will not be so simple

---

test/integration-test/Cargo.toml line 21 at r7 (raw file):

[dependencies]
aya = { path = "../../aya", version = "^0.13.2", default-features = false }
libbpf-rs = { version = "0.25", optional = true }

please put this in the workspace Cargo.toml; we like to keep all the versions declared in just one place

---

ebpf/aya-ebpf/src/btf_maps/mod.rs line 29 at r6 (raw file):

    (
        $(#[$attr:meta])*
        $vis:vis struct $name:ident<T $(, const $cg_name:ident : usize $(= $cg_default:tt)?)*>,

this cg_default can be expr instead of tt, right?

---

ebpf/aya-ebpf/src/btf_maps/mod.rs line 38 at r6 (raw file):

    ) => {
        $(#[$attr])*
        #[repr(C)]

for my edification: is repr(c) needed?

---

ebpf/aya-ebpf/src/btf_maps/mod.rs line 39 at r6 (raw file):

        $(#[$attr])*
        #[repr(C)]
        #[allow(dead_code)]

should this be expect with a reason, just like we had before?

---

ebpf/aya-ebpf/src/btf_maps/mod.rs line 43 at r6 (raw file):

            r#type: *const [i32; $crate::bindings::bpf_map_type::$map_type as usize],
            key: *const $key_ty,
            value: *const T,

consider un-teaching the macro about the type parameters; instead require the invoker to provide this. that will generalize better when we want to support maps which will have K, V type parameters.

---

ebpf/aya-ebpf/src/btf_maps/mod.rs line 46 at r6 (raw file):

            max_entries: *const [i32; $max_entries],
            map_flags: *const [i32; $map_flags],
            $($extra_field: $extra_ty,)*

please restore the blank lines surrounding this

---

ebpf/aya-ebpf/src/btf_maps/mod.rs line 66 at r6 (raw file):

                    max_entries: ::core::ptr::null(),
                    map_flags: ::core::ptr::null(),
                    $($extra_field: ::core::ptr::null(),)*

can this move back to where it was?

---

ebpf/aya-ebpf/src/btf_maps/array.rs line 14 at r6 (raw file):

impl<T, const M: usize, const F: usize> Array<T, M, F> {
    /// Creates a new [`Array`] instance with elements of type `T`, maximum
    /// capacity of `M` and additional flags `F`.

should this mention the meanings of M an dF?

---

ebpf/aya-ebpf/src/btf_maps/mod.rs line 38 at r6 (raw file):

            $($field: $ty,)*

            // Anonymize the struct.

restore the comment?

[Brskt]

@Brskt made 12 comments.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @tamird and @vadorovsky).

---

ebpf/aya-ebpf/src/btf_maps/array.rs line 14 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

should this mention the meanings of M an dF?

Done.

---

ebpf/aya-ebpf/src/btf_maps/mod.rs line 29 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this cg_default can be expr instead of tt, right?

No, unfortunately. Rust's macro follow-set rules restrict what can follow an expr fragment to only =>, ,, or ;. Since the repetition can end with >, the compiler rejects it:

error: `$cg_default:expr` is followed by `>`, which is not allowed for `expr` fragments
    = note: allowed there are: `=>`, `,` or `;`

---

ebpf/aya-ebpf/src/btf_maps/mod.rs line 38 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

for my edification: is repr(c) needed?

Yes, repr(C) is required. I tested removing it and the libbpf test fails:

libbpf failed to open Rust eBPF object with btf_maps: Error: failed to open object from memory
Caused by: Invalid argument (os error 22)

Without repr(C), Rust may reorder struct fields, causing the BTF metadata layout to not match what libbpf expects.

---

ebpf/aya-ebpf/src/btf_maps/mod.rs line 38 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

restore the comment?

Done.

---

ebpf/aya-ebpf/src/btf_maps/mod.rs line 39 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

should this be expect with a reason, just like we had before?

Actually, neither allow nor expect is needed here - the dead_code lint isn't triggered for these struct fields. I've removed the attribute entirely and it compiles without warnings.

---

ebpf/aya-ebpf/src/btf_maps/mod.rs line 43 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

consider un-teaching the macro about the type parameters; instead require the invoker to provide this. that will generalize better when we want to support maps which will have K, V type parameters.

Done.

---

ebpf/aya-ebpf/src/btf_maps/mod.rs line 46 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

please restore the blank lines surrounding this

Done.

---

ebpf/aya-ebpf/src/btf_maps/mod.rs line 66 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

can this move back to where it was?

Done.

---

test/integration-test/Cargo.toml line 17 at r7 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

it'd be great to remove this feature, provided we can reliably install what's needed. i suspect cross compilation on the mac VM will not be so simple

Agreed, removing the feature would be cleaner. On Linux CI, adding libelf-dev should be straightforward. However, as you noted, cross-compilation on Mac VM is likely problematic - libbpf-sys would need libelf cross-compiled for the target architecture.

For now the feature flag allows the test to run when the environment supports it (e.g., local dev with libelf-dev installed) while keeping CI green. If there's an easy way to install libelf-dev on the Linux CI runners, we could enable the feature there and leave it disabled only for Mac cross-compilation.

---

test/integration-test/Cargo.toml line 21 at r7 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

please put this in the workspace Cargo.toml; we like to keep all the versions declared in just one place

Done.

---

test/integration-test/src/tests/btf_maps.rs line 6 at r7 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

does it require libelf-dev just at build time or at run time as well?

Both. libelf-dev provides headers for build time, and the shared library (libelf.so) is needed at runtime since libbpf-sys dynamically links libelf by default.

---

test/integration-test/src/tests/btf_maps.rs line 22 at r7 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

how about collecting the names and asserting it includes BTF_ARRAY; and if it doesn't, emit the list of names.

Done.

[tamird]

@tamird reviewed 13 files and all commit messages, made 10 comments, and resolved 7 discussions.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @Brskt and @vadorovsky).

---

ebpf/aya-ebpf/src/btf_maps/array.rs line 14 at r6 (raw file):

Previously, Brskt wrote…

Done.

M and F aren't type parameters, and we shouldn't repeat the default; it's already plain in the code.

another option is to rename the const generics: M => MaxEntries, F => Flags

---

ebpf/aya-ebpf/src/btf_maps/mod.rs line 29 at r6 (raw file):

Previously, Brskt wrote…

No, unfortunately. Rust's macro follow-set rules restrict what can follow an expr fragment to only =>, ,, or ;. Since the repetition can end with >, the compiler rejects it:

error: `$cg_default:expr` is followed by `>`, which is not allowed for `expr` fragments
    = note: allowed there are: `=>`, `,` or `;`

Thanks for checking.

---

ebpf/aya-ebpf/src/btf_maps/mod.rs line 38 at r6 (raw file):

Previously, Brskt wrote…

Yes, repr(C) is required. I tested removing it and the libbpf test fails:

libbpf failed to open Rust eBPF object with btf_maps: Error: failed to open object from memory
Caused by: Invalid argument (os error 22)

Without repr(C), Rust may reorder struct fields, causing the BTF metadata layout to not match what libbpf expects.

Thanks for testing. Can you please document the finding?

---

ebpf/aya-ebpf/src/btf_maps/mod.rs line 43 at r6 (raw file):

Previously, Brskt wrote…

Done.

Can you make $tp a one-or-more repetition?

---

test/integration-test/Cargo.toml line 17 at r7 (raw file):

Previously, Brskt wrote…

Agreed, removing the feature would be cleaner. On Linux CI, adding libelf-dev should be straightforward. However, as you noted, cross-compilation on Mac VM is likely problematic - libbpf-sys would need libelf cross-compiled for the target architecture.

For now the feature flag allows the test to run when the environment supports it (e.g., local dev with libelf-dev installed) while keeping CI green. If there's an easy way to install libelf-dev on the Linux CI runners, we could enable the feature there and leave it disabled only for Mac cross-compilation.

Yes, there is an easy way (at compile time). I trust you can figure it out. Please ensure this is running in (at least some) environments in CI.

---

test/integration-test/src/tests/btf_maps.rs line 6 at r7 (raw file):

Previously, Brskt wrote…

Both. libelf-dev provides headers for build time, and the shared library (libelf.so) is needed at runtime since libbpf-sys dynamically links libelf by default.

Can we make it link statically? The tests we run in the VM (even when the host is Linux) have next to no userland. If not, running this only in the non-VM case is fine too.

---

ebpf/aya-ebpf/src/btf_maps/ring_buf.rs line 19 at r8 (raw file):

    max_entries: M,
    map_flags: F,
    key: *const (),

hmm, the caller should probably not have to supply the *const bit, that's just a BTF trick

---

test/integration-test/src/tests/btf_maps.rs line 22 at r9 (raw file):

    // Verify libbpf can see the BTF map
    let maps: Vec<_> = obj.maps().collect();

no need to materialize this vector just go straight to names

---

test/integration-test/src/tests/btf_maps.rs line 26 at r9 (raw file):

        !maps.is_empty(),
        "libbpf should find BTF maps in the object"
    );

pointless assertion given the below?

Code quote:

    assert!(
        !maps.is_empty(),
        "libbpf should find BTF maps in the object"
    );

---

test/integration-test/src/tests/btf_maps.rs line 29 at r9 (raw file):

    // Check that our btf_map Array is found
    let map_names: Vec<_> = maps.iter().map(|m| m.name().to_string_lossy()).collect();

can we avoid the lossy here? if these are OsStr then I believe they support comparison to string.

[Brskt]

@Brskt made 9 comments.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @tamird and @vadorovsky).

---

ebpf/aya-ebpf/src/btf_maps/array.rs line 14 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

M and F aren't type parameters, and we shouldn't repeat the default; it's already plain in the code.

another option is to rename the const generics: M => MaxEntries, F => Flags

Done. Renamed the const generics to MAX_ENTRIES and FLAGS for self-documentation - removed the doc section since the names are now clear.

---

ebpf/aya-ebpf/src/btf_maps/mod.rs line 38 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Thanks for testing. Can you please document the finding?

Done.

---

ebpf/aya-ebpf/src/btf_maps/mod.rs line 43 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Can you make $tp a one-or-more repetition?

Done. Used ; as a separator between type parameters and const generics to resolve the macro parsing ambiguity:

// Single type param (no const generics)
pub struct SkStorage<T>,                                                                                                                                          

// Single type param with const generics
pub struct Array<T; const MAX_ENTRIES: usize, const FLAGS: usize = 0>,

// Multiple type params (for future HashMap<K, V>)
pub struct HashMap<K, V; const MAX_ENTRIES: usize>,

This syntax differs from standard Rust but is necessary because macro_rules! can't disambiguate between $($tp:ident),+ and $(, const ...) after a comma - both patterns could match.

---

ebpf/aya-ebpf/src/btf_maps/ring_buf.rs line 19 at r8 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

hmm, the caller should probably not have to supply the *const bit, that's just a BTF trick

Done. The macro now wraps key/value types in *const internally - callers just specify the bare types.

---

test/integration-test/Cargo.toml line 17 at r7 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Yes, there is an easy way (at compile time). I trust you can figure it out. Please ensure this is running in (at least some) environments in CI.

Done. Enabled the vendored feature on libbpf-rs which makes libbpf-sys statically link libelf at compile time. No runtime dependencies needed, so the test will work in VM environments too.

---

test/integration-test/src/tests/btf_maps.rs line 6 at r7 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Can we make it link statically? The tests we run in the VM (even when the host is Linux) have next to no userland. If not, running this only in the non-VM case is fine too.

Done.

---

test/integration-test/src/tests/btf_maps.rs line 22 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

no need to materialize this vector just go straight to names

Done.

---

test/integration-test/src/tests/btf_maps.rs line 26 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

pointless assertion given the below?

Done. Removed, the BTF_ARRAY assertion covers this.

---

test/integration-test/src/tests/btf_maps.rs line 29 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

can we avoid the lossy here? if these are OsStr then I believe they support comparison to string.

Done. OsStr supports direct comparison to &str, so no allocation or conversion needed.

[Brskt]

The vendored feature requires gettext (for autopoint) to build libbpf from source. What do u think?

[tamird]

I dunno, try it - add it where it needs to be added and we will see

@tamird reviewed 11 files and all commit messages, made 7 comments, and resolved 7 discussions.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @Brskt and @vadorovsky).

---

ebpf/aya-ebpf/src/btf_maps/mod.rs line 43 at r6 (raw file):

Previously, Brskt wrote…

Done. Used ; as a separator between type parameters and const generics to resolve the macro parsing ambiguity:

// Single type param (no const generics)
pub struct SkStorage<T>,                                                                                                                                          

// Single type param with const generics
pub struct Array<T; const MAX_ENTRIES: usize, const FLAGS: usize = 0>,

// Multiple type params (for future HashMap<K, V>)
pub struct HashMap<K, V; const MAX_ENTRIES: usize>,

This syntax differs from standard Rust but is necessary because macro_rules! can't disambiguate between $($tp:ident),+ and $(, const ...) after a comma - both patterns could match.

interesting. do we need to know if we're looking at a type or a const generic? in other words, could we parse something like

$($(const)? $generic:ident $(: usize)? $(= $generic_default:tt)?),*

we just have two types of expansions: with defaults, and without defaults -- we don't care which one is a const generic and which one is a type parameter.

---

test/integration-test/src/tests/btf_maps.rs line 29 at r9 (raw file):

Previously, Brskt wrote…

Done. OsStr supports direct comparison to &str, so no allocation or conversion needed.

great, but we do need to print the names in the not-found case.

---

ebpf/aya-ebpf/src/btf_maps/mod.rs line 36 at r10 (raw file):

        max_entries: $max_entries:expr,
        map_flags: $map_flags:expr,
        key: $key_ty:ty,

consider making these "keywords" key_type and value_type for clarity

---

ebpf/aya-ebpf/src/btf_maps/mod.rs line 53 at r10 (raw file):

            map_flags: *const [i32; $map_flags],

            $($extra_field: $extra_ty,)*

newline after this please as before

---

test/integration-test/Cargo.toml line 20 at r11 (raw file):

[dev-dependencies]
libbpf-rs = { workspace = true }

the features you need should be here, not in the workspace cargo.toml

---

Cargo.toml line 85 at r11 (raw file):

indoc = { version = "2.0", default-features = false }
libc = { version = "0.2.105", default-features = false }
libbpf-rs = { version = "0.25", default-features = false, features = ["vendored"] }

features = ["vendored"] should be in the user crate, not here

[Brskt]

@Brskt made 6 comments.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @tamird and @vadorovsky).

---

Cargo.toml line 85 at r11 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

features = ["vendored"] should be in the user crate, not here

Done.

---

ebpf/aya-ebpf/src/btf_maps/mod.rs line 43 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

interesting. do we need to know if we're looking at a type or a const generic? in other words, could we parse something like

$($(const)? $generic:ident $(: usize)? $(= $generic_default:tt)?),*

we just have two types of expansions: with defaults, and without defaults -- we don't care which one is a const generic and which one is a type parameter.

The challenge is that $(const)? in the pattern just matches but doesn't capture - so in the expansion we can't conditionally output const and : usize based on what was matched.

We'd need something like $( $const_kw:tt $generic:ident $colon_ty:tt )? but then parsing gets fragile.

Happy to explore further if u think there's a cleaner way, but the semicolon separator felt like the most robust solution for now.

---

ebpf/aya-ebpf/src/btf_maps/mod.rs line 36 at r10 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

consider making these "keywords" key_type and value_type for clarity

Done.

---

ebpf/aya-ebpf/src/btf_maps/mod.rs line 53 at r10 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

newline after this please as before

Done.

---

test/integration-test/Cargo.toml line 20 at r11 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

the features you need should be here, not in the workspace cargo.toml

Done.

---

test/integration-test/src/tests/btf_maps.rs line 29 at r9 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

great, but we do need to print the names in the not-found case.

Done. Collecting to Vec<OsString> to print names on failure while keeping direct comparison.

[tamird]

@tamird reviewed 12 files and all commit messages, made 3 comments, and resolved 5 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Brskt and @vadorovsky).

---

ebpf/aya-ebpf/src/btf_maps/mod.rs line 43 at r6 (raw file):

Previously, Brskt wrote…

The challenge is that $(const)? in the pattern just matches but doesn't capture - so in the expansion we can't conditionally output const and : usize based on what was matched.

We'd need something like $( $const_kw:tt $generic:ident $colon_ty:tt )? but then parsing gets fragile.

Happy to explore further if u think there's a cleaner way, but the semicolon separator felt like the most robust solution for now.

Yeah ok this makes sense. I pushed a commit just to make this a bit easier to look at. Please squash it into your first commit if you're happy with it.

---

test/integration-test/src/tests/btf_maps.rs line 18 at r14 (raw file):

    // Verify libbpf can see the BTF_ARRAY map
    let map_names: Vec<_> = obj.maps().map(|m| m.name().to_os_string()).collect();

i pushed a commit to avoid the owned conversion and also to use display instead of debug in the failure message, please squash those last two commits into your test commit if you are happy with them

[tamird]

I'm trying to get CI working, but we have a blocker: this PR makes it impossible to run clippy on macOS because of the libbpf dependency. So I think we do need to go back to the optional feature and built it only on Linux.

[tamird]

Looks like we also have compilation problems on arm in general.

[Brskt]

@Brskt made 5 comments.
Reviewable status: 16 of 21 files reviewed, 7 unresolved discussions (waiting on @tamird and @vadorovsky).

---

Brewfile line 17 at r30 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

let's just give this one comment that explains these are for libbpf-sys

Done.

---

.github/workflows/ci.yml line 122 at r30 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

i don't understand this comment. do you?

The issue is that Ubuntu 24.04 GCC on 32-bit ARM defines _TIME_BITS=64 by default. However, glibc requires _FILE_OFFSET_BITS=64 to be set before _TIME_BITS=64 can be used (it's a dependency). Since GCC sets _TIME_BITS=64 but not _FILE_OFFSET_BITS=64, we get a compilation error.

The fix unsets _TIME_BITS (removing GCC's default) and sets _FILE_OFFSET_BITS=64 to satisfy glibc.

I've updated the comment to make this clearer.

---

.github/workflows/ci.yml line 396 at r30 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

we need to share this with clippy.sh so that it can continue to work on macOS.

Done.

---

.github/workflows/ci.yml line 414 at r30 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this issue also covers the extra_cflags=(-idirafter "/usr/include/${arch}-linux-gnu" -idirafter /usr/include) below which also explains why we install linux-libc-dev -- so we need to put this comment somewhere that covers all these things

Done.

---

.github/workflows/ci.yml line 418 at r30 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

are you sure these two are necessary?

Yes, these are needed for macOS cross-compilation. Without them, elfutils configure fails:

• gzdirect: "zlib not found but is required"
• fts_close: "failed to find fts_close"

The configure script can't find these functions when cross-compiling from Darwin to Linux musl.

[tamird]

@tamird reviewed 5 files and all commit messages, made 6 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @Brskt and @vadorovsky).

---

.github/workflows/ci.yml line 122 at r30 (raw file):

Previously, Brskt wrote…

The issue is that Ubuntu 24.04 GCC on 32-bit ARM defines _TIME_BITS=64 by default. However, glibc requires _FILE_OFFSET_BITS=64 to be set before _TIME_BITS=64 can be used (it's a dependency). Since GCC sets _TIME_BITS=64 but not _FILE_OFFSET_BITS=64, we get a compilation error.

The fix unsets _TIME_BITS (removing GCC's default) and sets _FILE_OFFSET_BITS=64 to satisfy glibc.

I've updated the comment to make this clearer.

Why is it necessary to unset _TIME_BITS? Wouldn't just setting _FILE_OFFSET_BITS be sufficient?

---

.github/workflows/ci.yml line 396 at r30 (raw file):

Previously, Brskt wrote…

Done.

It's duplicated. Can it be shared?

---

.github/workflows/ci.yml line 418 at r30 (raw file):

Previously, Brskt wrote…

Yes, these are needed for macOS cross-compilation. Without them, elfutils configure fails:

• gzdirect: "zlib not found but is required"
• fts_close: "failed to find fts_close"

The configure script can't find these functions when cross-compiling from Darwin to Linux musl.

the zlib thing is weird, we install it (see Brewfile)

---

clippy.sh line 5 at r38 (raw file):

set -eux

# macOS cross-compilation fixes for libbpf-sys vendored dependencies:

this is duplicating what we do in CI -- can we share it, instead? also not clear to me why we need to write to /tmp; could we check in these trampolines?

---

.github/workflows/ci.yml line 285 at r38 (raw file):

          sudo apt update
          # linux-libc-dev provides kernel headers for libbpf-sys cross-compilation.
          # See https://github.com/libbpf/libbpf-sys/issues/137

Please add a period and an empty comment line between these paragraphs

---

.github/workflows/ci.yml line 414 at r38 (raw file):

      # libbpf-sys cross-compilation workarounds:
      # - linux-libc-dev provides kernel headers (installed above)

🤔 aren't these userspace headers?

[Brskt]

@Brskt made 6 comments.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @tamird and @vadorovsky).

---

clippy.sh line 5 at r38 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this is duplicating what we do in CI -- can we share it, instead? also not clear to me why we need to write to /tmp; could we check in these trampolines?

Done. Checked in the make wrapper to ci/bin/make and both clippy.sh and CI now share it.

---

.github/workflows/ci.yml line 122 at r30 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Why is it necessary to unset _TIME_BITS? Wouldn't just setting _FILE_OFFSET_BITS be sufficient?

Yup. Looking at glibc's features-time64.h, the check only verifies that both are defined, not the order. Setting just _FILE_OFFSET_BITS=64 should be sufficient. I'll simplify the fix.

---

.github/workflows/ci.yml line 396 at r30 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

It's duplicated. Can it be shared?

Yes, both now use ci/bin/make.

---

.github/workflows/ci.yml line 418 at r30 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

the zlib thing is weird, we install it (see Brewfile)

The Homebrew zlib is built for macOS (Mach-O), but we're cross-compiling to Linux musl (ELF). The configure script can't link macOS libraries into Linux binaries, so we skip the check - zlib will be available at runtime on the Linux target.

---

.github/workflows/ci.yml line 285 at r38 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Please add a period and an empty comment line between these paragraphs

Done.

---

.github/workflows/ci.yml line 414 at r38 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

🤔 aren't these userspace headers?

Done, changed to "Linux API headers".

[tamird]

@tamird reviewed 3 files and all commit messages, made 10 comments, and resolved 3 discussions.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @Brskt and @vadorovsky).

---

.github/workflows/ci.yml line 122 at r30 (raw file):

Previously, Brskt wrote…

Yup. Looking at glibc's features-time64.h, the check only verifies that both are defined, not the order. Setting just _FILE_OFFSET_BITS=64 should be sufficient. I'll simplify the fix.

Ack. Looks like only the comment is updated.

---

.github/workflows/ci.yml line 418 at r30 (raw file):

Previously, Brskt wrote…

The Homebrew zlib is built for macOS (Mach-O), but we're cross-compiling to Linux musl (ELF). The configure script can't link macOS libraries into Linux binaries, so we skip the check - zlib will be available at runtime on the Linux target.

ack thanks

---

.github/workflows/ci.yml line 414 at r38 (raw file):

Previously, Brskt wrote…

Done, changed to "Linux API headers".

is that what they are? aren't they exactly the "system headers" mentioned on the next line?

---

ci/bin/make line 1 at r40 (raw file):

#!/bin/bash

#!/usr/bin/env sh

set -eux

(for consistency)

---

.github/workflows/ci.yml line 286 at r40 (raw file):

          # linux-libc-dev provides Linux API headers for libbpf-sys cross-compilation.
          #
          # See https://github.com/libbpf/libbpf-sys/issues/137

period

---

.github/workflows/ci.yml line 398 at r40 (raw file):

      # macOS cross-compilation fixes for libbpf-sys vendored dependencies.
      # See https://github.com/libbpf/libbpf-sys/issues/137

ditto (blank line) here and trailing period

---

.github/workflows/ci.yml line 404 at r40 (raw file):

          echo "${{ github.workspace }}/ci/bin" >> $GITHUB_PATH
          echo "AR=x86_64-linux-musl-ar" >> $GITHUB_ENV
          echo "RANLIB=x86_64-linux-musl-ranlib" >> $GITHUB_ENV

this is duplicated thrice, right? here, in clippy.sh and in the make wrapper

Code quote:

          echo "AR=x86_64-linux-musl-ar" >> $GITHUB_ENV
          echo "RANLIB=x86_64-linux-musl-ranlib" >> $GITHUB_ENV

---

test/integration-test/src/tests/btf_maps.rs line 21 at r40 (raw file):

    // Materialize the maps because `OpenMap::name` returns the wrong lifetime.
    //
    // TODO(https://github.com/libbpf/libbpf-rs/pull/1308): Remove this once the PR is merged.

Would you mind updating this link to say

// TODO(https://github.com/libbpf/libbpf-rs/commit/9622223): remove the `collect` when this commit is released.

feel free to squash all my messy commits as well

---

clippy.sh line 5 at r40 (raw file):

set -eux

# macOS cross-compilation fixes for libbpf-sys vendored dependencies.

blank comment line between paragraph and citation please

---

clippy.sh line 15 at r40 (raw file):

  export ac_cv_search__obstack_free='none required'
  export ac_cv_search_gzdirect='none required'
  export ac_cv_search_fts_close='none required'

but all this is still duplicated?

Code quote:

  export AR=x86_64-linux-musl-ar
  export RANLIB=x86_64-linux-musl-ranlib
  export ac_cv_search_argp_parse='none required'
  export ac_cv_search__obstack_free='none required'
  export ac_cv_search_gzdirect='none required'
  export ac_cv_search_fts_close='none required'

[Brskt]

@Brskt made 9 comments.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @tamird and @vadorovsky).

---

clippy.sh line 5 at r40 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

blank comment line between paragraph and citation please

Done.

---

clippy.sh line 15 at r40 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

but all this is still duplicated?

Done, extracted to ci/macos-cross.env that both clippy.sh and CI now source.

---

.github/workflows/ci.yml line 122 at r30 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Ack. Looks like only the comment is updated.

I tried removing -U_TIME_BITS but the build failed. Ubuntu 24.04 GCC defines _TIME_BITS=64 as a compiler built-in, and libbpf-sys's vendored zlib sees it before our -D flag. The -U is needed to unset the built-in.

---

.github/workflows/ci.yml line 414 at r38 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

is that what they are? aren't they exactly the "system headers" mentioned on the next line?

linux-libc-dev provides kernel UAPI headers (kernel-userspace interface), while the -idirafter paths are for cross-compile header resolution. Updated to be more precise.

---

.github/workflows/ci.yml line 286 at r40 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

period

Done.

---

.github/workflows/ci.yml line 398 at r40 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

ditto (blank line) here and trailing period

Done.

---

.github/workflows/ci.yml line 404 at r40 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this is duplicated thrice, right? here, in clippy.sh and in the make wrapper

Done. Extracted to ci/macos-cross.env that both clippy.sh and CI source. The make wrapper still passes AR/RANLIB directly to make (needed for zlib's configure override).

---

ci/bin/make line 1 at r40 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

#!/usr/bin/env sh

set -eux

(for consistency)

Done.

---

test/integration-test/src/tests/btf_maps.rs line 21 at r40 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Would you mind updating this link to say

// TODO(https://github.com/libbpf/libbpf-rs/commit/9622223): remove the `collect` when this commit is released.

feel free to squash all my messy commits as well

Done.

[tamird]

Nice work. Almost there!

@tamird reviewed 15 files and all commit messages, made 4 comments, and resolved 10 discussions.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Brskt and @vadorovsky).

---

clippy.sh line 10 at r44 (raw file):

if [ "$(uname -s)" = "Darwin" ]; then
  script_dir=$(cd "$(dirname "$0")" && pwd)
  export PATH="$script_dir/ci/bin:$PATH"

should this be in libbpf-sys.env since it is also just env?

EDIT: ah i guess they're separate because GITHUB_ENV and GITHUB_PATH in CI. Maybe a comment here would elucidate.

---

.github/workflows/ci.yml line 404 at r44 (raw file):

        run: |
          echo "${{ github.workspace }}/ci/bin" >> $GITHUB_PATH
          grep -v '^#' ci/libbpf-sys.env | grep -v '^$' | sed 's/"//g' >> $GITHUB_ENV

neat. consider a small comment explaining this stanza

---

.github/workflows/ci.yml line 418 at r44 (raw file):

          # Source cross-compilation env vars for libbpf-sys vendored dependencies.
          # AR/RANLIB are macOS-specific (musl-cross toolchain), filter them on Linux.

should we split it into two scripts rather than do this filtering after the fact, which is also encoding knowledge at quite some distance?

[Brskt]

@Brskt made 3 comments.
Reviewable status: 15 of 24 files reviewed, 5 unresolved discussions (waiting on @tamird and @vadorovsky).

---

clippy.sh line 10 at r44 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

should this be in libbpf-sys.env since it is also just env?

EDIT: ah i guess they're separate because GITHUB_ENV and GITHUB_PATH in CI. Maybe a comment here would elucidate.

Comment added.

---

.github/workflows/ci.yml line 404 at r44 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

neat. consider a small comment explaining this stanza

Done.

---

.github/workflows/ci.yml line 418 at r44 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

should we split it into two scripts rather than do this filtering after the fact, which is also encoding knowledge at quite some distance?

Done. Split into two env files:

• ci/libbpf-sys.env: shared autoconf cache variables (ac_cv_search_*)
• ci/macos-toolchain.env: macOS-specific AR/RANLIB for musl-cross

Now macOS sources both, Linux sources only libbpf-sys.env. No more filtering.

[tamird]

Should we squash these commits differently? first and last together (all non-test changes) and middle two together (all test changes)

@tamird reviewed 9 files and all commit messages, made 3 comments, and resolved 3 discussions.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Brskt and @vadorovsky).

---

clippy.sh line 9 at r47 (raw file):

# PATH and CFLAGS are set here (not in libbpf-sys.env) because they need
# absolute paths. In CI, they also use separate mechanisms (GITHUB_PATH vs
# GITHUB_ENV).

hm. why can't CFLAGS be set in libbpf-sys.env? I don't understand this absolute path commentary. I guess complicating the script to figure out its path would mess with the grep magic in ci.yml, but then that's what the comment should say, rather than this

Code quote:

# absolute paths. In CI, they also use separate mechanisms (GITHUB_PATH vs
# GITHUB_ENV).

---

clippy.sh line 37 at r47 (raw file):

else
  target_args=""
fi

please revert this bit - this script can take --target as an argument (and is usually not running on x86_64)

Code quote:

#
# On macOS, target Linux since aya uses Linux-specific libc constants.
if [ "$(uname -s)" = "Darwin" ]; then
  target_args="--target x86_64-unknown-linux-musl"
else
  target_args=""
fi

[Brskt]

@Brskt made 2 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @tamird and @vadorovsky).

---

clippy.sh line 9 at r47 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

hm. why can't CFLAGS be set in libbpf-sys.env? I don't understand this absolute path commentary. I guess complicating the script to figure out its path would mess with the grep magic in ci.yml, but then that's what the comment should say, rather than this

Updated the comment. The env file only contains static values, while PATH and CFLAGS need dynamic absolute paths computed at runtime ($script_dir here, ${{ github.workspace }} in CI). We could use relative paths in the env file, but they're fragile since they depend on the current working directory.

---

clippy.sh line 37 at r47 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

please revert this bit - this script can take --target as an argument (and is usually not running on x86_64)

Done, reverted.

[tamird]

Should we squash these commits differently? first and last together (all non-test changes) and middle two together (all test changes)

How about it?

@tamird reviewed 9 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Brskt and @vadorovsky).

[Brskt]

Oops, I forgot to do it. Should be good now.

@Brskt made 1 comment.
Reviewable status: 10 of 24 files reviewed, 2 unresolved discussions (waiting on @tamird and @vadorovsky).

[tamird]

@tamird reviewed 14 files and all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @vadorovsky).

[tamird]

LGTM. @vadorovsky to take a final pass and merge if LGTY

[vadorovsky]

[tamird]

@Brskt thanks for this PR! We also run aya's integration tests in bpf-linker, and those tests are now failing due to the missing libbpf-rs dependencies. Would you mind rehabilitating that infrastructure? https://github.com/aya-rs/bpf-linker/actions/runs/21425176833 -- thanks!

[Brskt]

@Brskt thanks for this PR! We also run aya's integration tests in bpf-linker, and those tests are now failing due to the missing libbpf-rs dependencies. Would you mind rehabilitating that infrastructure? https://github.com/aya-rs/bpf-linker/actions/runs/21425176833 -- thanks!

Sure! This PR should fix the aya integration tests aya-rs/bpf-linker#340