On Fri, Feb 28, 2025 at 4:40 PM Tamir Duberstein <tamird@xxxxxxxxx> wrote:
>
> On Fri, Feb 28, 2025 at 7:40 AM Alice Ryhl <aliceryhl@xxxxxxxxxx> 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).
Will do.
> > +/// 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?
It looks like this:
error: The #[export] attribute must be used on a function.
--> <linux>/rust/kernel/lib.rs:241:1
|
241 | #[export]
| ^^^^^^^^^
|
= note: this error originates in the attribute macro `export` (in
Nightly builds, run with -Z macro-backtrace for more info)
But I don't really think it's very useful to include this in the commit message.
> > + let no_mangle = "#[no_mangle]".parse::<TokenStream>().unwrap();
>
> Would this be simpler as `let no_mangle = quote!("#[no_mangle]");`?
I'll do that. It requires adding the # token to the previous commit.
> > +/// 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.
Will reword.
> > +#[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@xxxxxxxxx>
Thanks!
Alice
