[PATCH 0/2] rust: macros: fix `make rusttest` build on macOS

[PATCH 0/2] rust: macros: fix `make rusttest` build on macOS

[Tamir Duberstein]

Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
Tamir Duberstein (2):
      rust: macros: improve panic messages
      rust: macros: fix `make rusttest` build on macOS

 rust/macros/module.rs | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)
---
base-commit: beeb78d46249cab8b2b8359a2ce8fa5376b5ad2d
change-id: 20250207-macros-section-f7cddc7bbdea

Best regards,
-- 
Tamir Duberstein <tamird@gmail.com>

[PATCH 1/2] rust: macros: improve panic messages

[Tamir Duberstein]

Include unexpected input on parsing failures.  This has the side effect
of avoiding a spurious rust-analyzer warning:

  Variable `None` should have snake_case name, e.g. `none`

Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
 rust/macros/module.rs | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index cdf94f4982df..ca1b7e6a71ff 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -11,12 +11,14 @@ fn expect_string_array(it: &mut token_stream::IntoIter) -> Vec<String> {
     let mut it = group.stream().into_iter();
 
     while let Some(val) = try_string(&mut it) {
-        assert!(val.is_ascii(), "Expected ASCII string");
+        assert!(val.is_ascii(), "Expected ASCII string, got {}", val);
         values.push(val);
-        match it.next() {
-            Some(TokenTree::Punct(punct)) => assert_eq!(punct.as_char(), ','),
-            None => break,
-            _ => panic!("Expected ',' or end of array"),
+        let Some(token) = it.next() else {
+            break;
+        };
+        match token {
+            TokenTree::Punct(punct) => assert_eq!(punct.as_char(), ','),
+            token => panic!("Expected ',' or end of array, got {}", token),
         }
     }
     values
@@ -116,11 +118,10 @@ fn parse(it: &mut token_stream::IntoIter) -> Self {
         const REQUIRED_KEYS: &[&str] = &["type", "name", "license"];
         let mut seen_keys = Vec::new();
 
-        loop {
-            let key = match it.next() {
-                Some(TokenTree::Ident(ident)) => ident.to_string(),
-                Some(_) => panic!("Expected Ident or end"),
-                None => break,
+        while let Some(token) = it.next() {
+            let key = match token {
+                TokenTree::Ident(ident) => ident.to_string(),
+                token => panic!("Expected Ident or end, got {}", token),
             };
 
             if seen_keys.contains(&key) {

-- 
2.48.1

Re: [PATCH 1/2] rust: macros: improve panic messages

[Miguel Ojeda]

On Fri, Feb 7, 2025 at 6:22 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> Include unexpected input on parsing failures.  This has the side effect
> of avoiding a spurious rust-analyzer warning:
>
>   Variable `None` should have snake_case name, e.g. `none`

Hmm... That should be solved independently, but sure.

In any case, how is this related to the second patch in the series?
i.e. do you need both to solve the macOS issue?

> +        let Some(token) = it.next() else {
> +            break;
> +        };
> +        match token {
> +            TokenTree::Punct(punct) => assert_eq!(punct.as_char(), ','),
> +            token => panic!("Expected ',' or end of array, got {}", token),

Do we want to shadow here?

Also, I think you could write `{token}`. Same above.

Could you please show how the new output would look like in the commit message?

Cheers,
Miguel

Re: [PATCH 1/2] rust: macros: improve panic messages

[Tamir Duberstein]

On Fri, Feb 7, 2025 at 1:12 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Fri, Feb 7, 2025 at 6:22 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > Include unexpected input on parsing failures.  This has the side effect
> > of avoiding a spurious rust-analyzer warning:
> >
> >   Variable `None` should have snake_case name, e.g. `none`
>
> Hmm... That should be solved independently, but sure.
>
> In any case, how is this related to the second patch in the series?
> i.e. do you need both to solve the macOS issue?

No, it's not required - you're right to point that out. I'll send it
separately. Is it very inconvenient to consider the other patch on its
own?

> > +        let Some(token) = it.next() else {
> > +            break;
> > +        };
> > +        match token {
> > +            TokenTree::Punct(punct) => assert_eq!(punct.as_char(), ','),
> > +            token => panic!("Expected ',' or end of array, got {}", token),
>
> Do we want to shadow here?

I personally like this style because the shadow and shadowed are the
same variable and the presence of a binding in the match expression
hints to the reader that it's used somewhere in the body of the match
arm. Do you have another preference?

> Also, I think you could write `{token}`. Same above.

Good call. Will make it so.

> Could you please show how the new output would look like in the commit message?

Yes, I will include before and after output.

> Cheers,
> Miguel

[PATCH 2/2] rust: macros: fix `make rusttest` build on macOS

[Tamir Duberstein]

Do not emit `#[link_section = ".modinfo"]` on macOS (i.e. when building
userspace tests); .modinfo is not a legal section specifier in mach-o.

Before this change tests failed to compile:

  ---- ../rust/macros/lib.rs - module (line 66) stdout ----
  rustc-LLVM ERROR: Global variable '_ZN8rust_out13__module_init13__module_init27__MY_DEVICE_DRIVER_MODULE_017h141f80536770e0d4E' has an invalid section specifier '.modinfo': mach-o section specifier requires a segment and section separated by a comma.
  Couldn't compile the test.
  ---- ../rust/macros/lib.rs - module (line 33) stdout ----
  rustc-LLVM ERROR: Global variable '_ZN8rust_out13__module_init13__module_init20__MY_KERNEL_MODULE_017h5d79189564b41e07E' has an invalid section specifier '.modinfo': mach-o section specifier requires a segment and section separated by a comma.
  Couldn't compile the test.

Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
 rust/macros/module.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index ca1b7e6a71ff..f09431614f2b 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -58,7 +58,7 @@ fn emit_base(&mut self, field: &str, content: &str, builtin: bool) {
             "
                 {cfg}
                 #[doc(hidden)]
-                #[link_section = \".modinfo\"]
+                #[cfg_attr(not(target_os = \"macos\"), link_section = \".modinfo\")]
                 #[used]
                 pub static __{module}_{counter}: [u8; {length}] = *{string};
             ",

-- 
2.48.1

Re: [PATCH 2/2] rust: macros: fix `make rusttest` build on macOS

[Miguel Ojeda]

On Fri, Feb 7, 2025 at 6:22 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> Do not emit `#[link_section = ".modinfo"]` on macOS (i.e. when building
> userspace tests); .modinfo is not a legal section specifier in mach-o.

I thought the changes we already applied were the ones needed for
macOS support. Oh, well...

Cheers,
Miguel

Re: [PATCH 2/2] rust: macros: fix `make rusttest` build on macOS

[Tamir Duberstein]

On Fri, Feb 7, 2025 at 1:13 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Fri, Feb 7, 2025 at 6:22 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > Do not emit `#[link_section = ".modinfo"]` on macOS (i.e. when building
> > userspace tests); .modinfo is not a legal section specifier in mach-o.
>
> I thought the changes we already applied were the ones needed for
> macOS support. Oh, well...
>
> Cheers,
> Miguel

Yeah, that's on me. I didn't know about (and hadn't tried) `make
rusttest` until the cstr series[0].

[0] https://lore.kernel.org/all/20250203-cstr-core-v8-0-cb3f26e78686@gmail.com/t/#u

Re: [PATCH 0/2] rust: macros: fix `make rusttest` build on macOS

[Greg KH]

On Fri, Feb 07, 2025 at 12:21:51PM -0500, Tamir Duberstein wrote:
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> ---
> Tamir Duberstein (2):
>       rust: macros: improve panic messages
>       rust: macros: fix `make rusttest` build on macOS

Very minor nit, patch 0/X does not need a signed-off-by, and if you find
that you don't need any text for patch 0/X, there's no need to send a
patch 0/X at all!

Hopet this helps for future submissions.

thanks,

greg k-h

Re: [PATCH 0/2] rust: macros: fix `make rusttest` build on macOS

[Tamir Duberstein]

On Sat, Feb 8, 2025 at 3:17 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Fri, Feb 07, 2025 at 12:21:51PM -0500, Tamir Duberstein wrote:
> > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> > ---
> > Tamir Duberstein (2):
> >       rust: macros: improve panic messages
> >       rust: macros: fix `make rusttest` build on macOS
>
> Very minor nit, patch 0/X does not need a signed-off-by, and if you find
> that you don't need any text for patch 0/X, there's no need to send a
> patch 0/X at all!
>
> Hopet this helps for future submissions.
>
> thanks,
>
> greg k-h

Thanks Greg. This was me holding b4 wrong :(
Tamir