Message-ID: <CAJ-ks9ng6AqmDynFebR+2_ZEpmvxkUNWdTed2vr0kede0dxcxw@mail.gmail.com>
Date: Fri, 28 Feb 2025 10:40:19 -0500
From: Tamir Duberstein <tamird@...il.com>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Miguel Ojeda <ojeda@...nel.org>, 
	Petr Mladek <pmladek@...e.com>, Steven Rostedt <rostedt@...dmis.org>, 
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, 
	Rasmus Villemoes <linux@...musvillemoes.dk>, Sergey Senozhatsky <senozhatsky@...omium.org>, 
	Andrew Morton <akpm@...ux-foundation.org>, Boqun Feng <boqun.feng@...il.com>, 
	Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Trevor Gross <tmgross@...ch.edu>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, 
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, linux-kernel@...r.kernel.org, 
	rust-for-linux@...r.kernel.org, dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v2 3/5] rust: add #[export] macro

On Fri, Feb 28, 2025 at 7:40 AM Alice Ryhl <aliceryhl@...gle.com> wrote:
>
> Rust has two different tools for generating function declarations to
> call across the FFI boundary:
>
> * bindgen. Generates Rust declarations from a C header.
> * cbindgen. Generates C headers from Rust declarations.
>
> In the kernel, we only use bindgen. This is because cbindgen assumes a
> cargo-based buildsystem, so it is not compatible with the kernel's build
> system. This means that when C code calls a Rust function by name, its
> signature must be duplicated in both Rust code and a C header, and the
> signature needs to be kept in sync manually.

This needs an update given Miguel's comments on the cover letter. I
wonder if the code should also justify the choice (over cbindgen).

