* [PATCH 1/3] checkpatch: ctx_statement_block: Fix preprocessor guard tracking
2021-03-26 3:50 [PATCH 0/3] Detect suspicious indentation after conditional Julius Werner
@ 2021-03-26 3:50 ` Julius Werner
2021-03-26 3:50 ` [PATCH 2/3] Suspicious indentation detection after conditional statement Julius Werner
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Julius Werner @ 2021-03-26 3:50 UTC (permalink / raw)
To: Andy Whitcroft, Joe Perches; +Cc: Ivo Sieben, linux-kernel, Julius Werner
The preprocessor guard tracking in ctx_statement_block() is (and seems
to have always been) subtly broken whenever tracking over an #else: the
code is supposed to restore state from the current top of the stack
(like and #endif just without removing it). However, it indexes the
stack at [$#stack - 1]. In Perl, $# does not give you the length of an
array, it gives you the highest valid index. Therefore, the correct
index should just be [$#stack].
The preprocessor guard tracking also gets confused when
ctx_statement_block() was called on a line that is already inside a
preprocessor guard, and steps out of it within the same statement. This
happens commonly with constructs like this:
#if CONFIG_XXX
for (a = first_a(); !a_finished(); a = next_a()) {
#else
for (b = first_b(); !b_finished(); b = next_b()) {
#endif
... loop body ...
The best course of action in this case is to not try to restore any
previous state (which we don't have) at all, so we should just keep our
current state if $#stack is already 0.
Signed-off-by: Julius Werner <jwerner@chromium.org>
---
scripts/checkpatch.pl | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index df8b23dc1eb0af..ffccbd2033e579 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1572,9 +1572,9 @@ sub ctx_statement_block {
# Handle nested #if/#else.
if ($remainder =~ /^#\s*(?:ifndef|ifdef|if)\s/) {
push(@stack, [ $type, $level ]);
- } elsif ($remainder =~ /^#\s*(?:else|elif)\b/) {
- ($type, $level) = @{$stack[$#stack - 1]};
- } elsif ($remainder =~ /^#\s*endif\b/) {
+ } elsif ($remainder =~ /^#\s*(?:else|elif)\b/ && $#stack > 0) {
+ ($type, $level) = @{$stack[$#stack]};
+ } elsif ($remainder =~ /^#\s*endif\b/ && $#stack > 0) {
($type, $level) = @{pop(@stack)};
}
--
2.29.2
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 2/3] Suspicious indentation detection after conditional statement
2021-03-26 3:50 [PATCH 0/3] Detect suspicious indentation after conditional Julius Werner
2021-03-26 3:50 ` [PATCH 1/3] checkpatch: ctx_statement_block: Fix preprocessor guard tracking Julius Werner
@ 2021-03-26 3:50 ` Julius Werner
2021-03-26 3:50 ` [PATCH 3/3] checkpatch: Ignore labels when checking indentation Julius Werner
2021-04-14 21:18 ` [PATCH 0/3] Detect suspicious indentation after conditional Julius Werner
3 siblings, 0 replies; 7+ messages in thread
From: Julius Werner @ 2021-03-26 3:50 UTC (permalink / raw)
To: Andy Whitcroft, Joe Perches; +Cc: Ivo Sieben, linux-kernel
From: Ivo Sieben <meltedpianoman@gmail.com>
Raise a SUSPICIOUS_CODE_INDENT warning when unexpected indentation is found
after a conditional statement. This can be used to find missing braces or
wrong indentation in/after a conditional statement.
For example the following error is caught;
if (foo)
bar();
return;
Which can be either intended by the programmer as:
if (foo)
bar();
return;
or
if (foo) {
bar();
return;
}
Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com>
---
scripts/checkpatch.pl | 41 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 41 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index ffccbd2033e579..c1dfc0107be41d 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4185,6 +4185,47 @@ sub process {
WARN("SUSPECT_CODE_INDENT",
"suspect code indent for conditional statements ($indent, $sindent)\n" . $herecurr . "$stat_real\n");
}
+
+# Also check if the next statement after the previous condition has the same indent
+ my ($stat_next, undef, $line_nr_next_next) =
+ ctx_statement_block($line_nr_next, $remain_next, $off_next);
+ my $s_next = $stat_next;
+
+ # Remove line prefixes
+ $s_next =~ s/\n./\n/gs;
+
+ # Remove any comments
+ $s_next =~ s/$;//g;
+
+ # Skip this check for in case next statement starts with 'else'
+ if ($s_next !~ /\s*\belse\b/) {
+
+ # Remove while that belongs to a do {} while
+ if ($stat =~ /\bdo\b/) {
+ $s_next =~ s/^.*\bwhile\b\s*($balanced_parens)\s*?//;
+ }
+
+ # Remove blank lines
+ $s_next =~ s/\s*\\?\n//g;
+
+ # Get the real next lines
+ my $next_nof_lines = $line_nr_next_next - $line_nr_next;
+ my $stat_next_real = raw_line($line_nr_next, $next_nof_lines);
+ if (!defined($stat_next_real)) {
+ $stat_next_real = "";
+ } elsif ($next_nof_lines > 1) {
+ $stat_next_real = "[...]\n$stat_next_real";
+ }
+ my (undef, $nindent) = line_stats('+' . $s_next);
+
+ #print "stat_next<$stat_next> stat<$stat> indent<$indent> nindent<$nindent> s_next<$s_next> stat_next_real<$stat_next_real>\n";
+
+ if ($nindent > $indent) {
+ WARN("SUSPICIOUS_CODE_INDENT",
+ "suspicious code indentation after conditional statements\n" .
+ $herecurr . "$stat_real\n$stat_next_real\n");
+ }
+ }
}
# Track the 'values' across context and added lines.
--
2.29.2
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 3/3] checkpatch: Ignore labels when checking indentation
2021-03-26 3:50 [PATCH 0/3] Detect suspicious indentation after conditional Julius Werner
2021-03-26 3:50 ` [PATCH 1/3] checkpatch: ctx_statement_block: Fix preprocessor guard tracking Julius Werner
2021-03-26 3:50 ` [PATCH 2/3] Suspicious indentation detection after conditional statement Julius Werner
@ 2021-03-26 3:50 ` Julius Werner
2021-04-14 21:18 ` [PATCH 0/3] Detect suspicious indentation after conditional Julius Werner
3 siblings, 0 replies; 7+ messages in thread
From: Julius Werner @ 2021-03-26 3:50 UTC (permalink / raw)
To: Andy Whitcroft, Joe Perches; +Cc: Ivo Sieben, linux-kernel, Julius Werner
Goto labels are commonly written in the leftmost column (sometimes with
one space in front), regardless of indentation level. Sometimes they're
on a line of their own, but sometimes the same line is shared with a
normal code statement that then starts at the expected indentation
level. When checking indentation, we should check where that normal
piece of code starts, not where the label starts (there's a separate
INDENTED_LABEL test to check the label itself). Therefore, the
line_stats() function that is used to get indentation level should treat
goto labels like whitespace. The SUSPICIOUS_CODE_INDENT test also needs
to explicitly ignore labels to make sure it doesn't get confused by
them.
Signed-off-by: Julius Werner <jwerner@chromium.org>
---
scripts/checkpatch.pl | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index c1dfc0107be41d..d89367a59e7d37 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1396,8 +1396,12 @@ sub copy_spacing {
sub line_stats {
my ($line) = @_;
- # Drop the diff line leader and expand tabs
+ # Drop the diff line leader
$line =~ s/^.//;
+
+ # Treat labels like whitespace when counting indentation
+ $line =~ s/^( ?$Ident:)/" " x length($1)/e;
+
$line = expand_tabs($line);
# Pick the indent from the front of the line.
@@ -4197,6 +4201,9 @@ sub process {
# Remove any comments
$s_next =~ s/$;//g;
+ # Remove any leading labels
+ $s_next =~ s/\n( ?$Ident:)/"\n" . " " x length($1)/eg;
+
# Skip this check for in case next statement starts with 'else'
if ($s_next !~ /\s*\belse\b/) {
--
2.29.2
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 0/3] Detect suspicious indentation after conditional
2021-03-26 3:50 [PATCH 0/3] Detect suspicious indentation after conditional Julius Werner
` (2 preceding siblings ...)
2021-03-26 3:50 ` [PATCH 3/3] checkpatch: Ignore labels when checking indentation Julius Werner
@ 2021-04-14 21:18 ` Julius Werner
2021-04-15 23:55 ` Joe Perches
3 siblings, 1 reply; 7+ messages in thread
From: Julius Werner @ 2021-04-14 21:18 UTC (permalink / raw)
To: Julius Werner; +Cc: Andy Whitcroft, Joe Perches, Ivo Sieben, LKML
*friendly ping*
Hi Andy, Joe,
Any comments on this patch series? Are you guys the right point of
contact for checkpatch changes?
On Thu, Mar 25, 2021 at 8:50 PM Julius Werner <jwerner@chromium.org> wrote:
>
> This patch series is adding functionality to checkpatch.pl to test for
> incorrect code indentation after a conditional statement, like this:
>
> if (a)
> b;
> c;
>
> (Indentation implies that `c;` was guarded by the conditional, but it
> isn't.) The main part is re-sending a patch from Ivo Sieben that was
> already proposed in 2014 [1]. I don't know why it was never merged --
> it seems that there was no discussion on it. I hope that it was only
> overlooked, because it works great, and I think this is a very important
> class of common error to catch.
>
> I have tested it extensively on the kernel tree and in the course of
> that found a few more edge cases that get fixed by the other two
> patches. With all these applied, the vast majority of hits I get from
> this check on the kernel tree are actual indentation errors or other
> code style violations (e.g. case label and statement on the same line).
> The only significant remaining group of false positives I found are
> cases of macros being defined within a function, which are overall very
> rare. I think the benefit of adding this check would far outweigh the
> remaining amount of noise.
>
> [1]: https://lore.kernel.org/patchwork/patch/465116
>
> Ivo Sieben (1):
> Suspicious indentation detection after conditional statement
>
> Julius Werner (2):
> checkpatch: ctx_statement_block: Fix preprocessor guard tracking
> checkpatch: Ignore labels when checking indentation
>
> scripts/checkpatch.pl | 56 +++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 52 insertions(+), 4 deletions(-)
>
> --
> 2.29.2
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 0/3] Detect suspicious indentation after conditional
2021-04-14 21:18 ` [PATCH 0/3] Detect suspicious indentation after conditional Julius Werner
@ 2021-04-15 23:55 ` Joe Perches
2021-04-30 21:12 ` Julius Werner
0 siblings, 1 reply; 7+ messages in thread
From: Joe Perches @ 2021-04-15 23:55 UTC (permalink / raw)
To: Julius Werner; +Cc: Andy Whitcroft, Ivo Sieben, LKML
On Wed, 2021-04-14 at 14:18 -0700, Julius Werner wrote:
> *friendly ping*
>
> Hi Andy, Joe,
>
> Any comments on this patch series? Are you guys the right point of
> contact for checkpatch changes?
I don't have any issue with this patch set, but Andy is really
the person that should approve any changes to this block of code.
> On Thu, Mar 25, 2021 at 8:50 PM Julius Werner <jwerner@chromium.org> wrote:
> >
> > This patch series is adding functionality to checkpatch.pl to test for
> > incorrect code indentation after a conditional statement, like this:
> >
> > if (a)
> > b;
> > c;
> >
> > (Indentation implies that `c;` was guarded by the conditional, but it
> > isn't.) The main part is re-sending a patch from Ivo Sieben that was
> > already proposed in 2014 [1]. I don't know why it was never merged --
> > it seems that there was no discussion on it. I hope that it was only
> > overlooked, because it works great, and I think this is a very important
> > class of common error to catch.
> >
> > I have tested it extensively on the kernel tree and in the course of
> > that found a few more edge cases that get fixed by the other two
> > patches. With all these applied, the vast majority of hits I get from
> > this check on the kernel tree are actual indentation errors or other
> > code style violations (e.g. case label and statement on the same line).
> > The only significant remaining group of false positives I found are
> > cases of macros being defined within a function, which are overall very
> > rare. I think the benefit of adding this check would far outweigh the
> > remaining amount of noise.
> >
> > [1]: https://lore.kernel.org/patchwork/patch/465116
> >
> > Ivo Sieben (1):
> > Suspicious indentation detection after conditional statement
> >
> > Julius Werner (2):
> > checkpatch: ctx_statement_block: Fix preprocessor guard tracking
> > checkpatch: Ignore labels when checking indentation
> >
> > scripts/checkpatch.pl | 56 +++++++++++++++++++++++++++++++++++++++----
> > 1 file changed, 52 insertions(+), 4 deletions(-)
> >
> > --
> > 2.29.2
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 0/3] Detect suspicious indentation after conditional
2021-04-15 23:55 ` Joe Perches
@ 2021-04-30 21:12 ` Julius Werner
0 siblings, 0 replies; 7+ messages in thread
From: Julius Werner @ 2021-04-30 21:12 UTC (permalink / raw)
To: Joe Perches; +Cc: Julius Werner, Andy Whitcroft, Ivo Sieben, LKML
*another friendly ping*
Hi Andy, any comments?
Joe, if Andy doesn't have time to look at this anymore at the moment
(if I'm looking for this right I think the last mail from him I can
find on LKML was in 2019?), is there anything else I can do to
convince you to take this series?
^ permalink raw reply [flat|nested] 7+ messages in thread