[merged mm-nonmm-stable] checkpatch-add-warning-for-pr_-and-dev_-macros-without-a-trailing-newline.patch removed from -mm tree

[merged mm-nonmm-stable] checkpatch-add-warning-for-pr_-and-dev_-macros-without-a-trailing-newline.patch removed from -mm tree

[Andrew Morton]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 6839 bytes --]


The quilt patch titled
     Subject: checkpatch: add warning for pr_* and dev_* macros without a trailing newline
has been removed from the -mm tree.  Its filename was
     checkpatch-add-warning-for-pr_-and-dev_-macros-without-a-trailing-newline.patch

This patch was dropped because it was merged into the mm-nonmm-stable branch
of git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm

------------------------------------------------------
From: Alban Kurti <kurti@invicto.ai>
Subject: checkpatch: add warning for pr_* and dev_* macros without a trailing newline
Date: Fri, 07 Feb 2025 18:39:06 +0000 (UTC)

Add a new check in scripts/checkpatch.pl to detect usage of pr_(level) and
dev_(level) macros (for both C and Rust) when the string literal does not
end with '\n'.  Missing trailing newlines can lead to incomplete log lines
that do not appear properly in dmesg or in console output.  To show an
example of this working after applying the patch we can run the script on
the commit that likely motivated this need/issue:

  ./scripts/checkpatch.pl --strict -g "f431c5c581fa1"

Also, the patch is able to handle correctly if there is a printing call
without a newline which then has a newline printed via pr_cont for both
Rust and C alike.  If there is no newline printed and the patch ends or
there is another pr_* call before a newline with pr_cont is printed it
will show a warning.  Not implemented for dev_cont because it is not clear
to me if that is used at all.

One false warning that will be generated due to this change is in case we
have a patch that modifies a `pr_* call without a newline` which has a
pr_cont with a newline following it.  In this case there will be a warning
but because the patch does not include the following pr_cont it will warn
there is nothing creating a newline.  I have modified the warning to be
softer due to this known problem.

I have tested with comments, whitespace, differen orders of pr_* calls and
pr_cont and the only case that I suspect to be a problem is the one
outlined above.

Link: https://lkml.kernel.org/r/20250207-checkpatch-newline2-v4-1-26d8e80d0059@invicto.ai
Signed-off-by: Alban Kurti <kurti@invicto.ai>
Suggested-by: Miguel Ojeda <ojeda@kernel.org>
Closes: https://github.com/Rust-for-Linux/linux/issues/1140
Cc: Alex Gaynor <alex.gaynor@gmail.com>
Cc: Alice Ryhl <aliceryhl@google.com>
Cc: Andreas Hindborg <a.hindborg@kernel.org>
Cc: Andy Whitcroft <apw@canonical.com>
Cc: Benno Lossin <benno.lossin@proton.me>
Cc: Björn Roy Baron <bjorn3_gh@protonmail.com>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Dwaipayan Ray <dwaipayanray1@gmail.com>
Cc: Gary Guo <gary@garyguo.net>
Cc: Joe Perches <joe@perches.com>
Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Cc: Trevor Gross <tmgross@umich.edu>
Cc: Charalampos Mitrodimas <charmitro@posteo.net>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 scripts/checkpatch.pl |   96 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 96 insertions(+)

--- a/scripts/checkpatch.pl~checkpatch-add-warning-for-pr_-and-dev_-macros-without-a-trailing-newline
+++ a/scripts/checkpatch.pl
@@ -77,6 +77,8 @@ my ${CONFIG_} = "CONFIG_";
 
 my %maybe_linker_symbol; # for externs in c exceptions, when seen in *vmlinux.lds.h
 
+my $pending_log = undef;
+
 sub help {
 	my ($exitcode) = @_;
 
@@ -3878,6 +3880,91 @@ sub process {
 			}
 		}
 