> To eliminate this manual checking, introduce a new macro that verifies
> at compile time that the two function declarations use the same
> signature. The idea is to run the C declaration through bindgen, and
> then have rustc verify that the function pointers have the same type.
>
> The signature must still be written twice, but at least you can no
> longer get it wrong. If the signatures don't match, you will get errors
> that look like this:
>
> error[E0308]: `if` and `else` have incompatible types
>   --> <linux>/rust/kernel/print.rs:22:22
>    |
> 21 | #[export]
>    | --------- expected because of this
> 22 | unsafe extern "C" fn rust_fmt_argument(
>    |                      ^^^^^^^^^^^^^^^^^ expected `u8`, found `i8`
>    |
>    = note: expected fn item `unsafe extern "C" fn(*mut u8, *mut u8, *mut c_void) -> *mut u8 {bindings::rust_fmt_argument}`
>               found fn item `unsafe extern "C" fn(*mut i8, *mut i8, *const c_void) -> *mut i8 {print::rust_fmt_argument}`
>
> It is unfortunate that the error message starts out by saying "`if` and
> `else` have incompatible types", but I believe the rest of the error
> message is reasonably clear and not too confusing.
>
> Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>
> ---
>  rust/kernel/prelude.rs |  2 +-
>  rust/macros/export.rs  | 28 ++++++++++++++++++++++++++++
>  rust/macros/helpers.rs | 19 ++++++++++++++++++-
>  rust/macros/lib.rs     | 24 ++++++++++++++++++++++++
>  4 files changed, 71 insertions(+), 2 deletions(-)
>
> diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs
> index dde2e0649790..889102f5a81e 100644
> --- a/rust/kernel/prelude.rs
> +++ b/rust/kernel/prelude.rs
> @@ -17,7 +17,7 @@
>  pub use crate::alloc::{flags::*, Box, KBox, KVBox, KVVec, KVec, VBox, VVec, Vec};
>
>  #[doc(no_inline)]
> -pub use macros::{module, pin_data, pinned_drop, vtable, Zeroable};
> +pub use macros::{export, module, pin_data, pinned_drop, vtable, Zeroable};
>
>  pub use super::{build_assert, build_error};
>
> diff --git a/rust/macros/export.rs b/rust/macros/export.rs
> new file mode 100644
> index 000000000000..c5ec75f2b07f
> --- /dev/null
> +++ b/rust/macros/export.rs
> @@ -0,0 +1,28 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +use crate::helpers::function_name;
> +use proc_macro::TokenStream;
> +
> +/// Please see [`crate::export`] for documentation.
> +pub(crate) fn export(_attr: TokenStream, ts: TokenStream) -> TokenStream {
> +    let Some(name) = function_name(ts.clone()) else {
> +        return "::core::compile_error!(\"The #[export] attribute must be used on a function.\");"
> +            .parse::<TokenStream>()
> +            .unwrap();
> +    };

Could you also show in the commit message what this error looks like
when the macro is misused?

> +
> +    // This verifies that the function has the same signature as the declaration generated by
> +    // bindgen. It makes use of the fact that all branches of an if/else must have the same type.
> +    let signature_check = quote!(
> +        const _: () = {
> +            if true {
> +                ::kernel::bindings::#name
> +            } else {
> +                #name
> +            };
> +        };
> +    );
> +
> +    let no_mangle = "#[no_mangle]".parse::<TokenStream>().unwrap();

Would this be simpler as `let no_mangle = quote!("#[no_mangle]");`?

> +    TokenStream::from_iter([signature_check, no_mangle, ts])
> +}
> diff --git a/rust/macros/helpers.rs b/rust/macros/helpers.rs
> index 563dcd2b7ace..3e04f8ecfc74 100644
> --- a/rust/macros/helpers.rs
> +++ b/rust/macros/helpers.rs
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>
> -use proc_macro::{token_stream, Group, TokenStream, TokenTree};
> +use proc_macro::{token_stream, Group, Ident, TokenStream, TokenTree};
>
>  pub(crate) fn try_ident(it: &mut token_stream::IntoIter) -> Option<String> {
>      if let Some(TokenTree::Ident(ident)) = it.next() {
> @@ -215,3 +215,20 @@ pub(crate) fn parse_generics(input: TokenStream) -> (Generics, Vec<TokenTree>) {
>          rest,
>      )
>  }
> +
> +/// Given a function declaration, finds the name of the function.
> +pub(crate) fn function_name(input: TokenStream) -> Option<Ident> {
> +    let mut input = input.into_iter();
> +    while let Some(token) = input.next() {
> +        match token {
> +            TokenTree::Ident(i) if i.to_string() == "fn" => {
> +                if let Some(TokenTree::Ident(i)) = input.next() {
> +                    return Some(i);
> +                }
> +                return None;
> +            }
> +            _ => continue,
> +        }
> +    }
> +    None
> +}
> diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs
> index d61bc6a56425..fbb2860e991f 100644
> --- a/rust/macros/lib.rs
> +++ b/rust/macros/lib.rs
> @@ -9,6 +9,7 @@
>  #[macro_use]
>  mod quote;
>  mod concat_idents;
> +mod export;
>  mod helpers;
>  mod module;
>  mod paste;
> @@ -174,6 +175,29 @@ pub fn vtable(attr: TokenStream, ts: TokenStream) -> TokenStream {
>      vtable::vtable(attr, ts)
>  }
>
> +/// Export a function so that C code can call it via a header file.
> +///
> +/// Functions exported using this macro can be called from C code using the declaration in the
> +/// appropriate header file. It should only be used in cases where C calls the function through a
> +/// header file; cases where C calls into Rust via a function pointer in a vtable (such as
> +/// `file_operations`) should not use this macro.
> +///
> +/// This macro has the following effect:
> +///
> +/// * Disables name mangling for this function.
> +/// * Verifies at compile-time that the function signature matches the declaration in the header
> +///   file.
> +///
> +/// You must declare the signature of the Rust function in a header file that is included by
> +/// `rust/bindings/bindings_helper.h`.
> +///
> +/// This macro is *not* the same as the C macros `EXPORT_SYMBOL_*`, since all Rust symbols are
> +/// currently automatically exported with `EXPORT_SYMBOL_GPL`.

nit: "since" implies causation, which isn't the case, I think.
Consider if ", since" can be replaced with a semicolon.

> +#[proc_macro_attribute]
> +pub fn export(attr: TokenStream, ts: TokenStream) -> TokenStream {
> +    export::export(attr, ts)
> +}
> +
>  /// Concatenate two identifiers.
>  ///
>  /// This is useful in macros that need to declare or reference items with names
>
> --
> 2.48.1.711.g2feabab25a-goog

Minor comments.

Reviewed-by: Tamir Duberstein <tamird@...il.com>

Powered by blists - more mailing lists