Checkpatch generates incorrect error message for certain macros. When checkpatch was run on commit 4649079b9de1 "tracing: Make ftrace packed events have align of 1"), it generated multiple errors of the form: ERROR: Macros with multiple statements should be enclosed in a do - while loop +#define __field_packed(type, container, item) type item; This should certainly not be enclosed in a do - while loop. The problem was with the trailing semicolon. Any statement with trailing semicolon was marked as a multi-line macro. This was fixed by having a seperate rule for excluding single statements terminated by semicolon. The second problem is due to while (...) {...} macros not being handled. For example, on commit fe557319aa06 ("maccess: rename probe_kernel_ {read,write} to copy_{from,to}_kernel_nofault"), checkpatch generated following error: ERROR: Macros with multiple statements should be enclosed in a do - while loop +#define copy_to_kernel_nofault_loop(dst, src, len, type, err_label) \ while (len >= sizeof(type)) { .... This was fixed by having a seperate rule to match such while blocks. Signed-off-by: Dwaipayan Ray <dwaipayanray1@gmail.com> --- scripts/checkpatch.pl | 2 ++ 1 file changed, 2 insertions(+) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 9e65d21456f1..4fb612200c27 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -5342,10 +5342,12 @@ 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$/ && # while (...) {...} $dstat !~ /^for\s*$Constant$/ && # for (...) $dstat !~ /^for\s*$Constant\s+(?:$Ident|-?$Constant)$/ && # for (...) bar() $dstat !~ /^do\s*{/ && # do {... $dstat !~ /^\(\{/ && # ({... + $dstat !~ /;$/ && # 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
Dwaipayan, did you checkpatch.pl your patch before sending it out? On Wed, 30 Sep 2020, Dwaipayan Ray wrote: > Checkpatch generates incorrect error message for certain > macros. > > When checkpatch was run on commit 4649079b9de1 > "tracing: Make ftrace packed events have align of 1"), it brace ( is missing, checkpatch should warn you. > generated multiple errors of the form: > Present tense is fine. checkpatch runs on ..., it generates ... > ERROR: Macros with multiple statements should be enclosed in a do - > while loop > +#define __field_packed(type, container, item) type item; > > This should certainly not be enclosed in a do - while loop. > The problem was with the trailing semicolon. Any statement > with trailing semicolon was marked as a multi-line macro. > This was fixed by having a seperate rule for excluding > single statements terminated by semicolon. > > The second problem is due to while (...) {...} macros not being > handled. > For example, on commit fe557319aa06 ("maccess: rename probe_kernel_ > {read,write} to copy_{from,to}_kernel_nofault"), checkpatch > generated following error: > > ERROR: Macros with multiple statements should be enclosed in a do - > while loop > +#define copy_to_kernel_nofault_loop(dst, src, len, type, err_label) \ > while (len >= sizeof(type)) { > .... > > This was fixed by having a seperate rule to match such while blocks. > s/seperate/separate/ If you address two different problems, make two patches not one. Kernel developers can handle many patches... Create two patches in a patch series, checkpatch.pl them, send them out here; then we will do a quick review and out they go to Joe and lkml. Lukas > Signed-off-by: Dwaipayan Ray <dwaipayanray1@gmail.com> > --- > scripts/checkpatch.pl | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 9e65d21456f1..4fb612200c27 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -5342,10 +5342,12 @@ 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$/ && # while (...) {...} tab and spacing might be different than lines around it? > $dstat !~ /^for\s*$Constant$/ && # for (...) > $dstat !~ /^for\s*$Constant\s+(?:$Ident|-?$Constant)$/ && # for (...) bar() > $dstat !~ /^do\s*{/ && # do {... > $dstat !~ /^\(\{/ && # ({... > + $dstat !~ /;$/ && # 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
On Thu, Oct 1, 2020 at 12:40 AM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote: > > > Dwaipayan, did you checkpatch.pl your patch before sending it out? > Oh right! I goofed up! Sorry about that. > On Wed, 30 Sep 2020, Dwaipayan Ray wrote: > > > Checkpatch generates incorrect error message for certain > > macros. > > > > When checkpatch was run on commit 4649079b9de1 > > "tracing: Make ftrace packed events have align of 1"), it > > brace ( is missing, checkpatch should warn you. > Again, I may have been too hasty in sending out the patch :( > > generated multiple errors of the form: > > > Present tense is fine. checkpatch runs on ..., > it generates ... > > > ERROR: Macros with multiple statements should be enclosed in a do - > > while loop > > +#define __field_packed(type, container, item) type item; > > > > This should certainly not be enclosed in a do - while loop. > > The problem was with the trailing semicolon. Any statement > > with trailing semicolon was marked as a multi-line macro. > > This was fixed by having a seperate rule for excluding > > single statements terminated by semicolon. > > > > The second problem is due to while (...) {...} macros not being > > handled. > > For example, on commit fe557319aa06 ("maccess: rename probe_kernel_ > > {read,write} to copy_{from,to}_kernel_nofault"), checkpatch > > generated following error: > > > > ERROR: Macros with multiple statements should be enclosed in a do - > > while loop > > +#define copy_to_kernel_nofault_loop(dst, src, len, type, err_label) \ > > while (len >= sizeof(type)) { > > .... > > > > This was fixed by having a seperate rule to match such while blocks. > > > > s/seperate/separate/ > > If you address two different problems, make two patches not one. > > Kernel developers can handle many patches... > > Create two patches in a patch series, checkpatch.pl them, send them out > here; then we will do a quick review and out they go to Joe and lkml. > Sure, I will do that. > Lukas > > > Signed-off-by: Dwaipayan Ray <dwaipayanray1@gmail.com> > > --- > > scripts/checkpatch.pl | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > > index 9e65d21456f1..4fb612200c27 100755 > > --- a/scripts/checkpatch.pl > > +++ b/scripts/checkpatch.pl > > @@ -5342,10 +5342,12 @@ 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$/ && # while (...) {...} > > tab and spacing might be different than lines around it? Weirdly, the other lines had 4 spaces instead of one of the tabs. Will correct that. > > > $dstat !~ /^for\s*$Constant$/ && # for (...) > > $dstat !~ /^for\s*$Constant\s+(?:$Ident|-?$Constant)$/ && # for (...) bar() > > $dstat !~ /^do\s*{/ && # do {... > > $dstat !~ /^\(\{/ && # ({... > > + $dstat !~ /;$/ && # 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
On Thu, 1 Oct 2020, Dwaipayan Ray wrote: > On Thu, Oct 1, 2020 at 12:40 AM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote: > > > > > > Dwaipayan, did you checkpatch.pl your patch before sending it out? > > > > Oh right! I goofed up! Sorry about that. > > > On Wed, 30 Sep 2020, Dwaipayan Ray wrote: > > > > > Checkpatch generates incorrect error message for certain > > > macros. > > > > > > When checkpatch was run on commit 4649079b9de1 > > > "tracing: Make ftrace packed events have align of 1"), it > > > > brace ( is missing, checkpatch should warn you. > > > > Again, I may have been too hasty in sending out the patch :( > That is what the linux-kernel-mentees list is for. Send something out quickly and have some people with patience check it. Lukas _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees