Repository: libbpf/libbpf-rs (language: rust)

Issue:
libbpf-cargo: enums with duplicated values cause compilation issues 
In https://github.com/libbpf/libbpf-rs/commit/4e62c38322cced90ad673d25e8253f47ad37c566, Rust type generation changed from the used types to all types. This is mostly harmless, except in the case were various enum entries share the same value. This is perfectly valid in C and[ this happens in the `bpf_map_type` enum](https://github.com/torvalds/linux/blob/98f7e32f20d28ec452afb208f9cffc08448a2652/include/uapi/linux/bpf.h#L984). The error currently get:

```
error[E0081]: discriminant value `19` assigned more than once
   --> lightswitch-capabilities/src/bpf/features_skel.rs:439:9
    |
439 |         pub enum bpf_map_type {
    |         ^^^^^^^^^^^^^^^^^^^^^
...
460 |             BPF_MAP_TYPE_CGROUP_STORAGE_DEPRECATED = 19,
    |                                                      -- `19` assigned here
461 |             BPF_MAP_TYPE_CGROUP_STORAGE = 19,
    |                                           -- `19` assigned here
```

I've verified that the parent commit https://github.com/libbpf/libbpf-rs/commit/65094bc5d40186cc83d7017ad1b77feed47f1927 does not show this issue. Perhaps rather than [emitting Rust enums](https://github.com/libbpf/libbpf-rs/blob/a0577750e54710b8949c65bff1321ede5f7ea2ed/libbpf-cargo/src/gen/btf.rs#L815) we could declare them as constants? Opening this issues mostly so I don't forget. I'll be happy to submit a PR if this approach sounds reasonable. 

cc @d-e-s-o


Discussion / hints:
> I've verified that the parent commit https://github.com/libbpf/libbpf-rs/commit/65094bc5d40186cc83d7017ad1b77feed47f1927 does not show this issue.

But that's likely only because we don't generate a binding for the enum to begin with, right?

