[PATCH 00/11] checkpatch fixes

[PATCH 00/11] checkpatch fixes

[Andy Whitcroft]

After all the fun with kernel.org (and at least in part due to work
contraints) I have gotten behind on pushing out fixes for checkpatch.
Following this email are a bunch of patches I have had lying about for
some time.  There are a couple of optimisation for performance on some
outlying cases, plus a number of little fixes.

-apw

Andy Whitcroft (11):
  checkpatch: correctly track the end of preprocessor commands in context
  checkpatch: check for common memset parameter issues against statments
  checkpatch: ## is not a valid modifier
  checkpatch: optimise statement scanner when mid-statement
  checkpatch: only apply kconfig help checks for options which prompt
  checkpatch: fix EXPORT_SYMBOL handling following a function
  checkpatch: complex macro should allow the empty do while loop
  checkpatch: fix 'return is not a function' square bracket handling
  checkpatch: fix complex macros handling of square brackets
  checkpatch: ensure cast type is unique in the context parser
  checkpatch: typeof may have more complex arguments

 scripts/checkpatch.pl |  169 +++++++++++++++++++++++++++++--------------------
 1 files changed, 100 insertions(+), 69 deletions(-)

-- 
1.7.5.4

[PATCH 01/11] checkpatch: correctly track the end of preprocessor commands in context

[Andy Whitcroft]

When looking for a statement we currently run on through preprocessor
commands.  This means that a header file with just definitions is parsed
over and over again combining all of the lines from the current line to
the end of file leading to severe performance issues.

Fix up context accumulation to track preprocessor commands and stop when
reaching the end of them.  At the same time vastly simplify the #define
handling.

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |   90 +++++++++++++++++++++---------------------------
 1 files changed, 39 insertions(+), 51 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 8fda3b3..eac342d 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -676,6 +676,10 @@ sub ctx_statement_block {
 			if ($off >= $len) {
 				last;
 			}
+			if ($level == 0 && substr($blk, $off) =~ /^.\s*#\s*define/) {
+				$level++;
+				$type = '#';
+			}
 		}
 		$p = $c;
 		$c = substr($blk, $off, 1);
@@ -738,6 +742,13 @@ sub ctx_statement_block {
 				last;
 			}
 		}
+		# Preprocessor commands end at the newline unless escaped.
+		if ($type eq '#' && $c eq "\n" && $p ne "\\") {
+			$level--;
+			$type = '';
+			$off++;
+			last;
+		}
 		$off++;
 	}
 	# We are truly at the end, so shuffle to the next line.
