Occasionally someone goofs the argument order to memset.
This patch makes checkpatch catch those.
I made memset with size of 0 an error, because it's never correct,
whereas memset with a size of 1 isn't technically an incorrect
thing to do so I left it as a warning. It may still be better to replace
it with a single variable assignment in the false positive cases.
Signed-off-by: Dave Jones <[email protected]>
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2039acd..3690173 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2794,6 +2794,14 @@ sub process {
}
}
+# check for memset with reversed 2nd/3rd arguments.
+ if ($line =~ /memset.*\,(\ |)(0x|)0(\ |0|)\);/) {
+ ERROR("memset takes size as third argument, not the second.\n" . $herecurr);
+ }
+ if ($line =~ /memset.*\,(\ |)(0x|)1\);/) {
+ WARN("single byte memset is suspicious. Swapped 2nd/3rd argument?\n" . $herecurr);
+ }
+
# check for lockdep_set_novalidate_class
if ($line =~ /^.\s*lockdep_set_novalidate_class\s*\(/ ||
$line =~ /__lockdep_no_validate__\s*\)/ ) {
On Wed, 2010-08-18 at 16:40 -0400, Dave Jones wrote:
> Occasionally someone goofs the argument order to memset.
> This patch makes checkpatch catch those.
>
> I made memset with size of 0 an error, because it's never correct,
> whereas memset with a size of 1 isn't technically an incorrect
> thing to do so I left it as a warning. It may still be better to replace
> it with a single variable assignment in the false positive cases.
Maybe something like allows more flexible checking?
if ($line =~ /\bmemset\s*\(\s*($Lval)\s*,\s*($Lval)\s*,\s*($Lval)\s*\)/) {
my $memset_addr = $1;
my $memset_val = $2;
my $memset_size = $3;
if ($memset_size =~ /(0x|)0$/i) {
ERROR("memset uses second argument as constant byte value, not third.\n" . $herecurr);
elsif ($memset_size =~ /(0x|)1/i) {
WARN("single byte memset is suspicious. Swapped 2nd/3rd argument?\n" . $herecurr);
}
}
On Wed, Aug 18, 2010 at 02:32:13PM -0700, Joe Perches wrote:
> On Wed, 2010-08-18 at 16:40 -0400, Dave Jones wrote:
> > Occasionally someone goofs the argument order to memset.
> > This patch makes checkpatch catch those.
> >
> > I made memset with size of 0 an error, because it's never correct,
> > whereas memset with a size of 1 isn't technically an incorrect
> > thing to do so I left it as a warning. It may still be better to replace
> > it with a single variable assignment in the false positive cases.
>
> Maybe something like allows more flexible checking?
>
> if ($line =~ /\bmemset\s*\(\s*($Lval)\s*,\s*($Lval)\s*,\s*($Lval)\s*\)/) {
> my $memset_addr = $1;
> my $memset_val = $2;
> my $memset_size = $3;
> if ($memset_size =~ /(0x|)0$/i) {
> ERROR("memset uses second argument as constant byte value, not third.\n" . $herecurr);
> elsif ($memset_size =~ /(0x|)1/i) {
> WARN("single byte memset is suspicious. Swapped 2nd/3rd argument?\n" . $herecurr);
> }
> }
I'm all for improving my shoddy perl where possible, but this doesn't seem to actually
catch any of the test cases I wrote. (it's also missing a } )
Dave
On Wed, 2010-08-18 at 17:48 -0400, Dave Jones wrote:
> On Wed, Aug 18, 2010 at 02:32:13PM -0700, Joe Perches wrote:
> > On Wed, 2010-08-18 at 16:40 -0400, Dave Jones wrote:
> > > Occasionally someone goofs the argument order to memset.
> > if ($line =~ /\bmemset\s*\(\s*($Lval)\s*,\s*($Lval)\s*,\s*($Lval)\s*\)/) {
> > my $memset_addr = $1;
> > my $memset_val = $2;
> > my $memset_size = $3;
> > if ($memset_size =~ /(0x|)0$/i) {
> > ERROR("memset uses second argument as constant byte value, not third.\n" . $herecurr);
> > elsif ($memset_size =~ /(0x|)1/i) {
> > WARN("single byte memset is suspicious. Swapped 2nd/3rd argument?\n" . $herecurr);
> > }
> > }
> I'm all for improving my shoddy perl where possible, but this doesn't seem to actually
> catch any of the test cases I wrote. (it's also missing a } )
I intend never to be a perl monk.
I notice the missing { before the elsif after I sent it.
Oh well.
I just typed it in the emailer, so it's not tested at all.
Also it's missing a $ after 1 in the second $memset_size test.
What are your test cases anyway?
Likely $Lval isn't matching things like
sizeof(*foo) so this isn't checked:
memset(foo, bar, sizeof(*foo));
Maybe Andy has a better idea?
On Wed, Aug 18, 2010 at 02:57:59PM -0700, Joe Perches wrote:
> > I'm all for improving my shoddy perl where possible, but this doesn't seem to actually
> > catch any of the test cases I wrote. (it's also missing a } )
>
> I intend never to be a perl monk.
>
> I notice the missing { before the elsif after I sent it.
> Oh well.
>
> I just typed it in the emailer, so it's not tested at all.
> Also it's missing a $ after 1 in the second $memset_size test.
still didn't catch anything for me.
> What are your test cases anyway?
memset(foo, 0, 10);
memset(foo, 10, 0);
memset(foo, 1, 10);
memset(foo, 10, 1);
> Likely $Lval isn't matching things like
> sizeof(*foo) so this isn't checked:
>
> memset(foo, bar, sizeof(*foo));
I chose to just ignore any non integer arguments to keep things simple.
Dave
On Wed, 2010-08-18 at 18:17 -0400, Dave Jones wrote:
> On Wed, Aug 18, 2010 at 02:57:59PM -0700, Joe Perches wrote:
> > > I'm all for improving my shoddy perl where possible, but this doesn't seem to actually
> > > catch any of the test cases I wrote. (it's also missing a } )
> > I intend never to be a perl monk.
> > I notice the missing { before the elsif after I sent it.
> > Oh well.
> > I just typed it in the emailer, so it's not tested at all.
> > Also it's missing a $ after 1 in the second $memset_size test.
> still didn't catch anything for me.
> > What are your test cases anyway?
>
> memset(foo, 0, 10);
> memset(foo, 10, 0);
> memset(foo, 1, 10);
> memset(foo, 10, 1);
>
> > Likely $Lval isn't matching things like
> > sizeof(*foo) so this isn't checked:
> >
> > memset(foo, bar, sizeof(*foo));
>
> I chose to just ignore any non integer arguments to keep things simple.
This seems to work.
I think $FuncArg is not great though.
Because $FuncArg uses $match_balanced_parens, the
match list args are unexpected.
$2, $4, and $6 are the args of any memset argument
that uses style func(args)
ie: memset(addr(foo), val(bar), sizeof(*foo))
Andy, what do you think?
Not-signed-off-by: Joe Perches
---
scripts/checkpatch.pl | 18 ++++++++++++++++++
1 files changed, 18 insertions(+), 0 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2039acd..af7a4f9 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -172,6 +172,10 @@ our $Operators = qr{
&&|\|\||,|\^|\+\+|--|&|\||\+|-|\*|\/|%
}x;
+our $match_balanced_parens = qr/(\((?:[^\(\)]++|(?-1))*\))/;
+our $Function = qr{(?:$Ident\s*$match_balanced_parens)};
+our $FuncArg = qr{(?:$Lval|$Constant|$Function)};
+
our $NonptrType;
our $Type;
our $Declare;
@@ -2655,6 +2659,20 @@ sub process {
WARN("sizeof(& should be avoided\n" . $herecurr);
}
+# Check for misused memsets
+
+ if ($line =~ /^\+.*\bmemset\s*\(\s*($FuncArg)\s*,\s*($FuncArg)\s*,\s*($FuncArg)\s*\)/) {
+ my $memset_addr = $1;
+ my $memset_val = $3;
+ my $memset_size = $5;
+
+ if ($memset_size =~ /^(0x|)0$/i) {
+ ERROR("memset uses second argument as constant byte value, not third.\n" . $herecurr);
+ } elsif ($memset_size =~ /^(0x|)1$/i) {
+ WARN("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)