libbpf-cargo: enums with duplicated values cause compilation issues

[javierhonduco]

In 4e62c38, 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. 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 65094bc does not show this issue. Perhaps rather than emitting Rust enums 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

[danielocfb]

I've verified that the parent commit 65094bc 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 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.

[javierhonduco]

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.

[javierhonduco]

Just a quick one before logging off for the day 😄 javierhonduco@1582426. libbpf-cargo tests pass, but have not tested it with my project yet or made the whole test suite pass, yet.

[danielocfb]

Perhaps constants in a module name after the enum would be acceptable, but that would still not allow for passing them around similar to what we can with enums. But maybe we can get close with a unit type + constants. Basically,

struct bpf_map_type(u8 /*or whatever*/);

impl bpf_map_type {
	const BPF_MAP_TYPE_UNSPEC: bpf_map_type = bpf_map_type(1);
	const BPF_MAP_TYPE_CGROUP_STORAGE_DEPRECATED: bpf_map_type = bpf_map_type(19);
	const BPF_MAP_TYPE_CGROUP_STORAGE: bpf_map_type = bpf_map_type(19);
}

[javierhonduco]

A discussion on the topic of using const for enums: rust-lang/rust-bindgen#758

[anakryiko]

If it's possible to mark BPF_MAP_TYPE_CGROUP_STORAGE as deprecated (does Rust have some annotation or something like that?) then dropping BPF_MAP_TYPE_CGROUP_STORAGE_DEPRECATED completely would make total sense. This dance with BPF_MAP_TYPE_CGROUP_STORAGE_DEPRECATED and BPF_MAP_TYPE_CGROUP_STORAGE was meant just to make it obvious that BPF_MAP_TYPE_CGROUP_STORAGE is deprecated, however crude that approach is.

[javierhonduco]

Enums can't have repeated values as far as I am aware, and there are no escape hatches.

[danielocfb]

I'll be happy to submit a PR if this approach sounds reasonable.

@javierhonduco we should take a look at the generated skeleton once done, but I'd say that the newtype struct logic mentioned above would be reasonable to implement. Please feel free to open a PR if you agree.

[javierhonduco]

@danielocfb Sounds good, will submit a PR in the next few days

[javierhonduco]

Just submitted a PR, I can't add you as a reviewer for some reason 😕.

[SidongYang]

Hi, I have some problem about this issue.
After the patch resolves this issue, It seems that enum types could not be used with match expression. because there is no PartialEq for generated struct. Is it just missed or intentional change?

[d-e-s-o]

You should be able to match like so

https://github.com/libbpf/blazesym/blob/58307de7bd64b7e1b512736222bcd22abe0f01c3/capi/src/error.rs#L90-L94

But if PartialEq is missing we should probably add it. Feel free to open a PR.

[SidongYang]

https://github.com/libbpf/blazesym/blob/58307de7bd64b7e1b512736222bcd22abe0f01c3/capi/src/error.rs#L90-L94

In this code, blaze_err is enum type and it can be used for as u32. I think it's different for my case. The generated code have struct like below. I'm using transmute for converting to u32 for now.

#[derive(Debug, Copy, Clone)]
        #[repr(transparent)]
        pub struct event_type(u32);
        #[allow(non_upper_case_globals)]
        impl event_type {
            pub const A: event_type = event_type(0);
            pub const B: event_type = event_type(1);
        }
        impl Default for event_type {
            fn default() -> Self {
                event_type::A
            }
        }

[danielocfb]

I added PartialEq impl with #1014

[SidongYang]

@danielocfb thanks!

[javierhonduco]

Thanks, Daniel! Could I please ask you for a new release? Very keen to update the version of libbpf-rs we are using 😀

[danielocfb]

We can, but given that it's a breaking release I'd rather wait some more to see if more breaking changes can be included as well. I was going to look at #350 some time not too far out (I hope...), for example, though not sure when that will happen and if we can find a satisfactory solution.
Is there anything blocking you from just pinning the git snapshot for now @javierhonduco ?

[javierhonduco]

I get that minimising the number of releases with breaking changes is a good experience, but also the chance of introducing regressions might be smaller? Not sure. I know you care about user experience and minimising disruption, so I understand whatever you decide.

In my particular case, a fresh release would allow to publish my crate as typically some crate registries are not happy with git snapshots and only allow published releases. I haven't been able to upgrade libbpf-rs for a while due to this bug and there are other features that I would like to rely on, besides trying to use a newer version of the library (and not the 3+ month old one we run 😬 ).

[d-e-s-o]

We can certainly publish a beta release as a compromise. A subset of users seem to live at HEAD and that, among other things, is how bugs get spotted early. #982 (comment) is a prime example here, of course, but far from the only one from what I recall.

In this particular instance, I noticed that the logic is still wrong. We can't just unconditionally use u32 for the inner type. We need to honor signedness and width as reported by BTF. So this will need to get fixed before anything (not sure when I'll have time to get to that this week). On top of that, we will also need a way to more easily convert into the underlying integer type. transmute is too heavy a tool for the job. It should be more something like fn as_int(&self) -> <whatever-int-we-use> or so,or we could make the inner value public, actually.

[javierhonduco]

We can certainly publish a beta release as a compromise.

That works for me, thanks.

In this particular instance, I noticed that the logic is still wrong. We can't just unconditionally use u32 for the inner type.

Not sure exactly what you mean. The logic for choosing the inner type size did not change as far as I can see

libbpf-rs/libbpf-cargo/src/gen/btf.rs

Line 813 in 662887e

writeln!(def, r#"pub struct {enum_name}({signed}{repr_size});"#)?;

On top of that, we will also need a way to more easily convert into the underlying integer type. transmute is too heavy a tool for the job. It should be more something like fn as_int(&self) -> or so,or we could make the inner value public, actually.

Makes sense, definitely having to transmute is not ideal, sorry for the oversight.

[danielocfb]

Not sure exactly what you mean. The logic for choosing the inner type size did not change as far as I can see

Yeah, you are right. Not sure what I was looking at.

Will add the logic for "to int" conversion and then cut a beta release.

[danielocfb]

Could I please ask you for a new release?

FYI, https://crates.io/crates/libbpf-rs/0.25.0-beta.0 is out now