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

libbpf-cargo: Generate C enums as custom types with const fields #989

Conversation

javierhonduco
Copy link
Contributor

Rather than generating Rust enums from C enums, which might not be correct, as for example, C allows enum variants to share a value while Rust does not.

This fixes #982.

Test Plan

Verified that tests pass in CI in my fork and that my project compiles fine and works will with this change.

@javierhonduco javierhonduco force-pushed the libbpf-cargo-c-enums-as-custom-type-with-const-fields branch from d70329c to 9f0de1c Compare November 4, 2024 15:54
Copy link
Collaborator

@danielocfb danielocfb left a comment

Choose a reason for hiding this comment

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

Thanks for working on this fix! Overall this looks great, but I think I spotted an issue with the handling of default values. Please take another look. Also, can you please add a test for the generation of a Rust enum for a C correspondent with two variants of the same value?

libbpf-cargo/src/gen/btf.rs Outdated Show resolved Hide resolved
libbpf-cargo/src/gen/btf.rs Show resolved Hide resolved
@javierhonduco javierhonduco force-pushed the libbpf-cargo-c-enums-as-custom-type-with-const-fields branch from 9f0de1c to 6eaf3d0 Compare November 8, 2024 09:57
Rather than generating Rust enums from C enums, which might not be
correct, as for example, C allows enum variants to share a value while
Rust does not.

This fixes libbpf#982.

Test Plan
=========

Added regression test where a C defined enum with duplicated values is
converted to Rust using BTF and then successfully compiled with Rustc.

Verified that tests pass in CI in my fork and that my project compiles
fine and works well with this change.
@javierhonduco
Copy link
Contributor Author

Apologies for the delay and thanks for the feedback. Applied all the changes and added an integration tests to ensure compilation succeeds. I've also re-tested this branch with my project and it compiles fine. Just for completeness, the type that was causing issues before:

        #[derive(Debug, Copy, Clone)]
        #[repr(transparent)]
        pub struct bpf_map_type(u32);
        #[allow(non_upper_case_globals)]
        impl bpf_map_type {
            pub const BPF_MAP_TYPE_UNSPEC: bpf_map_type = bpf_map_type(0);
            pub const BPF_MAP_TYPE_HASH: bpf_map_type = bpf_map_type(1);
            pub const BPF_MAP_TYPE_ARRAY: bpf_map_type = bpf_map_type(2);
            pub const BPF_MAP_TYPE_PROG_ARRAY: bpf_map_type = bpf_map_type(3);
            pub const BPF_MAP_TYPE_PERF_EVENT_ARRAY: bpf_map_type = bpf_map_type(4);
            pub const BPF_MAP_TYPE_PERCPU_HASH: bpf_map_type = bpf_map_type(5);
            pub const BPF_MAP_TYPE_PERCPU_ARRAY: bpf_map_type = bpf_map_type(6);
            pub const BPF_MAP_TYPE_STACK_TRACE: bpf_map_type = bpf_map_type(7);
            pub const BPF_MAP_TYPE_CGROUP_ARRAY: bpf_map_type = bpf_map_type(8);
            pub const BPF_MAP_TYPE_LRU_HASH: bpf_map_type = bpf_map_type(9);
            pub const BPF_MAP_TYPE_LRU_PERCPU_HASH: bpf_map_type = bpf_map_type(10);
            pub const BPF_MAP_TYPE_LPM_TRIE: bpf_map_type = bpf_map_type(11);
            pub const BPF_MAP_TYPE_ARRAY_OF_MAPS: bpf_map_type = bpf_map_type(12);
            pub const BPF_MAP_TYPE_HASH_OF_MAPS: bpf_map_type = bpf_map_type(13);
            pub const BPF_MAP_TYPE_DEVMAP: bpf_map_type = bpf_map_type(14);
            pub const BPF_MAP_TYPE_SOCKMAP: bpf_map_type = bpf_map_type(15);
            pub const BPF_MAP_TYPE_CPUMAP: bpf_map_type = bpf_map_type(16);
            pub const BPF_MAP_TYPE_XSKMAP: bpf_map_type = bpf_map_type(17);
            pub const BPF_MAP_TYPE_SOCKHASH: bpf_map_type = bpf_map_type(18);
            pub const BPF_MAP_TYPE_CGROUP_STORAGE_DEPRECATED: bpf_map_type = bpf_map_type(19);
            pub const BPF_MAP_TYPE_CGROUP_STORAGE: bpf_map_type = bpf_map_type(19);
            pub const BPF_MAP_TYPE_REUSEPORT_SOCKARRAY: bpf_map_type = bpf_map_type(20);
            pub const BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE: bpf_map_type = bpf_map_type(21);
            pub const BPF_MAP_TYPE_QUEUE: bpf_map_type = bpf_map_type(22);
            pub const BPF_MAP_TYPE_STACK: bpf_map_type = bpf_map_type(23);
            pub const BPF_MAP_TYPE_SK_STORAGE: bpf_map_type = bpf_map_type(24);
            pub const BPF_MAP_TYPE_DEVMAP_HASH: bpf_map_type = bpf_map_type(25);
            pub const BPF_MAP_TYPE_STRUCT_OPS: bpf_map_type = bpf_map_type(26);
            pub const BPF_MAP_TYPE_RINGBUF: bpf_map_type = bpf_map_type(27);
            pub const BPF_MAP_TYPE_INODE_STORAGE: bpf_map_type = bpf_map_type(28);
            pub const BPF_MAP_TYPE_TASK_STORAGE: bpf_map_type = bpf_map_type(29);
            pub const BPF_MAP_TYPE_BLOOM_FILTER: bpf_map_type = bpf_map_type(30);
            pub const BPF_MAP_TYPE_USER_RINGBUF: bpf_map_type = bpf_map_type(31);
            pub const BPF_MAP_TYPE_CGRP_STORAGE: bpf_map_type = bpf_map_type(32);
        }
        impl Default for bpf_map_type {
            fn default() -> Self {
                bpf_map_type::BPF_MAP_TYPE_UNSPEC
            }
        }

@javierhonduco javierhonduco force-pushed the libbpf-cargo-c-enums-as-custom-type-with-const-fields branch from 6eaf3d0 to e31378b Compare November 8, 2024 10:10
@javierhonduco
Copy link
Contributor Author

(Pushed another update to write Rust compiler generated code to a temp folder and to fix a lint that I didn't see with my local version of Clippy)

Copy link
Collaborator

@danielocfb danielocfb left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks. I think I'll cut another patch release before merging this change, because I believe we have to treat it as breaking.

@danielocfb danielocfb merged commit e241983 into libbpf:master Nov 8, 2024
13 checks passed
danielocfb pushed a commit to d-e-s-o/libbpf-rs that referenced this pull request Nov 8, 2024
Fix the CHANGELOG entry for pull request libbpf#989 and convert the
compilation test to a proper end-to-end style one going all the way from
C code to buildable Rust project.

Signed-off-by: Daniel Müller <[email protected]>
danielocfb pushed a commit that referenced this pull request Nov 8, 2024
Fix the CHANGELOG entry for pull request #989 and convert the
compilation test to a proper end-to-end style one going all the way from
C code to buildable Rust project.

Signed-off-by: Daniel Müller <[email protected]>
@javierhonduco javierhonduco deleted the libbpf-cargo-c-enums-as-custom-type-with-const-fields branch November 8, 2024 20:04
@javierhonduco
Copy link
Contributor Author

Thanks! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

libbpf-cargo: enums with duplicated values cause compilation issues
2 participants