> Perhaps rather than [emitting Rust enums](https://github.com/libbpf/libbpf-rs/blob/a0577750e54710b8949c65bff1321ede5f7ea2ed/libbpf-cargo/src/gen/btf.rs#L815) we could declare them as constants?

It's a possibility, yeah, but I think there are ergonomic benefits to using a Rust enum. The other possibility is to throw away one variant. It's not great either, but then given that we've only seen this now suggests to me that this is a pretty rare thing to begin with -- and I don't know if penalizing everybody by switching to constants is worth it.
> But that's likely only because we don't generate a binding for the enum to begin with, right?

Indeed, just mentioned this to clarify that this is narrowed down to the other commit.

> It's a possibility, yeah, but I think there are ergonomic benefits to using a Rust enum. The other possibility is to throw away one variant. It's not great either, but then given that we've only seen this now suggests to me that this is a pretty rare thing to begin with -- and I don't know if penalizing everybody by switching to constants is worth it.

Rust enums are so much nicer indeed... but throwing away one variant is IMO a big surprise for users, and it's possible that they prefer to the other name for a particular value. Seems like bindgen by default generates constants.


Fix patch:
diff --git a/libbpf-cargo/CHANGELOG.md b/libbpf-cargo/CHANGELOG.md
index 54b56e71..69c53223 100644
--- a/libbpf-cargo/CHANGELOG.md
+++ b/libbpf-cargo/CHANGELOG.md
@@ -1,6 +1,7 @@
 0.24.7
 ------
 - Fixed handling of empty unions in BPF types
+- Represent C enums with custom types and const fields
 
 
 0.24.6
diff --git a/libbpf-cargo/src/gen/btf.rs b/libbpf-cargo/src/gen/btf.rs
index 4acdbf3c..d5616122 100644
--- a/libbpf-cargo/src/gen/btf.rs
+++ b/libbpf-cargo/src/gen/btf.rs
@@ -797,6 +797,8 @@ impl<'s> GenBtf<'s> {
             _ => bail!("Invalid enum size: {}", t.size()),
         };
 
+        let enum_name = self.anon_types.type_name_or_anon(&t);
+
         let mut signed = "u";
         for value in t.iter() {
             if value.value < 0 {
@@ -805,30 +807,37 @@ impl<'s> GenBtf<'s> {
             }
         }
 
-        writeln!(
-            def,
-            r#"#[derive(Debug, Copy, Clone, Default, PartialEq, Eq)]"#
-        )?;
-        writeln!(def, r#"#[repr({signed}{repr_size})]"#)?;
-        writeln!(
-            def,
-            r#"pub enum {name} {{"#,
-            name = self.anon_types.type_name_or_anon(&t),
-        )?;
+        let mut first_field = None;
+
+        writeln!(def, r#"#[derive(Debug, Copy, Clone)]"#)?;
+        writeln!(def, r#"#[repr(transparent)]"#)?;
+        writeln!(def, r#"pub struct {enum_name}({signed}{repr_size});"#)?;
+        writeln!(def, "#[allow(non_upper_case_globals)]")?;
+        writeln!(def, r#"impl {enum_name} {{"#,)?;
+
+        for value in t.iter() {
+            first_field = first_field.or(Some(value));
 
-        for (i, value) in t.iter().enumerate() {
-            if i == 0 {
-                writeln!(def, r#"    #[default]"#)?;
-            }
             writeln!(
                 def,
-                r#"    {name} = {value},"#,
+                r#"    pub const {name}: {enum_name} = {enum_name}({value});"#,
                 name = value.name.unwrap().to_string_lossy(),
                 value = value.value,
             )?;
         }
 
-        writeln!(def, "}}")?;
+        writeln!(def, r#"}}"#)?;
+
+        if let Some(first_field) = first_field {
+            writeln!(def, r#"impl Default for {enum_name} {{"#)?;
+            writeln!(
+                def,
+                r#"    fn default() -> Self {{ {enum_name}::{name} }}"#,
+                name = first_field.name.unwrap().to_string_lossy()
+            )?;
+            writeln!(def, r#"}}"#)?;
+        }
+
         Ok(())
     }
 


Test patch:
diff --git a/libbpf-cargo/src/test.rs b/libbpf-cargo/src/test.rs
index 6d44271f..b76ba6c8 100644
--- a/libbpf-cargo/src/test.rs
+++ b/libbpf-cargo/src/test.rs
@@ -1817,20 +1817,26 @@ fn test_btf_dump_definition_enum() {
 enum Foo {
     Zero = 0,
     One,
-    seven = 7,
+    Seven = 7,
+    ZeroDup = Zero,
 };
 
 enum Foo foo;
 "#;
 
     let expected_output = r#"
-#[derive(Debug, Copy, Clone, Default, PartialEq, Eq)]
-#[repr(u32)]
-pub enum Foo {
-    #[default]
-    Zero = 0,
-    One = 1,
-    seven = 7,
+#[derive(Debug, Copy, Clone)]
+#[repr(transparent)]
+pub struct Foo(u32);
+#[allow(non_upper_case_globals)]
+impl Foo {
+    pub const Zero: Foo = Foo(0);
+    pub const One: Foo = Foo(1);
+    pub const Seven: Foo = Foo(7);
+    pub const ZeroDup: Foo = Foo(0);
+}
+impl Default for Foo {
+    fn default() -> Self { Foo::Zero }
 }
 "#;
 
@@ -1841,6 +1847,26 @@ pub enum Foo {
     let enum_foo = find_type_in_btf!(btf, types::Enum<'_>, "Foo");
 
     assert_definition(&btf, &enum_foo, expected_output);
+
+    // Ensure this code is valid Rust. See https://github.com/libbpf/libbpf-rs/issues/982.
+    let mut rust_code = NamedTempFile::new().unwrap();
+    let temp_dir = TempDir::new().unwrap();
+    rust_code.write_all(expected_output.as_bytes()).unwrap();
+    rust_code.write_all(b"\n").unwrap();
+    // Add `main` so the code compiles.
+    rust_code.write_all(b"fn main() {}").unwrap();
+    let status = Command::new("rustc")
+        .arg(rust_code.path())
+        // Crate names can't contain certain symbols that might be used
+        // by `tempfile` so override its name.
+        .arg("--crate-name")
+        .arg("btf_to_rust_test")
+        // We don't care about the produced object file.
+        .arg("--out-dir")
+        .arg(temp_dir.into_path())
+        .status()
+        .expect("failed to compile rust code");
+    assert!(status.success());
 }
 
 #[test]
@@ -2565,13 +2591,16 @@ impl Default for Foo {
         }
     }
 }
-#[derive(Debug, Copy, Clone, Default, PartialEq, Eq)]
-#[repr(u32)]
-pub enum __anon_1 {
-    #[default]
-    FOO = 1,
+#[derive(Debug, Copy, Clone)]
+#[repr(transparent)]
+pub struct __anon_1(u32);
+#[allow(non_upper_case_globals)]
+impl __anon_1 {
+    pub const FOO: __anon_1 = __anon_1(1);
 }
-"#;
+impl Default for __anon_1 {
+    fn default() -> Self { __anon_1::FOO }
+}"#;
 
     let mmap = build_btf_mmap(prog_text);
     let btf = btf_from_mmap(&mmap);