Re: [PATCH v3 01/16] rust: io: register: reimplement as proc macro
"Gary Guo" <[email protected]>
| Newsgroups | dev.linux.lists.nova-gpu,dev.linux.lists.driver-core,org.freedesktop.lists.dri-devel,org.kernel.vger.linux-kernel,org.kernel.vger.linux-pci,org.kernel.vger.rust-for-linux |
|---|---|
| Message-ID | <[email protected]> |
On Thu Aug 27, 2026 at 7:16 AM BST, Alexandre Courbot wrote:
> On Wed Aug 19, 2026 at 8:09 PM JST, Gary Guo wrote:
>> The existing `register!` macro is implemented as a declarative macro.
>> Reimplement it as proc macro instead, with no functional changes intended.
>>
>> The old implementation produces unhelpful diagnostics when things go wrong.
>> For example, for code like
>>
>> register! {
>> pub(crate) TESTREG(u32) {
>> 31:0 data;
>> }
>> }
>>
>> which misses out the "@ offset" part of the specification, and the
>> following error is produced:
>>
>> error: no rules expected `{`
>> --> test.rs:42:5
>> |
>> 42 | / register! {
>> 43 | | pub(crate) TESTREG(u32) {
>> 44 | | 31:0 data;
>> ... |
>> 100 | | }
>> | |_____^ no rules expected this token in macro call
>>
>> which isn't very helpful. With the proc macro implementation, the following
>> error is produced:
>>
>> error: expected `@` or `=>`
>> --> tests.rs:43:33
>> |
>> 43 | pub(crate) TESTREG(u32) {
>> | ^
>>
>> which is much more helpful. Apart from diagnostics, proc macro also has a
>> benefit of not having follow-set restrictions, which makes syntax like
>>
>> register!(name: ty @ offset);
>>
>> possible; declarative macro will reject this as `@` is not in the
>> follow-set of "ty" metavariable kind.
>>
>> Signed-off-by: Gary Guo <[email protected]>
>
> Well that patch was a great way for me to finally learn how to write
> proc macros! :)
>
> The parser and generator are both simple enough, but the new macro
> module would read a bit better with some light high-level doc (i.e.
> module-level and most important types). But even for a beginner like me
> this is almost self-explanatory, so no need to go too deep.
>
> <...>
>> diff --git a/rust/macros/io/mod.rs b/rust/macros/io/mod.rs
>> new file mode 100644
>> index 000000000000..39fa5bc302ba
>> --- /dev/null
>> +++ b/rust/macros/io/mod.rs
>> @@ -0,0 +1,3 @@
>> +// SPDX-License-Identifier: Apache-2.0 OR MIT
>
> Other files use GPL-2.0 as license, is this on purpose?
>
> <...>
>> +pub(crate) fn register(def: RegDef) -> Result<TokenStream> {
>> + let mut outputs = TokenStream::new();
>> +
>> + for reg in def.regs {
>> + let Reg {
>> + attrs,
>> + vis,
>> + name,
>> + storage,
>> + array,
>> + relative_base,
>> + offset,
>> + bitfield_args,
>> + } = reg;
>> +
>> + // Use register name's span for generated code, so error messages (if any) can point to it
>> + // instead of the entire register allocation.
>> + let span = name.span().resolved_at(Span::mixed_site());
>> +
>> + let offset = match offset {
>> + RegOffset::Fixed { offset } => quote!(#offset),
>> + RegOffset::Alias { alias } => {
>> + quote_spanned!(alias.span().resolved_at(span) =>
>> + <#alias as ::kernel::io::register::Register>::OFFSET
>> + )
>> + }
>> + RegOffset::ElementAlias { alias, idx } => {
>> + outputs.extend(quote_spanned!(idx.span().resolved_at(span) =>
>> + ::kernel::build_assert::static_assert!(
>> + #idx < <#alias as ::kernel::io::register::RegisterArray>::SIZE
>> + );
>> + ));
>> + quote_spanned!(alias.span().resolved_at(span) =>
>> + <#alias as ::kernel::io::register::Register>::OFFSET
>> + + #idx * <#alias as ::kernel::io::register::RegisterArray>::STRIDE
>
> Sashiko raised this but `#idx` should be within parentheses (this code
> is replaced later down the series but let's make it correct
> nonetheless).
Ack
>
> <...>
>> diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs
>> index 24f96feaeb34..5807dee84747 100644
>> --- a/rust/macros/lib.rs
>> +++ b/rust/macros/lib.rs
>> @@ -19,6 +19,7 @@
>> mod fmt;
>> mod for_lt;
>> mod helpers;
>> +mod io;
>> mod kunit;
>> mod module;
>> mod paste;
>> @@ -481,6 +482,15 @@ pub fn paste(input: TokenStream) -> TokenStream {
>> .into()
>> }
>>
>> +#[doc(hidden)] // Documented in `kernel` crate.
>> +#[proc_macro]
>> +#[allow(non_snake_case)]
>
> Is `non_snake_case` needed here?
You probably already guessed it but both this and the Apache license is from
copy-pasta :) Will fix in next version.
Best,
Gary