+# check for pr_* and dev_* logs without a newline for C and Rust files to avoid missing log messages
+  my $pr_cont_pattern = qr{
+      \b
+      pr_cont!?
+      \s*
+      \(
+      \s*
+      "([^"]*)"
+      [^)]*
+      \)
+  }x;
+  my $log_macro_pattern = qr{
+      \b
+      (
+          pr_(?:emerg|alert|crit|err|warn|notice|info|debug)
+        | dev_(?:emerg|alert|crit|err|warn|notice|info|dbg)
+      )
+      (!?)
+      \s*
+      \(
+      \s*
+      "([^"]*)"
+  }x;
+
+  if ($realfile =~ /\.(?:c|h|rs)$/) {
+      if ($rawline =~ /^\+/) {
+          my $cleanline = $rawline;
+          $cleanline =~ s/^[+\s]+//;
+          $cleanline =~ s/\r?$//;
+          $cleanline =~ s{/\*.*?\*/}{}g;
+          $cleanline =~ s{//.*}{}g;
+
+          if ($pending_log) {
+              if ($cleanline =~ /$pr_cont_pattern/) {
+                  my $cont_string_arg = $1;
+                  if ($cont_string_arg =~ /\\n$/) {
+                      $pending_log = undef;
+                  }
+              } elsif ($cleanline =~ /$log_macro_pattern/) {
+                  WARN($pending_log->{lang} . "_LOG_NO_NEWLINE",
+                       "Possible usage of $pending_log->{macro_call} without a trailing newline.\n" .
+                       $pending_log->{herecurr});
+
+                  $pending_log = undef;
+
+                  my $macro_call  = $1;
+                  my $maybe_excl  = $2;
+                  my $string_arg  = $3;
+                  $string_arg =~ s/\s+$//;
+
+                  if ($realfile =~ /\.rs$/ && $maybe_excl ne '!') {
+                      return;
+                  }
+
+                  if ($string_arg !~ /\\n$/ && $string_arg !~ /\n$/) {
+                      $pending_log = {
+                          macro_call => $macro_call,
+                          herecurr => $herecurr,
+                          lang => ($realfile =~ /\.rs$/) ? "Rust" : "C",
+                      };
+                  }
+              }
+          } else {
+              if ($cleanline =~ /$log_macro_pattern/) {
+                  my $macro_call = $1;
+                  my $maybe_excl = $2;
+                  my $string_arg = $3;
+                  $string_arg =~ s/\s+$//;
+
+                  if ($realfile =~ /\.rs$/ && $maybe_excl ne '!') {
+                      return;
+                  }
+
+                  if ($string_arg !~ /\\n$/ && $string_arg !~ /\n$/) {
+                      $pending_log = {
+                          macro_call => $macro_call,
+                          herecurr   => $herecurr,
+                          lang       => ($realfile =~ /\.rs$/) ? "Rust" : "C",
+                      };
+                  }
+              }
+          }
+      }
+  }
+
 # check for .L prefix local symbols in .S files
 		if ($realfile =~ /\.S$/ &&
 		    $line =~ /^\+\s*(?:[A-Z]+_)?SYM_[A-Z]+_(?:START|END)(?:_[A-Z_]+)?\s*\(\s*\.L/) {
@@ -7670,6 +7757,15 @@ sub process {
 		}
 	}
 
+# pending log means a pr_* without an ending newline has not
+# been followed by a pr_cont call with a newline at the end
+  if ($pending_log) {
+    WARN($pending_log->{lang} . "_LOG_NO_NEWLINE",
+      "Usage of $pending_log->{macro_call} without a trailing newline.\n" .
+      $pending_log->{herecurr});
+    $pending_log = undef;
+  }
+
 	# If we have no input at all, then there is nothing to report on
 	# so just keep quiet.
 	if ($#rawlines == -1) {
_

Patches currently in -mm which might be from kurti@invicto.ai are

Re: [merged mm-nonmm-stable] checkpatch-add-warning-for-pr_-and-dev_-macros-without-a-trailing-newline.patch removed from -mm tree

[Joe Perches]

On Sun, 2025-03-16 at 22:31 -0700, Andrew Morton wrote:
> The quilt patch titled
>      Subject: checkpatch: add warning for pr_* and dev_* macros without a trailing newline
> has been removed from the -mm tree.  Its filename was
>      checkpatch-add-warning-for-pr_-and-dev_-macros-without-a-trailing-newline.patch
> 
> This patch was dropped because it was merged into the mm-nonmm-stable branch
> of git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
> 
> ------------------------------------------------------
> From: Alban Kurti <kurti@invicto.ai>
> Subject: checkpatch: add warning for pr_* and dev_* macros without a trailing newline
> Date: Fri, 07 Feb 2025 18:39:06 +0000 (UTC)
> 
> Add a new check in scripts/checkpatch.pl to detect usage of pr_(level) and
> dev_(level) macros (for both C and Rust) when the string literal does not
> end with '\n'.  Missing trailing newlines can lead to incomplete log lines
> that do not appear properly in dmesg or in console output.  To show an
> example of this working after applying the patch we can run the script on
> the commit that likely motivated this need/issue:
> 
>   ./scripts/checkpatch.pl --strict -g "f431c5c581fa1"

Not sure it's a 'real' need/issue as any new KERN_<LEVEL> that's
not KERN_CONT will already add a new line.

And there are many other <foo>_<level> calls that do not add
newlines to the macros or functions that could be used too.

> One false warning that will be generated due to this change is in case we
> have a patch that modifies a `pr_* call without a newline` which has a
> pr_cont with a newline following it.  In this case there will be a warning
> but because the patch does not include the following pr_cont it will warn
> there is nothing creating a newline.  I have modified the warning to be
> softer due to this known problem.

I think this patch is a merely a conversion of one defect for another
and it's quite compilcated code too.

Re: [merged mm-nonmm-stable] checkpatch-add-warning-for-pr_-and-dev_-macros-without-a-trailing-newline.patch removed from -mm tree

[Andrew Morton]

On Sun, 16 Mar 2025 22:51:53 -0700 Joe Perches <joe@perches.com> wrote:

> > One false warning that will be generated due to this change is in case we
> > have a patch that modifies a `pr_* call without a newline` which has a
> > pr_cont with a newline following it.  In this case there will be a warning
> > but because the patch does not include the following pr_cont it will warn
> > there is nothing creating a newline.  I have modified the warning to be
> > softer due to this known problem.
> 
> I think this patch is a merely a conversion of one defect for another
> and it's quite compilcated code too.

Well darn, it would have been nice to receive this review during the
review!

Should I drop the patch?

Re: [merged mm-nonmm-stable] checkpatch-add-warning-for-pr_-and-dev_-macros-without-a-trailing-newline.patch removed from -mm tree

[Joe Perches]

On Sun, 2025-03-16 at 23:09 -0700, Andrew Morton wrote:
> On Sun, 16 Mar 2025 22:51:53 -0700 Joe Perches <joe@perches.com> wrote:
> 
> > > One false warning that will be generated due to this change is in case we
> > > have a patch that modifies a `pr_* call without a newline` which has a
> > > pr_cont with a newline following it.  In this case there will be a warning
> > > but because the patch does not include the following pr_cont it will warn
> > > there is nothing creating a newline.  I have modified the warning to be
> > > softer due to this known problem.
> > 
> > I think this patch is a merely a conversion of one defect for another
> > and it's quite compilcated code too.
> 
> Well darn, it would have been nice to receive this review during the
> review!
> 
> Should I drop the patch?

I don't think it's particularly valuable or useful. 

Re: [merged mm-nonmm-stable] checkpatch-add-warning-for-pr_-and-dev_-macros-without-a-trailing-newline.patch removed from -mm tree

[Andrew Morton]

On Sun, 16 Mar 2025 23:12:06 -0700 Joe Perches <joe@perches.com> wrote:

> On Sun, 2025-03-16 at 23:09 -0700, Andrew Morton wrote:
> > On Sun, 16 Mar 2025 22:51:53 -0700 Joe Perches <joe@perches.com> wrote:
> > 
> > > > One false warning that will be generated due to this change is in case we
> > > > have a patch that modifies a `pr_* call without a newline` which has a
> > > > pr_cont with a newline following it.  In this case there will be a warning
> > > > but because the patch does not include the following pr_cont it will warn
> > > > there is nothing creating a newline.  I have modified the warning to be
> > > > softer due to this known problem.
> > > 
> > > I think this patch is a merely a conversion of one defect for another
> > > and it's quite compilcated code too.
> > 
> > Well darn, it would have been nice to receive this review during the
> > review!
> > 
> > Should I drop the patch?
> 
> I don't think it's particularly valuable or useful. 

OK, thanks, I dropped the patch.

Re: [merged mm-nonmm-stable] checkpatch-add-warning-for-pr_-and-dev_-macros-without-a-trailing-newline.patch removed from -mm tree

[Miguel Ojeda]

On Mon, Mar 17, 2025 at 6:52 AM Joe Perches <joe@perches.com> wrote:
>
> Not sure it's a 'real' need/issue as any new KERN_<LEVEL> that's
> not KERN_CONT will already add a new line.

But it is still intended that one uses an explicit newline, no? I am
not sure I follow your other comment, so I may be missing something
though.

Regarding complexity, agreed, it is fairly involved -- I think the
continuation case could be ignored, since it is rare and it is just a
warning anyway.

Cheers,
Miguel

Re: [merged mm-nonmm-stable] checkpatch-add-warning-for-pr_-and-dev_-macros-without-a-trailing-newline.patch removed from -mm tree

[Joe Perches]

On Mon, 2025-03-17 at 20:50 +0100, Miguel Ojeda wrote:
> On Mon, Mar 17, 2025 at 6:52 AM Joe Perches <joe@perches.com> wrote:
> > 
> > Not sure it's a 'real' need/issue as any new KERN_<LEVEL> that's
> > not KERN_CONT will already add a new line.
> 
> But it is still intended that one uses an explicit newline, no?

That's my preference, yes.

> I am
> not sure I follow your other comment, so I may be missing something
> though.

My understanding is that
	printk(KERN_INFO "foo");	// or pr_info("foo");
//with some long time before
	printk(KERN_INFO "bar");	// or pr_info("bar");

will defer emitting "foo" and will append a newline to "foo"
when emitted only after the printk for "bar" is performed.

So it's generally useful to always use a newline for
complete messages as those are never deferred until
another printk is done.

> Regarding complexity, agreed, it is fairly involved -- I think the
> continuation case could be ignored, since it is rare and it is just a
> warning anyway.

My belief as well.

There are a number of macros and functions that are printk equivalents
that append newlines automatically and as well some pr_fmt uses
that include newlines too so it's almost impossible to get checkpatch
to work correctly anyway.

Things like:

fs/bcachefs/bcachefs.h:#define pr_fmt(fmt) "bcachefs: %s() " fmt "\n", __func__