Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Forward references to enum types end up generating the wrong type #3179

Open
ojeda opened this issue Mar 25, 2025 · 0 comments
Open

Forward references to enum types end up generating the wrong type #3179

ojeda opened this issue Mar 25, 2025 · 0 comments
Labels
A-enums bug I-bogus-codegen rust-for-linux Issues relevant to the Rust for Linux project

Comments

@ojeda
Copy link
Contributor

ojeda commented Mar 25, 2025

Forward references to enum types end up generating the wrong type for the enum. For instance:

enum E;
enum E { A };

generates:

pub const E_A: E = 0;
pub type E = i32;

instead of the expected:

pub const E_A: E = 0;
pub type E = ::std::os::raw::c_uint;

If we move it below:

enum E { A };
enum E;

or if we remove it:

enum E { A };

then the generated type is the expected one.

Another reproduction case, closer to what we hit in the kernel, was in a return type:

enum E f(void);
enum E { A };

In turn, this can mean that big codebases that #include headers from other headers end up in situations that are quite confusing.

For instance, in the Linux kernel, one may end up with a forward reference or not, depending on how the headers are included, which in turn may depend on the kernel configuration or the architecture. In turn, that means that we need to either be careful to avoid those (e.g. adding extra #includes to the top of our bindgen input to bring the actual definitions first), or developers may end up forcing casts and adding #[allow(clippy::unnecessary_cast)] (since the type of the generated constants, like E_A above, depends on the particular configuration):

#[repr(u32)]
pub enum HrTimerRestart {
    #[allow(clippy::unnecessary_cast)]
    NoRestart = bindings::hrtimer_restart_HRTIMER_NORESTART as u32,

    #[allow(clippy::unnecessary_cast)]
    Restart = bindings::hrtimer_restart_HRTIMER_RESTART as u32,
}

It is worth noting that GCC and Clang both allow these forward references to enum types and do not complain unless -Wpedantic is used:

warning: ISO C forbids forward references to 'enum' types [-Wpedantic]

Other compilers such as MSVC and TCC allow them, too.

Moreover, to double-check that the type of the actual enum in GCC and Clang is not changing on their side whether there is a forward reference or not, I added the following to the C examples above and it always passes (assuming the compiler picks unsigned int), which contradicts bindgen's output:

_Static_assert(_Generic((enum E)A, unsigned int: 1, default: 0), "");

Finally, I could reproduce the issue with bindgen 0.71.1 and libclang 20.1.1, i.e. latest releases, among others.

@ojeda ojeda added A-enums bug I-bogus-codegen rust-for-linux Issues relevant to the Rust for Linux project labels Mar 25, 2025
ojeda added a commit to ojeda/linux that referenced this issue Mar 25, 2025
`bindgen` currently generates the wrong type for an `enum` when there
is a forward reference to it. For instance:

    enum E;
    enum E { A };

generates:

    pub const E_A: E = 0;
    pub type E = i32;

instead of the expected:

    pub const E_A: E = 0;
    pub type E = ffi::c_uint;

The issue was reported to upstream `bindgen` [1].

Now, both GCC and Clang support silently these forward references to
`enum` types, unless `-Wpedantic` is passed, and it turns out that some
headers in the kernel depend on them.

Thus, depending on how the headers are included, which in turn may depend
on the kernel configuration or the architecture, we may get a different
type on the Rust side for a given C `enum`.

That can be quite confusing, to say the least, especially since
developers may only notice issues when building for other architectures
like in [2]. In particular, they may end up forcing a cast and adding
an `#[allow(clippy::unnecessary_cast)]` like it was done in commit
94e05a6 ("rust: hrtimer: allow timer restart from timer handler"),
which isn't great.

Instead, let's have a section at the top of our `bindings_helper.h` that
`#include`s the headers with the affected types -- hopefully there are
not many cases and there is a single ordering that covers all cases.

This allows us to remove the cast and the `#[allow]`, thus keeping the
correct code in the source files. When the issue gets resolved in upstream
`bindgen` (and we update our minimum `bindgen` version), we can easily
remove this section at the top.

Link: rust-lang/rust-bindgen#3179 [1]
Link: https://lore.kernel.org/rust-for-linux/87tt7md1s6.fsf@kernel.org/ [2]
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-enums bug I-bogus-codegen rust-for-linux Issues relevant to the Rust for Linux project
Projects
None yet
Development

No branches or pull requests

1 participant