libbpf-cargo: Relative anonymous type naming for struct fields #1178

Waujito · 2025-05-22T16:56:36Z

Previously, anonymous types were named as _anon{id} with id as a global counter. This approach caused cascading renaming of all subsequent anonymous definitions after any C anonymous struct order/amount change. This was a major design problem not for only development but even portability, since here may be the case when something was changed in vmlinux.h leading to compilation errors.

This PR addresses the issue by localizing cascading effects to the local struct layer rather than the global layer. While the solution is not fully ideal - cascading still occurs within individual structs - it eliminates the need for wide Rust code modifications when the order of anonymous struct definitions changes.

The PR is breaking and should be included in major version update.

The PR fixes #1161

While this approach is ok, I still think the better way will be to use getters, since this is breaking and less intuitive.

danielocfb · 2025-05-22T17:01:56Z

Haven't looked at it yet, but this is great. I was considering doing that for 0.25.0, but didn't have the cycles. Will postpone the release until I've gotten a chance to take a look here. Please look into the CI failures, which all seem legit.

Previously, anonymous types were named as __anon_{id} with id as a global counter. This approach caused cascading renaming of all subsequent anonymous definitions after any C anonymous struct order/amount change. This was a major design problem not for only development but even portability, since here may be the case when something was changed in vmlinux.h leading to compilation errors. This commit addresses the issue by localizing cascading effects to the local struct layer rather than the global layer. While the solution is not fully ideal - cascading still occurs within individual structs - it eliminates the need for wide Rust code modifications when the order of anonymous struct definitions changes. Note, that the commit does not fully eliminates __anon_{id}, but the most important part of it. The commit is breaking and should be included in major version update.

Waujito · 2025-05-22T18:17:32Z

Here are still some plain __anon types like in maps. I'll try to catch more and may be add some defenders of name collisions.

danielocfb

In general this seems okay to me, but the code seems under documented and under-tested in the sense that I can remove/change various lines and all tests keep passing. Similarly, I am not sure I understand some of the assignments done. Left some comments.

danielocfb · 2025-05-22T20:51:45Z

libbpf-cargo/src/gen/btf.rs

@@ -367,6 +368,13 @@ fn escape_reserved_keyword(identifier: Cow<'_, str>) -> Cow<'_, str> {
    }
 }

+#[derive(Debug, Clone)]
+pub struct BtfDependency {


Doesn't have to be pub, please restrict visibility. Same for fields.

danielocfb · 2025-05-22T20:52:15Z

libbpf-cargo/src/gen/btf.rs

+
+    dependencies: RefCell<HashMap<TypeId, BtfDependency>>,


Can you please add a doc comment describing the member?

danielocfb · 2025-05-22T20:53:09Z

libbpf-cargo/src/gen/btf.rs

 }

 impl TypeMap {
+    pub fn derive_parent<'s>(&self, ty: &BtfType<'s>, parent: &BtfType<'s>) {


Suggested change

pub fn derive_parent<'s>(&self, ty: &BtfType<'s>, parent: &BtfType<'s>) {

fn derive_parent<'s>(&self, ty: &BtfType<'s>, parent: &BtfType<'s>) {

Is the name really fitting? Aren't you more registering a parent relationship rather than "deriving" anything?

assign_parent?

danielocfb · 2025-05-22T20:54:39Z

libbpf-cargo/src/gen/btf.rs

+                dep.child_counter = Rc::new(RefCell::new(0));
+            }
+
+            let parent_counter = Rc::<RefCell<i32>>::clone(&pdep.child_counter);


Suggested change

let parent_counter = Rc::<RefCell<i32>>::clone(&pdep.child_counter);

let parent_counter = Rc::clone(&pdep.child_counter);

danielocfb · 2025-05-22T21:21:59Z

libbpf-cargo/src/gen/btf.rs

+            if ty.name().is_some() {
+                dep.child_counter = Rc::new(RefCell::new(0));
+            }


Why is this code needed?

Added a comment

danielocfb · 2025-05-22T21:40:25Z

libbpf-cargo/src/gen/btf.rs

+            if ty.name().is_some() {
+                dep.child_counter = Rc::new(RefCell::new(0));
+            }
+            dep.dep_id = 1;


Why is this value set here and not already during initialization?

Added a comment

danielocfb · 2025-05-22T21:40:38Z

libbpf-cargo/src/gen/btf.rs

+            if let Some(n) = parent.name() {
+                dep.name = Some(n.to_string_lossy().to_string());
+            }


Why is this value set here and not during initialization?

danielocfb · 2025-05-22T21:57:49Z

libbpf-cargo/src/gen/btf.rs

+    pub name: Option<String>,
+    pub dep_id: i32,
+    pub child_counter: Rc<RefCell<i32>>,


Please document what these represent.

danielocfb · 2025-05-22T22:04:06Z

Here are still some plain __anon types like in maps. I'll try to catch more and may be add some defenders of name collisions.

What do you mean exactly? Where is "here"?

Waujito · 2025-05-23T09:22:38Z

In 0e504e6 I simplified the code slightly, modified the global anon counting behavior, added documentation for new structure fields and included a test for depth-aligned anonymous structs.

The global anonymous struct counting now excludes relative anons to make its behavior more representative and easier to use. I also added some fallback protection on it for (im)possible case when global anonymous struct is assigned before a relative one: in such case we will use old type of naming.

Here are still some plain __anon types like in maps. I'll try to catch more and may be add some defenders of name collisions.

What do you mean exactly? Where is "here"?

Nvm. Those anonymous structs are at the global level and should be kept with old naming.

I think the code is ok now, but I'm aware of possible name collisions for relative types.
I can't mind the case, but what if. Maybe I should implement dep id counting in a separate hashmap for parent names as strings, but not with that crazy child_counter? This solution will not be elegant, but more trustworthy...

Waujito force-pushed the rel_anon branch from e956876 to 75cb804 Compare May 22, 2025 17:04

Waujito force-pushed the rel_anon branch from 75cb804 to 5695464 Compare May 22, 2025 17:20

libbpf-cargo: Fix broken tests for 3a9e1d5

4f4c5f9

Waujito force-pushed the rel_anon branch from 5695464 to 4f4c5f9 Compare May 22, 2025 17:54

danielocfb reviewed May 22, 2025

View reviewed changes

danielocfb mentioned this pull request May 22, 2025

Release 0.25.0 #1126

Open

libbpf-cargo: Relative anonymous type naming

0e604e6

	pub fn derive_parent<'s>(&self, ty: &BtfType<'s>, parent: &BtfType<'s>) {
	fn derive_parent<'s>(&self, ty: &BtfType<'s>, parent: &BtfType<'s>) {

	let parent_counter = Rc::<RefCell<i32>>::clone(&pdep.child_counter);
	let parent_counter = Rc::clone(&pdep.child_counter);

libbpf-cargo: Relative anonymous type naming for struct fields #1178

Are you sure you want to change the base?

libbpf-cargo: Relative anonymous type naming for struct fields #1178

Uh oh!

Conversation

Waujito commented May 22, 2025

Uh oh!

danielocfb commented May 22, 2025

Uh oh!

Waujito commented May 22, 2025

Uh oh!

danielocfb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielocfb commented May 22, 2025

Uh oh!

Waujito commented May 23, 2025

Uh oh!

Uh oh!