[PATCH] rust: workaround `bindgen` issue with forward references to `enum` types

[PATCH] rust: workaround `bindgen` issue with forward references to `enum` types

[Miguel Ojeda]

`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
94e05a66ea3e ("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: https://github.com/rust-lang/rust-bindgen/issues/3179 [1]
Link: https://lore.kernel.org/rust-for-linux/87tt7md1s6.fsf@kernel.org/ [2]
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
---
 rust/bindings/bindings_helper.h | 19 +++++++++++++++++++
 rust/kernel/time/hrtimer.rs     |  6 ++----
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index ccb988340df6..9f29855d5ab0 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -6,6 +6,25 @@
  * Sorted alphabetically.
  */
 
+/*
+ * First, avoid forward references to `enum` types.
+ *
+ * This workarounds a `bindgen` issue with them:
+ * <https://github.com/rust-lang/rust-bindgen/issues/3179>.
+ *
+ * Without this, the generated Rust type may be the wrong one (`i32`) or
+ * the proper one (typically `c_uint`) depending on how the headers are
+ * included, which in turn may depend on the particular kernel configuration
+ * or the architecture.
+ *
+ * The alternative would be to use casts and likely an
+ * `#[allow(clippy::unnecessary_cast)]` in the Rust source files. Instead,
+ * this approach allows us to keep the correct code in the source files and
+ * simply remove this section when the issue is fixed upstream and we bump
+ * the minimum `bindgen` version.
+ */
+#include <linux/hrtimer_types.h>
+
 #include <kunit/test.h>
 #include <linux/blk-mq.h>
 #include <linux/blk_types.h>
diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index ce53f8579d18..d52ce884303d 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -384,11 +384,9 @@ unsafe fn start(this: *const Self, expires: Ktime) {
 #[repr(u32)]
 pub enum HrTimerRestart {
     /// Timer should not be restarted.
-    #[allow(clippy::unnecessary_cast)]
-    NoRestart = bindings::hrtimer_restart_HRTIMER_NORESTART as u32,
+    NoRestart = bindings::hrtimer_restart_HRTIMER_NORESTART,
     /// Timer should be restarted.
-    #[allow(clippy::unnecessary_cast)]
-    Restart = bindings::hrtimer_restart_HRTIMER_RESTART as u32,
+    Restart = bindings::hrtimer_restart_HRTIMER_RESTART,
 }
 
 impl HrTimerRestart {
-- 
2.49.0

Re: [PATCH] rust: workaround `bindgen` issue with forward references to `enum` types

[Miguel Ojeda]

On Tue, Mar 25, 2025 at 7:43 PM Miguel Ojeda <ojeda@kernel.org> wrote:
>
> 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.

By the way, I was curious and from a quick look at generated bindings
lines matching `type.*i32` for a x86_64 build I have, I see only other
three cases:

  - `key_serial_t` is not an `enum`, i.e. it is correct.

  - `fs_value_type` and `drm_mode_status` are both `enum`s, and they
indeed suffer from this issue. However, we do not use them and I don't
see any patches posted for them either.

Those are good news.

However, there are also bad news: if for instance we actually needed
either of those two latter ones, then we cannot just include the
header that defines them, because that header happens to include
others that in turn use forward references to that `enum` the root one
defines.

So in some cases, if we start to use them before we have a fix, would
require C header changes to avoid the forward references. Thus, for
those, I think it may be simpler to instead use a cast and mark them
with a comment so that we know we should clean them up.

Cheers,
Miguel

Re: [PATCH] rust: workaround `bindgen` issue with forward references to `enum` types

[Miguel Ojeda]

On Tue, Mar 25, 2025 at 7:43 PM Miguel Ojeda <ojeda@kernel.org> wrote:
>
> `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
> 94e05a66ea3e ("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: https://github.com/rust-lang/rust-bindgen/issues/3179 [1]
> Link: https://lore.kernel.org/rust-for-linux/87tt7md1s6.fsf@kernel.org/ [2]
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

Andreas: are you OK with something along the lines of this change?

If you don't have anything against it, then I think it would be nice
to put it in, mainly so that the issue is documented, and so that we
don't forget about this possible workaround. Plus it removes an
`allow` and a cast, which is also nice.

The only downside, as far as I know, is that the workaround cannot be
always applied (see my sibling reply), but at least for this case, it
can be.

Thanks!

Cheers,
Miguel

Re: [PATCH] rust: workaround `bindgen` issue with forward references to `enum` types

[Andreas Hindborg]

Miguel Ojeda <ojeda@kernel.org> writes:

> `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
> 94e05a66ea3e ("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: https://github.com/rust-lang/rust-bindgen/issues/3179 [1]
> Link: https://lore.kernel.org/rust-for-linux/87tt7md1s6.fsf@kernel.org/ [2]
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>


Acked-by: Andreas Hindborg <a.hindborg@kernel.org>


Best regards,
Andreas Hindborg

Re: [PATCH] rust: workaround `bindgen` issue with forward references to `enum` types

[Miguel Ojeda]

On Tue, Mar 25, 2025 at 7:43 PM Miguel Ojeda <ojeda@kernel.org> wrote:
>
> `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
> 94e05a66ea3e ("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: https://github.com/rust-lang/rust-bindgen/issues/3179 [1]
> Link: https://lore.kernel.org/rust-for-linux/87tt7md1s6.fsf@kernel.org/ [2]
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

Applied to `rust-next` -- thanks everyone!

    [ Added extra paragraph on the comment to clarify that the workaround may
      not be possible in some cases. - Miguel ]

Cheers,
Miguel