linux-kernel-mentees.lists.linuxfoundation.org archive mirror
 help / color / mirror / Atom feed
* [Linux-kernel-mentees] [PATCH RFC 1/2] checkpatch: fix multi-statement macro checks
@ 2020-09-30 21:03 Dwaipayan Ray
  2020-09-30 21:03 ` [Linux-kernel-mentees] [PATCH RFC 2/2] " Dwaipayan Ray
  2020-10-01  8:14 ` [Linux-kernel-mentees] [PATCH RFC 1/2] " Lukas Bulwahn
  0 siblings, 2 replies; 5+ messages in thread
From: Dwaipayan Ray @ 2020-09-30 21:03 UTC (permalink / raw)
  To: lukas.bulwahn; +Cc: dwaipayanray1, linux-kernel-mentees

Checkpatch.pl generates errors which are false positive for
certain multi-statemenent macros.

The specific case investigated was whitespace separated statement
terminated by a semicolon. Checkpatch wrongly classifies such
as a multi-statement macro.

For example, commit 4649079b9de1 ("tracing: Make ftrace packed
events have align of 1") when analyzed with checkpatch generates:

ERROR: Macros with multiple statements should be enclosed in a do -
while loop
+#define __field_packed(type, container, item)    type item;

The error is misleading in this case and should not be produced.
The solution undertaken is to exclude any macro which doesn't
have any non-WSP character after the first and only semicolon(;)
present in it. This shall allow macros of the form "type item;",
"type item[size];", etc. to not generate the multi-statement macro
error.

Signed-off-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
---
 scripts/checkpatch.pl | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 9e65d21456f1..72c4072307ea 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5346,6 +5346,7 @@ sub process {
 			    $dstat !~ /^for\s*$Constant\s+(?:$Ident|-?$Constant)$/ &&	# for (...) bar()
 			    $dstat !~ /^do\s*{/ &&					# do {...
 			    $dstat !~ /^\(\{/ &&						# ({...
+			    $dstat !~ /^[^;]*;\s*$/ &&				# space separated statement;
 			    $ctx !~ /^.\s*#\s*define\s+TRACE_(?:SYSTEM|INCLUDE_FILE|INCLUDE_PATH)\b/)
 			{
 				if ($dstat =~ /^\s*if\b/) {
-- 
2.27.0

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [Linux-kernel-mentees] [PATCH RFC 2/2] checkpatch: fix multi-statement macro checks
  2020-09-30 21:03 [Linux-kernel-mentees] [PATCH RFC 1/2] checkpatch: fix multi-statement macro checks Dwaipayan Ray
@ 2020-09-30 21:03 ` Dwaipayan Ray
  2020-10-01  8:22   ` Lukas Bulwahn
  2020-10-01  8:14 ` [Linux-kernel-mentees] [PATCH RFC 1/2] " Lukas Bulwahn
  1 sibling, 1 reply; 5+ messages in thread
From: Dwaipayan Ray @ 2020-09-30 21:03 UTC (permalink / raw)
  To: lukas.bulwahn; +Cc: dwaipayanray1, linux-kernel-mentees

Checkpatch.pl doesn't have a check for excluding while (...) {...}
blocks from MULTISTATEMENT_MACRO_USE_DO_WHILE error.

For example, running checkpatch.pl on the file mm/access.c in the
kernel generates the following error:

ERROR: Macros with complex values should be enclosed in parentheses
+#define copy_from_kernel_nofault_loop(dst, src, len, type, err_label)	\
+	while (len >= sizeof(type)) {					\
+		__get_kernel_nofault(dst, src, type, err_label);	\
+		dst += sizeof(type);					\
+		src += sizeof(type);					\
+		len -= sizeof(type);					\
+	}

The error is misleading for this case. Enclosing it in a paranthesis
doesn't make any sense.

Checkpatch already has an exception list for such common macro types.
Added a new exception for while (...) {...} style blocks to the same.
This effectively fixed the wrong error message.

Signed-off-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
---
 scripts/checkpatch.pl | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 72c4072307ea..c2c211374662 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5342,6 +5342,7 @@ sub process {
 			    $dstat !~ /^\.$Ident\s*=/ &&				# .foo =
 			    $dstat !~ /^(?:\#\s*$Ident|\#\s*$Constant)\s*$/ &&		# stringification #foo
 			    $dstat !~ /^do\s*$Constant\s*while\s*$Constant;?$/ &&	# do {...} while (...); // do {...} while (...)
+			    $dstat !~ /^while\s*$Constant\s*$Constant\s*$/ &&			# while (...) {...}
 			    $dstat !~ /^for\s*$Constant$/ &&				# for (...)
 			    $dstat !~ /^for\s*$Constant\s+(?:$Ident|-?$Constant)$/ &&	# for (...) bar()
 			    $dstat !~ /^do\s*{/ &&					# do {...
-- 
2.27.0

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH RFC 1/2] checkpatch: fix multi-statement macro checks
  2020-09-30 21:03 [Linux-kernel-mentees] [PATCH RFC 1/2] checkpatch: fix multi-statement macro checks Dwaipayan Ray
  2020-09-30 21:03 ` [Linux-kernel-mentees] [PATCH RFC 2/2] " Dwaipayan Ray
@ 2020-10-01  8:14 ` Lukas Bulwahn
  1 sibling, 0 replies; 5+ messages in thread
From: Lukas Bulwahn @ 2020-10-01  8:14 UTC (permalink / raw)
  To: Dwaipayan Ray, Theodoros Chatziioannidis; +Cc: linux-kernel-mentees



On Thu, 1 Oct 2020, Dwaipayan Ray wrote:

> Checkpatch.pl generates errors which are false positive for
> certain multi-statemenent macros.
> 
> The specific case investigated was whitespace separated statement
> terminated by a semicolon. Checkpatch wrongly classifies such
> as a multi-statement macro.
> 
> For example, commit 4649079b9de1 ("tracing: Make ftrace packed
> events have align of 1") when analyzed with checkpatch generates:
> 
> ERROR: Macros with multiple statements should be enclosed in a do -
> while loop
> +#define __field_packed(type, container, item)    type item;
> 
> The error is misleading in this case and should not be produced.
> The solution undertaken is to exclude any macro which doesn't
> have any non-WSP character after the first and only semicolon(;)

We all know what a semicolon is; so, you do not need to put that in 
braces.

> present in it. This shall allow macros of the form "type item;",
> "type item[size];", etc. to not generate the multi-statement macro
> error.
>

I think the core reason this might work is that it is a string without any 
semicolon and then semicolon is at the very end of this expression.

Okay, we need an evaluation if this makes the whole situation better or 
worse...

Theodoros, can you help us and run checkpatch.pl for this one rule on the 
current kernel source code before and after the patch is applied, and let 
us know which differences you observe?

Dwaipayan, can you try to do a similar evaluation on git commits?

> Signed-off-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
> ---
>  scripts/checkpatch.pl | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 9e65d21456f1..72c4072307ea 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -5346,6 +5346,7 @@ sub process {
>  			    $dstat !~ /^for\s*$Constant\s+(?:$Ident|-?$Constant)$/ &&	# for (...) bar()
>  			    $dstat !~ /^do\s*{/ &&					# do {...
>  			    $dstat !~ /^\(\{/ &&						# ({...
> +			    $dstat !~ /^[^;]*;\s*$/ &&				# space separated statement;

I do not understand this comment "space separated statement". The example 
above was a macro for a type definition. Isn't that what you have in mind?


Give it another iteration, but I think the patch is pretty clear and so 
let us send it out to Joe and lkml with the next patch version.

Lukas

>  			    $ctx !~ /^.\s*#\s*define\s+TRACE_(?:SYSTEM|INCLUDE_FILE|INCLUDE_PATH)\b/)
>  			{
>  				if ($dstat =~ /^\s*if\b/) {
> -- 
> 2.27.0
> 
> 
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH RFC 2/2] checkpatch: fix multi-statement macro checks
  2020-09-30 21:03 ` [Linux-kernel-mentees] [PATCH RFC 2/2] " Dwaipayan Ray
@ 2020-10-01  8:22   ` Lukas Bulwahn
  2020-10-01 10:24     ` Dwaipayan Ray
  0 siblings, 1 reply; 5+ messages in thread
From: Lukas Bulwahn @ 2020-10-01  8:22 UTC (permalink / raw)
  To: Dwaipayan Ray, Theodoros Chatziioannidis; +Cc: linux-kernel-mentees



On Thu, 1 Oct 2020, Dwaipayan Ray wrote:

> Checkpatch.pl doesn't have a check for excluding while (...) {...}
> blocks from MULTISTATEMENT_MACRO_USE_DO_WHILE error.
> 
> For example, running checkpatch.pl on the file mm/access.c in the
> kernel generates the following error:
> 
> ERROR: Macros with complex values should be enclosed in parentheses
> +#define copy_from_kernel_nofault_loop(dst, src, len, type, err_label)	\
> +	while (len >= sizeof(type)) {					\
> +		__get_kernel_nofault(dst, src, type, err_label);	\
> +		dst += sizeof(type);					\
> +		src += sizeof(type);					\
> +		len -= sizeof(type);					\
> +	}
> 
> The error is misleading for this case. Enclosing it in a paranthesis
> doesn't make any sense.
>

s/a paranthesis/parentheses/

> Checkpatch already has an exception list for such common macro types.
> Added a new exception for while (...) {...} style blocks to the same.
> This effectively fixed the wrong error message.
>

This seems easier to understand and to judge than the other patch.

We will still need an evaluation, though. Theodoros, maybe you can help us 
here.

Dwaipayan, looks good to me. Do you fix up the spelling mistake and then 
send that patch to Joe and lkml?
 
> Signed-off-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
> ---
>  scripts/checkpatch.pl | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 72c4072307ea..c2c211374662 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -5342,6 +5342,7 @@ sub process {
>  			    $dstat !~ /^\.$Ident\s*=/ &&				# .foo =
>  			    $dstat !~ /^(?:\#\s*$Ident|\#\s*$Constant)\s*$/ &&		# stringification #foo
>  			    $dstat !~ /^do\s*$Constant\s*while\s*$Constant;?$/ &&	# do {...} while (...); // do {...} while (...)
> +			    $dstat !~ /^while\s*$Constant\s*$Constant\s*$/ &&			# while (...) {...}

Is it just the tab setting in my email client or is there a tab too much 
that it does not align with the other comments?

>  			    $dstat !~ /^for\s*$Constant$/ &&				# for (...)
>  			    $dstat !~ /^for\s*$Constant\s+(?:$Ident|-?$Constant)$/ &&	# for (...) bar()
>  			    $dstat !~ /^do\s*{/ &&					# do {...
> -- 
> 2.27.0
> 
> 
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH RFC 2/2] checkpatch: fix multi-statement macro checks
  2020-10-01  8:22   ` Lukas Bulwahn
@ 2020-10-01 10:24     ` Dwaipayan Ray
  0 siblings, 0 replies; 5+ messages in thread
From: Dwaipayan Ray @ 2020-10-01 10:24 UTC (permalink / raw)
  To: Lukas Bulwahn; +Cc: linux-kernel-mentees, Theodoros Chatziioannidis

On Thu, Oct 1, 2020 at 1:52 PM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
>
>
>
> On Thu, 1 Oct 2020, Dwaipayan Ray wrote:
>
> > Checkpatch.pl doesn't have a check for excluding while (...) {...}
> > blocks from MULTISTATEMENT_MACRO_USE_DO_WHILE error.
> >
> > For example, running checkpatch.pl on the file mm/access.c in the
> > kernel generates the following error:
> >
> > ERROR: Macros with complex values should be enclosed in parentheses
> > +#define copy_from_kernel_nofault_loop(dst, src, len, type, err_label)        \
> > +     while (len >= sizeof(type)) {                                   \
> > +             __get_kernel_nofault(dst, src, type, err_label);        \
> > +             dst += sizeof(type);                                    \
> > +             src += sizeof(type);                                    \
> > +             len -= sizeof(type);                                    \
> > +     }
> >
> > The error is misleading for this case. Enclosing it in a paranthesis
> > doesn't make any sense.
> >
>
> s/a paranthesis/parentheses/
>

I trusted checkpatch too much. Seems spelling.txt does not have an entry
for parentheses. (maybe not the most common spelling error).

> > Checkpatch already has an exception list for such common macro types.
> > Added a new exception for while (...) {...} style blocks to the same.
> > This effectively fixed the wrong error message.
> >
>
> This seems easier to understand and to judge than the other patch.
>
> We will still need an evaluation, though. Theodoros, maybe you can help us
> here.
>
> Dwaipayan, looks good to me. Do you fix up the spelling mistake and then
> send that patch to Joe and lkml?
>

Thanks, I will do that. Sending just this patch for now. The other one
needs more testing. I will send in the report once done.

> Is it just the tab setting in my email client or is there a tab too much
> that it does not align with the other comments?

Yes, it was an extra tab. The tabs at that particular place are too
non-uniform, causing confusion :(

Thanks,
Dwaipayan.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-10-01 10:25 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-30 21:03 [Linux-kernel-mentees] [PATCH RFC 1/2] checkpatch: fix multi-statement macro checks Dwaipayan Ray
2020-09-30 21:03 ` [Linux-kernel-mentees] [PATCH RFC 2/2] " Dwaipayan Ray
2020-10-01  8:22   ` Lukas Bulwahn
2020-10-01 10:24     ` Dwaipayan Ray
2020-10-01  8:14 ` [Linux-kernel-mentees] [PATCH RFC 1/2] " Lukas Bulwahn

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).