On Fri, Feb 28, 2025 at 9:20 AM Andreas Hindborg <a.hindborg@xxxxxxxxxx> wrote:
>
> "Alice Ryhl" <aliceryhl@xxxxxxxxxx> writes:
>
> > This macro behaves like #[no_mangle], but also performs an assertion
> > that the Rust function has the same signature as what is declared in the
> > C header.
> >
> > 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}`
> >
> > Signed-off-by: Alice Ryhl <aliceryhl@xxxxxxxxxx>
> > ---
> > rust/kernel/prelude.rs | 2 +-
> > rust/macros/export.rs | 25 +++++++++++++++++++++++++
> > rust/macros/helpers.rs | 19 ++++++++++++++++++-
> > rust/macros/lib.rs | 18 ++++++++++++++++++
> > rust/macros/quote.rs | 21 +++++++++++++++++++--
> > 5 files changed, 81 insertions(+), 4 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..3398e1655124
> > --- /dev/null
> > +++ b/rust/macros/export.rs
> > @@ -0,0 +1,25 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +use crate::helpers::function_name;
> > +use proc_macro::TokenStream;
> > +
> > +pub(crate) fn export(_attr: TokenStream, ts: TokenStream) -> TokenStream {
>
> This function is documented in macros/lib.rs. Could you insert a
> docstring with a link to the function that carries the docs?
These functions are not visible in the docs, and no other macro does that.
> Please describe how the function operates and what mechanics it uses to
> achieve its goal in a implementation detail comment.
>
> > + let Some(name) = function_name(ts.clone()) else {
> > + return "::core::compile_error!(\"The #[export] attribute must be used on a function.\");"
> > + .parse::<TokenStream>()
> > + .unwrap();
> > + };
> > +
> > + let signature_check = quote!(
> > + const _: () = {
> > + if true {
> > + ::kernel::bindings::#name
> > + } else {
> > + #name
> > + };
> > + };
> > + );
> > +
> > + let no_mangle = "#[no_mangle]".parse::<TokenStream>().unwrap();
> > + 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> {
>
> It would be great with a few tests for this function.
I don't think we have a mechanism for tests in the macro crate?
> > + 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..3cbf7705c4c1 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,23 @@ pub fn vtable(attr: TokenStream, ts: TokenStream) -> TokenStream {
> > vtable::vtable(attr, ts)
> > }
> >
> > +/// Export a function so that C code can call it.
> > +///
> > +/// This macro has the following effect:
> > +///
> > +/// * Disables name mangling for this function.
> > +/// * Verifies at compile-time that the function signature matches what's in the header file.
> > +///
> > +/// This macro requires that the function is mentioned in a C header file, and that the header file
> > +/// is included in `rust/bindings/bindings_helper.h`.
> > +///
> > +/// This macro is *not* the same as the C macro `EXPORT_SYMBOL*`, since all Rust symbols are
> > +/// currently automatically exported with `EXPORT_SYMBOL_GPL`.
>
> Perhaps add the following:
>
> This macro is useful when rust code is providing a function symbol whose
> signature is dictated by a C header file.
I do think this could use more info about when to use it. E.g., you
don't use it if C calls it via a vtable, but only if C calls it via a
declaration in a header file. I'll add more info.
> > +#[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
> > diff --git a/rust/macros/quote.rs b/rust/macros/quote.rs
> > index 33a199e4f176..c18960a91082 100644
> > --- a/rust/macros/quote.rs
> > +++ b/rust/macros/quote.rs
> > @@ -20,6 +20,12 @@ fn to_tokens(&self, tokens: &mut TokenStream) {
> > }
> > }
> >
> > +impl ToTokens for proc_macro::Ident {
> > + fn to_tokens(&self, tokens: &mut TokenStream) {
> > + tokens.extend([TokenTree::from(self.clone())]);
> > + }
> > +}
> > +
> > impl ToTokens for TokenTree {
> > fn to_tokens(&self, tokens: &mut TokenStream) {
> > tokens.extend([self.clone()]);
> > @@ -40,7 +46,7 @@ fn to_tokens(&self, tokens: &mut TokenStream) {
> > /// `quote` crate but provides only just enough functionality needed by the current `macros` crate.
> > macro_rules! quote_spanned {
> > ($span:expr => $($tt:tt)*) => {{
> > - let mut tokens;
> > + let mut tokens: ::std::vec::Vec<::proc_macro::TokenTree>;
> > #[allow(clippy::vec_init_then_push)]
> > {
> > tokens = ::std::vec::Vec::new();
> > @@ -65,7 +71,8 @@ macro_rules! quote_spanned {
> > quote_spanned!(@proc $v $span $($tt)*);
> > };
> > (@proc $v:ident $span:ident ( $($inner:tt)* ) $($tt:tt)*) => {
> > - let mut tokens = ::std::vec::Vec::new();
> > + #[allow(unused_mut)]
> > + let mut tokens = ::std::vec::Vec::<::proc_macro::TokenTree>::new();
> > quote_spanned!(@proc tokens $span $($inner)*);
> > $v.push(::proc_macro::TokenTree::Group(::proc_macro::Group::new(
> > ::proc_macro::Delimiter::Parenthesis,
> > @@ -136,6 +143,16 @@ macro_rules! quote_spanned {
> > ));
> > quote_spanned!(@proc $v $span $($tt)*);
> > };
> > + (@proc $v:ident $span:ident = $($tt:tt)*) => {
> > + $v.push(::proc_macro::TokenTree::Punct(
> > + ::proc_macro::Punct::new('=', ::proc_macro::Spacing::Alone)
> > + ));
> > + quote_spanned!(@proc $v $span $($tt)*);
> > + };
> > + (@proc $v:ident $span:ident _ $($tt:tt)*) => {
> > + $v.push(::proc_macro::TokenTree::Ident(::proc_macro::Ident::new("_", $span)));
> > + quote_spanned!(@proc $v $span $($tt)*);
> > + };
> > (@proc $v:ident $span:ident $id:ident $($tt:tt)*) => {
> > $v.push(::proc_macro::TokenTree::Ident(::proc_macro::Ident::new(stringify!($id), $span)));
> > quote_spanned!(@proc $v $span $($tt)*);
>
> The update to `impl ToTokens for TokenTree` should be split out in a
> separate patch and should carry some explanation of the change.
I think this case is borderline for whether it's necessary to split
up, but okay.
Alice