@@ -1798,6 +1809,8 @@ sub process {
 			$stat =~ s/\n./\n /g;
 			$cond =~ s/\n./\n /g;
 
+#print "stat<$stat>\n";
+
 			# Find the real next line.
 			$realline_next = $line_nr_next;
 			if (defined $realline_next &&
@@ -2778,47 +2791,13 @@ sub process {
 			my $cnt = $realcnt;
 			my ($off, $dstat, $dcond, $rest);
 			my $ctx = '';
-
-			my $args = defined($1);
-
-			# Find the end of the macro and limit our statement
-			# search to that.
-			while ($cnt > 0 && defined $lines[$ln - 1] &&
-				$lines[$ln - 1] =~ /^(?:-|..*\\$)/)
-			{
-				$ctx .= $rawlines[$ln - 1] . "\n";
-				$cnt-- if ($lines[$ln - 1] !~ /^-/);
-				$ln++;
-			}
-			$ctx .= $rawlines[$ln - 1];
-
 			($dstat, $dcond, $ln, $cnt, $off) =
-				ctx_statement_block($linenr, $ln - $linenr + 1, 0);
+				ctx_statement_block($linenr, $realcnt, 0);
+			$ctx = $dstat;
 			#print "dstat<$dstat> dcond<$dcond> cnt<$cnt> off<$off>\n";
 			#print "LINE<$lines[$ln-1]> len<" . length($lines[$ln-1]) . "\n";
 
-			# Extract the remainder of the define (if any) and
-			# rip off surrounding spaces, and trailing \'s.
-			$rest = '';
-			while ($off != 0 || ($cnt > 0 && $rest =~ /\\\s*$/)) {
-				#print "ADDING cnt<$cnt> $off <" . substr($lines[$ln - 1], $off) . "> rest<$rest>\n";
-				if ($off != 0 || $lines[$ln - 1] !~ /^-/) {
-					$rest .= substr($lines[$ln - 1], $off) . "\n";
-					$cnt--;
-				}
-				$ln++;
-				$off = 0;
-			}
-			$rest =~ s/\\\n.//g;
-			$rest =~ s/^\s*//s;
-			$rest =~ s/\s*$//s;
-
-			# Clean up the original statement.
-			if ($args) {
-				substr($dstat, 0, length($dcond), '');
-			} else {
-				$dstat =~ s/^.\s*\#\s*define\s+$Ident\s*//;
-			}
+			$dstat =~ s/^.\s*\#\s*define\s+$Ident(?:\([^\)]*\))?\s*//;
 			$dstat =~ s/$;//g;
 			$dstat =~ s/\\\n.//g;
 			$dstat =~ s/^\s*//s;
@@ -2844,23 +2823,32 @@ sub process {
 				^\"|\"$
 			}x;
 			#print "REST<$rest> dstat<$dstat> ctx<$ctx>\n";
-			if ($rest ne '' && $rest ne ',') {
-				if ($rest !~ /while\s*\(/ &&
-				    $dstat !~ /$exceptions/)
-				{
-					ERROR("MULTISTATEMENT_MACRO_USE_DO_WHILE",
-					      "Macros with multiple statements should be enclosed in a do - while loop\n" . "$here\n$ctx\n");
+			if ($dstat ne '' &&
+			    $dstat !~ /^(?:$Ident|-?$Constant),$/ &&			# 10, // foo(),
+			    $dstat !~ /^(?:$Ident|-?$Constant);$/ &&			# foo();
+			    $dstat !~ /^(?:$Ident|-?$Constant)$/ &&			# 10 // foo()
+			    $dstat !~ /$exceptions/ &&
+			    $dstat !~ /^\.$Ident\s*=/ &&				# .foo =
+			    $dstat !~ /^do\s*$Constant\s*while\s*$Constant;$/ &&	# do {...} while (...);
+			    $dstat !~ /^for\s*$Constant$/ &&				# for (...)
+			    $dstat !~ /^for\s*$Constant\s+(?:$Ident|-?$Constant)$/ &&	# for (...) bar()
+			    $dstat !~ /^do\s*{/ &&					# do {...
+			    $dstat !~ /^\({/)						# ({...
+			{
+				$ctx =~ s/\n*$//;
+				my $herectx = $here . "\n";
+				my $cnt = statement_rawlines($ctx);
+
+				for (my $n = 0; $n < $cnt; $n++) {
+					$herectx .= raw_line($linenr, $n) . "\n";
 				}
 
-			} elsif ($ctx !~ /;/) {
-				if ($dstat ne '' &&
-				    $dstat !~ /^(?:$Ident|-?$Constant)$/ &&
-				    $dstat !~ /$exceptions/ &&
-				    $dstat !~ /^\.$Ident\s*=/ &&
-				    $dstat =~ /$Operators/)
-				{
+				if ($dstat =~ /;/) {
+					ERROR("MULTISTATEMENT_MACRO_USE_DO_WHILE",
+					      "Macros with multiple statements should be enclosed in a do - while loop\n" . "$herectx");
+				} else {
 					ERROR("COMPLEX_MACRO",
-					      "Macros with complex values should be enclosed in parenthesis\n" . "$here\n$ctx\n");
+					      "Macros with complex values should be enclosed in parenthesis\n" . "$here$ctx");
 				}
 			}
 		}
-- 
1.7.5.4

[PATCH 02/11] checkpatch: check for common memset parameter issues against statments

[Andy Whitcroft]

Move the memset checks over to work against the statement.  Also add
checks for 0 and 1 used as lengths.  Generally these indicate badly
ordered parameters.

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |   28 ++++++++++++++++++++++------
 1 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index eac342d..e07c36a 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3111,6 +3111,28 @@ sub process {
 			     "Avoid line continuations in quoted strings\n" . $herecurr);
 		}
 
+# Check for misused memsets
+		if (defined $stat && $stat =~ /\bmemset\s*\((.*)\)/s) {
+			my $args = $1;
+
+			# Flatten any parentheses and braces
+			while ($args =~ s/\([^\(\)]*\)/10/s ||
+			       $args =~ s/\{[^\{\}]*\}/10/s ||
+			       $args =~ s/\[[^\[\]]*\]/10/s)
+			{
+			}
+			# Extract the simplified arguments.
+			my ($ms_addr, $ms_val, $ms_size) =
+						split(/\s*,\s*/, $args);
+			if ($ms_size =~ /^(0x|)0$/i) {
+				ERROR("MEMSET",
+				      "memset size is 3rd argument, not the second.\n" . $herecurr);
+			} elsif ($ms_size =~ /^(0x|)1$/i) {
+				WARN("MEMSET",
+				     "single byte memset is suspicious. Swapped 2nd/3rd argument?\n" . $herecurr);
+			}
+		}
+
 # check for new externs in .c files.
 		if ($realfile =~ /\.c$/ && defined $stat &&
 		    $stat =~ /^.\s*(?:extern\s+)?$Type\s+($Ident)(\s*)\(/s)
@@ -3282,12 +3304,6 @@ sub process {
 			WARN("EXPORTED_WORLD_WRITABLE",
 			     "Exporting world writable files is usually an error. Consider more restrictive permissions.\n" . $herecurr);
 		}
-
-		# Check for memset with swapped arguments
-		if ($line =~ /memset.*\,(\ |)(0x|)0(\ |0|)\);/) {
-			ERROR("MEMSET",
-			      "memset size is 3rd argument, not the second.\n" . $herecurr);
-		}
 	}
 
 	# If we have no input at all, then there is nothing to report on
-- 
1.7.5.4

[PATCH 03/11] checkpatch: ## is not a valid modifier

[Andy Whitcroft]

Inserting a # into the modifiers list will incorrectly add the null string
to the modifiers list, leading to an infinite loop.  As neither of these
is a valid modifier form simply ignore them.

Reported-by: Joe Perches <joe@perches.com>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e07c36a..6dad5dd 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1223,7 +1223,9 @@ sub possible {
 			case|
 			else|
 			asm|__asm__|
-			do
+			do|
+			\#|
+			\#\#|
 		)(?:\s|$)|
 		^(?:typedef|struct|enum)\b
 	    )}x;
-- 
1.7.5.4

[PATCH 04/11] checkpatch: optimise statement scanner when mid-statement

[Andy Whitcroft]

In the middle of a long definition or similar, there is no possibility
of finding a smaller sub-statement.  Optimise this case by skipping
statement aquirey where there are no starts of statement (open brace
'{' or semi-colon ';').  We are likely to scan slightly more than needed
still but this is safest.

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |   22 ++++++++++++++++++++--
 1 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 6dad5dd..74d3d65 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1372,6 +1372,7 @@ sub process {
 	my %suppress_ifbraces;
 	my %suppress_whiletrailers;
 	my %suppress_export;
+	my $suppress_statement = 0;
 
 	# Pre-scan the patch sanitizing the lines.
 	# Pre-scan the patch looking for any __setup documentation.
@@ -1481,6 +1482,7 @@ sub process {
 			%suppress_ifbraces = ();
 			%suppress_whiletrailers = ();
 			%suppress_export = ();
+			$suppress_statement = 0;
 			next;
 
 # track the line number as we move through the hunk, note that
@@ -1805,13 +1807,23 @@ sub process {
 # Check for potential 'bare' types
 		my ($stat, $cond, $line_nr_next, $remain_next, $off_next,
 		    $realline_next);
-		if ($realcnt && $line =~ /.\s*\S/) {
+#print "LINE<$line>\n";
+		if ($linenr >= $suppress_statement &&
+		    $realcnt && $line =~ /.\s*\S/) {
 			($stat, $cond, $line_nr_next, $remain_next, $off_next) =
 				ctx_statement_block($linenr, $realcnt, 0);
 			$stat =~ s/\n./\n /g;
 			$cond =~ s/\n./\n /g;
 
-#print "stat<$stat>\n";
+#print "linenr<$linenr> <$stat>\n";
+			# If this statement has no statement boundaries within
+			# it there is no point in retrying a statement scan
+			# until we hit end of it.
+			my $frag = $stat; $frag =~ s/;+\s*$//;
+			if ($frag !~ /(?:{|;)/) {
+#print "skip<$line_nr_next>\n";
+				$suppress_statement = $line_nr_next;
+			}
 
 			# Find the real next line.
 			$realline_next = $line_nr_next;
@@ -1938,6 +1950,9 @@ sub process {
 
 # Check relative indent for conditionals and blocks.
 		if ($line =~ /\b(?:(?:if|while|for)\s*\(|do\b)/ && $line !~ /^.\s*#/ && $line !~ /\}\s*while\s*/) {
+			($stat, $cond, $line_nr_next, $remain_next, $off_next) =
+				ctx_statement_block($linenr, $realcnt, 0)
+					if (!defined $stat);
 			my ($s, $c) = ($stat, $cond);
 
 			substr($s, 0, length($c), '');
@@ -2638,6 +2653,9 @@ sub process {
 # Check for illegal assignment in if conditional -- and check for trailing
 # statements after the conditional.
 		if ($line =~ /do\s*(?!{)/) {
+			($stat, $cond, $line_nr_next, $remain_next, $off_next) =
+				ctx_statement_block($linenr, $realcnt, 0)
+					if (!defined $stat);
 			my ($stat_next) = ctx_statement_block($line_nr_next,
 						$remain_next, $off_next);
 			$stat_next =~ s/\n./\n /g;
-- 
1.7.5.4

[PATCH 05/11] checkpatch: only apply kconfig help checks for options which prompt

[Andy Whitcroft]

The intent of this check is to catch the options which the user will see
and ensure they are properly described.  It is also common for internal
only options to have a brief description.  Allow this form.

Reported-by: Steven Rostedt <rostedt@goodmis.org>
Tested-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 74d3d65..ca70bbf 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1670,19 +1670,26 @@ sub process {
 # Only applies when adding the entry originally, after that we do not have
 # sufficient context to determine whether it is indeed long enough.
 		if ($realfile =~ /Kconfig/ &&
-		    $line =~ /\+\s*(?:---)?help(?:---)?$/) {
+		    $line =~ /.\s*config\s+/) {
 			my $length = 0;
 			my $cnt = $realcnt;
 			my $ln = $linenr + 1;
 			my $f;
+			my $is_start = 0;
 			my $is_end = 0;
-			while ($cnt > 0 && defined $lines[$ln - 1]) {
+			for (; $cnt > 0 && defined $lines[$ln - 1]; $ln++) {
 				$f = $lines[$ln - 1];
 				$cnt-- if ($lines[$ln - 1] !~ /^-/);
 				$is_end = $lines[$ln - 1] =~ /^\+/;
-				$ln++;
 
 				next if ($f =~ /^-/);
+
+				if ($lines[$ln - 1] =~ /.\s*(?:bool|tristate)\s*\"/) {
+					$is_start = 1;
+				} elsif ($lines[$ln - 1] =~ /.\s*(?:---)?help(?:---)?$/) {
+					$length = -1;
+				}
+
 				$f =~ s/^.//;
 				$f =~ s/#.*//;
 				$f =~ s/^\s+//;
@@ -1694,8 +1701,8 @@ sub process {
 				$length++;
 			}
 			WARN("CONFIG_DESCRIPTION",
-			     "please write a paragraph that describes the config symbol fully\n" . $herecurr) if ($is_end && $length < 4);
-			#print "is_end<$is_end> length<$length>\n";
+			     "please write a paragraph that describes the config symbol fully\n" . $herecurr) if ($is_start && $is_end && $length < 4);
+			#print "is_start<$is_start> is_end<$is_end> length<$length>\n";
 		}
 
 		if (($realfile =~ /Makefile.*/ || $realfile =~ /Kbuild.*/) &&
-- 
1.7.5.4

[PATCH 06/11] checkpatch: fix EXPORT_SYMBOL handling following a function

[Andy Whitcroft]

The following fragment defeats the DEVICE_ATTR style handing, check for
and ignore the close brace '}' in this context:

    int foo()
    {
    }
    DEVICE_ATTR(link_power_management_policy, S_IRUGO | S_IWUSR,
                ata_scsi_lpm_show, ata_scsi_lpm_put);
    EXPORT_SYMBOL_GPL(dev_attr_link_power_management_policy);

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index ca70bbf..1ebfd17 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2127,7 +2127,7 @@ sub process {
 			#   XXX(foo);
 			#   EXPORT_SYMBOL(something_foo);
 			my $name = $1;
-			if ($stat =~ /^.([A-Z_]+)\s*\(\s*($Ident)/ &&
+			if ($stat =~ /^(?:.\s*}\s*\n)?.([A-Z_]+)\s*\(\s*($Ident)/ &&
 			    $name =~ /^${Ident}_$2/) {
 #print "FOO C name<$name>\n";
 				$suppress_export{$realline_next} = 1;
-- 
1.7.5.4

[PATCH 07/11] checkpatch: complex macro should allow the empty do while loop

[Andy Whitcroft]

It is common to stub out a function as below, this is triggering a
complex macro format incorrectly.  Sort this out:

    #define cma_early_regions_reserve(reserve)   do { } while (0)

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 1ebfd17..4d1069a 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2856,7 +2856,7 @@ sub process {
 			    $dstat !~ /^(?:$Ident|-?$Constant)$/ &&			# 10 // foo()
 			    $dstat !~ /$exceptions/ &&
 			    $dstat !~ /^\.$Ident\s*=/ &&				# .foo =
-			    $dstat !~ /^do\s*$Constant\s*while\s*$Constant;$/ &&	# do {...} while (...);
+			    $dstat !~ /^do\s*$Constant\s*while\s*$Constant;?$/ &&	# do {...} while (...); // do {...} while (...)
 			    $dstat !~ /^for\s*$Constant$/ &&				# for (...)
 			    $dstat !~ /^for\s*$Constant\s+(?:$Ident|-?$Constant)$/ &&	# for (...) bar()
 			    $dstat !~ /^do\s*{/ &&					# do {...
-- 
1.7.5.4

[PATCH 08/11] checkpatch: fix 'return is not a function' square bracket handling

[Andy Whitcroft]

We are incorrectly matching square brackets '[' and ']' leading to false
positives on more complex functions as below:

    return (dt3155_fbuffer[m]->ready_head -
	dt3155_fbuffer[m]->ready_len +
	dt3155_fbuffer[m]->nbuffers)%
	(dt3155_fbuffer[m]->nbuffers);

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4d1069a..52507f6 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2605,7 +2605,7 @@ sub process {
 			# Flatten any parentheses
 			$value =~ s/\(/ \(/g;
 			$value =~ s/\)/\) /g;
-			while ($value =~ s/\[[^\{\}]*\]/1/ ||
+			while ($value =~ s/\[[^\[\]]*\]/1/ ||
 			       $value !~ /(?:$Ident|-?$Constant)\s*
 					     $Compare\s*
 					     (?:$Ident|-?$Constant)/x &&
-- 
1.7.5.4

[PATCH 09/11] checkpatch: fix complex macros handling of square brackets

[Andy Whitcroft]

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 52507f6..71655dc 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2833,7 +2833,7 @@ sub process {
 			# Flatten any parentheses and braces
 			while ($dstat =~ s/\([^\(\)]*\)/1/ ||
 			       $dstat =~ s/\{[^\{\}]*\}/1/ ||
-			       $dstat =~ s/\[[^\{\}]*\]/1/)
+			       $dstat =~ s/\[[^\[\]]*\]/1/)
 			{
 			}
 
-- 
1.7.5.4

[PATCH 10/11] checkpatch: ensure cast type is unique in the context parser

[Andy Whitcroft]

Ensure the cast type is unique in the context parser, we do not want
them to detect as a comma ','.

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 71655dc..1c341be 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1031,7 +1031,7 @@ sub annotate_values {
 		} elsif ($cur =~ /^(\(\s*$Type\s*)\)/ && $av_pending eq '_') {
 			print "CAST($1)\n" if ($dbg_values > 1);
 			push(@av_paren_type, $type);
-			$type = 'C';
+			$type = 'c';
 
 		} elsif ($cur =~ /^($Type)\s*(?:$Ident|,|\)|\(|\s*$)/) {
 			print "DECLARE($1)\n" if ($dbg_values > 1);
-- 
1.7.5.4

[PATCH 11/11] checkpatch: typeof may have more complex arguments

[Andy Whitcroft]

typeof may have various more complex forms as its arguement, not just an
identifier.  For now allow us to leak to the first close perenthesis ')'.

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 1c341be..fb20de4 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -315,7 +315,7 @@ sub build_types {
 	$NonptrType	= qr{
 			(?:$Modifier\s+|const\s+)*
 			(?:
-				(?:typeof|__typeof__)\s*\(\s*\**\s*$Ident\s*\)|
+				(?:typeof|__typeof__)\s*\([^\)]*\)|
 				(?:$typeTypedefs\b)|
 				(?:${all}\b)
 			)
-- 
1.7.5.4

Re: [PATCH 02/11] checkpatch: check for common memset parameter issues against statments

[Joe Perches]

On Mon, 2011-11-28 at 15:42 +0000, Andy Whitcroft wrote:
> Move the memset checks over to work against the statement.  Also add
> checks for 0 and 1 used as lengths.  Generally these indicate badly
> ordered parameters.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -3111,6 +3111,28 @@ sub process {
>  			     "Avoid line continuations in quoted strings\n" . $herecurr);
>  		}
>  
> +# Check for misused memsets
> +		if (defined $stat && $stat =~ /\bmemset\s*\((.*)\)/s) {

Perhaps this is better.
It allows close parenthesis in the memset.
eg:	memset(foo, bar(baz), sizeof(foo));

	if (defined $stat && $stat = /\bmemset\s*$match_balanced_parentheses/s) {

[PATCH] checkpatch: Improve memset and min/max with cast checking

[Joe Perches]

Improve the checking of arguments to memset and min/max tests.

Move the checking of min/max to statement blocks instead of single line.
Change $Constant to allow any case type 0x initiator and trailing ul specifier.
Add $FuncArg type as any function argument with or without a cast.
Print the whole statement when showing memset or min/max messages.
Improve the memset with 0 as 3rd argument error message.

There are still weaknesses in the $FuncArg and $Constant code as
arbitrary parentheses and negative signs are not generically supported.

Signed-off-by: Joe Perches <joe@perches.com>
---

On top of Andy's 0/11 series.

 scripts/checkpatch.pl |   69 +++++++++++++++++++++++-------------------------
 1 files changed, 33 insertions(+), 36 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 59435a9..d72a71e 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -227,7 +227,7 @@ our $Inline	= qr{inline|__always_inline|noinline};
 our $Member	= qr{->$Ident|\.$Ident|\[[^]]*\]};
 our $Lval	= qr{$Ident(?:$Member)*};
 
-our $Constant	= qr{(?:[0-9]+|0x[0-9a-fA-F]+)[UL]*};
+our $Constant	= qr{(?i:[0-9]+|0x[0-9a-f]+)[ul]*};
 our $Assignment	= qr{(?:\*\=|/=|%=|\+=|-=|<<=|>>=|&=|\^=|\|=|=)};
 our $Compare    = qr{<=|>=|==|!=|<|>};
 our $Operators	= qr{
@@ -334,6 +334,7 @@ our $match_balanced_parentheses = qr/(\((?:[^\(\)]+|(-1))*\))/;
 
 our $Typecast	= qr{\s*(\(\s*$NonptrType\s*\)){0,1}\s*};
 our $LvalOrFunc	= qr{($Lval)\s*($match_balanced_parentheses{0,1})\s*};
+our $FuncArg = qr{$Typecast{0,1}($LvalOrFunc|$Constant)};
 
 sub deparenthesize {
 	my ($string) = @_;
@@ -2633,28 +2634,6 @@ sub process {
 			}
 		}
 
-# typecasts on min/max could be min_t/max_t
-		if ($line =~ /^\+(?:.*?)\b(min|max)\s*\($Typecast{0,1}($LvalOrFunc)\s*,\s*$Typecast{0,1}($LvalOrFunc)\s*\)/) {
-			if (defined $2 || defined $8) {
-				my $call = $1;
-				my $cast1 = deparenthesize($2);
-				my $arg1 = $3;
-				my $cast2 = deparenthesize($8);
-				my $arg2 = $9;
-				my $cast;
-
-				if ($cast1 ne "" && $cast2 ne "") {
-					$cast = "$cast1 or $cast2";
-				} elsif ($cast1 ne "") {
-					$cast = $cast1;
-				} else {
-					$cast = $cast2;
-				}
-				WARN("MINMAX",
-				     "$call() should probably be ${call}_t($cast, $arg1, $arg2)\n" . $herecurr);
-			}
-		}
-
 # Need a space before open parenthesis after if, while etc
 		if ($line=~/\b(if|while|for|switch)\(/) {
 			ERROR("SPACING", "space required before the open parenthesis '('\n" . $herecurr);
@@ -3148,24 +3127,42 @@ sub process {
 		}
 
 # Check for misused memsets
-		if (defined $stat && $stat =~ /\bmemset\s*\((.*)\)/s) {
-			my $args = $1;
+		if (defined $stat &&
+		    $stat =~ /^\+(?:.*?)\bmemset\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\,\s*$FuncArg\s*\)/s) {
+
+			my $ms_addr = $2;
+			my $ms_val = $8;
+			my $ms_size = $14;
 
-			# Flatten any parentheses and braces
-			while ($args =~ s/\([^\(\)]*\)/10/s ||
-			       $args =~ s/\{[^\{\}]*\}/10/s ||
-			       $args =~ s/\[[^\[\]]*\]/10/s)
-			{
-			}
-			# Extract the simplified arguments.
-			my ($ms_addr, $ms_val, $ms_size) =
-						split(/\s*,\s*/, $args);
 			if ($ms_size =~ /^(0x|)0$/i) {
 				ERROR("MEMSET",
-				      "memset size is 3rd argument, not the second.\n" . $herecurr);
+				      "memset to 0's uses 0 as the 2nd argument, not the 3rd\n" . "$here\n$stat\n");
 			} elsif ($ms_size =~ /^(0x|)1$/i) {
 				WARN("MEMSET",
-				     "single byte memset is suspicious. Swapped 2nd/3rd argument?\n" . $herecurr);
+				     "single byte memset is suspicious. Swapped 2nd/3rd argument?\n" . "$here\n$stat\n");
+			}
+		}
+
+# typecasts on min/max could be min_t/max_t
+		if (defined $stat &&
+		    $stat =~ /^\+(?:.*?)\b(min|max)\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\)/) {
+			if (defined $2 || defined $8) {
+				my $call = $1;
+				my $cast1 = deparenthesize($2);
+				my $arg1 = $3;
+				my $cast2 = deparenthesize($8);
+				my $arg2 = $9;
+				my $cast;
+
+				if ($cast1 ne "" && $cast2 ne "") {
+					$cast = "$cast1 or $cast2";
+				} elsif ($cast1 ne "") {
+					$cast = $cast1;
+				} else {
+					$cast = $cast2;
+				}
+				WARN("MINMAX",
+				     "$call() should probably be ${call}_t($cast, $arg1, $arg2)\n" . "$here\n$stat\n");
 			}
 		}
 

Re: [PATCH] checkpatch: Improve memset and min/max with cast checking

[Andy Whitcroft]

On Mon, Nov 28, 2011 at 07:24:30PM -0800, Joe Perches wrote:
> Improve the checking of arguments to memset and min/max tests.
> 
> Move the checking of min/max to statement blocks instead of single line.
> Change $Constant to allow any case type 0x initiator and trailing ul specifier.
> Add $FuncArg type as any function argument with or without a cast.
> Print the whole statement when showing memset or min/max messages.
> Improve the memset with 0 as 3rd argument error message.
> 
> There are still weaknesses in the $FuncArg and $Constant code as
> arbitrary parentheses and negative signs are not generically supported.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
> 
> On top of Andy's 0/11 series.
> 
>  scripts/checkpatch.pl |   69 +++++++++++++++++++++++-------------------------
>  1 files changed, 33 insertions(+), 36 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 59435a9..d72a71e 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -227,7 +227,7 @@ our $Inline	= qr{inline|__always_inline|noinline};
>  our $Member	= qr{->$Ident|\.$Ident|\[[^]]*\]};
>  our $Lval	= qr{$Ident(?:$Member)*};
>  
> -our $Constant	= qr{(?:[0-9]+|0x[0-9a-fA-F]+)[UL]*};
> +our $Constant	= qr{(?i:[0-9]+|0x[0-9a-f]+)[ul]*};

This should actually be as below so that the UL is included in the case
insensitive match:

our $Constant  = qr{(?i:(?:[0-9]+|0x[0-9a-f]+)[ul]*)};

Other than that it looks good passing my testing.

Acked-by: Andy Whitcroft <apw@canonical.com>

Applied to my next branch.

>  our $Assignment	= qr{(?:\*\=|/=|%=|\+=|-=|<<=|>>=|&=|\^=|\|=|=)};
>  our $Compare    = qr{<=|>=|==|!=|<|>};
>  our $Operators	= qr{
> @@ -334,6 +334,7 @@ our $match_balanced_parentheses = qr/(\((?:[^\(\)]+|(-1))*\))/;
>  
>  our $Typecast	= qr{\s*(\(\s*$NonptrType\s*\)){0,1}\s*};
>  our $LvalOrFunc	= qr{($Lval)\s*($match_balanced_parentheses{0,1})\s*};
> +our $FuncArg = qr{$Typecast{0,1}($LvalOrFunc|$Constant)};
>  
>  sub deparenthesize {
>  	my ($string) = @_;
> @@ -2633,28 +2634,6 @@ sub process {
>  			}
>  		}
>  
> -# typecasts on min/max could be min_t/max_t
> -		if ($line =~ /^\+(?:.*?)\b(min|max)\s*\($Typecast{0,1}($LvalOrFunc)\s*,\s*$Typecast{0,1}($LvalOrFunc)\s*\)/) {
> -			if (defined $2 || defined $8) {
> -				my $call = $1;
> -				my $cast1 = deparenthesize($2);
> -				my $arg1 = $3;
> -				my $cast2 = deparenthesize($8);
> -				my $arg2 = $9;
> -				my $cast;
> -
> -				if ($cast1 ne "" && $cast2 ne "") {
> -					$cast = "$cast1 or $cast2";
> -				} elsif ($cast1 ne "") {
> -					$cast = $cast1;
> -				} else {
> -					$cast = $cast2;
> -				}
> -				WARN("MINMAX",
> -				     "$call() should probably be ${call}_t($cast, $arg1, $arg2)\n" . $herecurr);
> -			}
> -		}
> -
>  # Need a space before open parenthesis after if, while etc
>  		if ($line=~/\b(if|while|for|switch)\(/) {
>  			ERROR("SPACING", "space required before the open parenthesis '('\n" . $herecurr);
> @@ -3148,24 +3127,42 @@ sub process {
>  		}
>  
>  # Check for misused memsets
> -		if (defined $stat && $stat =~ /\bmemset\s*\((.*)\)/s) {
> -			my $args = $1;
> +		if (defined $stat &&
> +		    $stat =~ /^\+(?:.*?)\bmemset\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\,\s*$FuncArg\s*\)/s) {
> +
> +			my $ms_addr = $2;
> +			my $ms_val = $8;
> +			my $ms_size = $14;
>  
> -			# Flatten any parentheses and braces
> -			while ($args =~ s/\([^\(\)]*\)/10/s ||
> -			       $args =~ s/\{[^\{\}]*\}/10/s ||
> -			       $args =~ s/\[[^\[\]]*\]/10/s)
> -			{
> -			}
> -			# Extract the simplified arguments.
> -			my ($ms_addr, $ms_val, $ms_size) =
> -						split(/\s*,\s*/, $args);
>  			if ($ms_size =~ /^(0x|)0$/i) {
>  				ERROR("MEMSET",
> -				      "memset size is 3rd argument, not the second.\n" . $herecurr);
> +				      "memset to 0's uses 0 as the 2nd argument, not the 3rd\n" . "$here\n$stat\n");
>  			} elsif ($ms_size =~ /^(0x|)1$/i) {
>  				WARN("MEMSET",
> -				     "single byte memset is suspicious. Swapped 2nd/3rd argument?\n" . $herecurr);
> +				     "single byte memset is suspicious. Swapped 2nd/3rd argument?\n" . "$here\n$stat\n");
> +			}
> +		}
> +
> +# typecasts on min/max could be min_t/max_t
> +		if (defined $stat &&
> +		    $stat =~ /^\+(?:.*?)\b(min|max)\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\)/) {
> +			if (defined $2 || defined $8) {
> +				my $call = $1;
> +				my $cast1 = deparenthesize($2);
> +				my $arg1 = $3;
> +				my $cast2 = deparenthesize($8);
> +				my $arg2 = $9;
> +				my $cast;
> +
> +				if ($cast1 ne "" && $cast2 ne "") {
> +					$cast = "$cast1 or $cast2";
> +				} elsif ($cast1 ne "") {
> +					$cast = $cast1;
> +				} else {
> +					$cast = $cast2;
> +				}
> +				WARN("MINMAX",
> +				     "$call() should probably be ${call}_t($cast, $arg1, $arg2)\n" . "$here\n$stat\n");
>  			}
>  		}

-apw

Re: [PATCH] checkpatch: Improve memset and min/max with cast checking

[Andy Whitcroft]

On Tue, Nov 29, 2011 at 09:28:51PM +0000, Andy Whitcroft wrote:

> Applied to my next branch.

Now that kernel.org is back you can see what I have pending
as below:

    git://git.kernel.org/pub/scm/linux/kernel/git/apw/checkpatch.git next

The master branch in the same repo represents what has been submitted to
akpm.

The latest tip is normally exported here for testing:

    http://people.canonical.com/~apw/checkpatch/checkpatch-next.pl

-apw