Re: [PATCH v4] checkpatch: add warning for pr_* and dev_* macros without a trailing newline

[Charalampos Mitrodimas <charmitro@posteo.net>]

Alban Kurti <kurti@invicto.ai> writes:

> 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.

This is now a more significant change, I belive we should document it
this under Documentation/dev-tools/checkpatch.rst. Where you can also
provide examples/use-cases.

>
> Suggested-by: Miguel Ojeda <ojeda@kernel.org>
> Closes: https://github.com/Rust-for-Linux/linux/issues/1140
> Signed-off-by: Alban Kurti <kurti@invicto.ai>
> ---
> Changes since v3:
> - Just reordered the checkpatch.pl code original addition as it did not work properly
> - Link to v3: https://lore.kernel.org/all/20250207-checkpatch-newline-v3-1-20d8774f16ea@invicto.ai/
> ---
>  scripts/checkpatch.pl | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 96 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 9eed3683ad76caffbbb2418e5dbea7551d374406..0e7684d2f0cf30575640d7c4da9e51a13d91463b 100755
> --- a/scripts/checkpatch.pl
> +++ b/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) = @_;
>  
> @@ -3888,6 +3890,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/) {
> @@ -7678,6 +7765,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) {
>
> ---
> base-commit: ceff0757f5dafb5be5205988171809c877b1d3e3
> change-id: 20250207-checkpatch-newline2-f60275e30eb6
>
> Best regards,