* [PATCH 1/1] scripts/checkpatch.pl: update kmalloc_array/kcalloc conversion warning
@ 2014-06-27 20:10 Fabian Frederick
2014-06-27 21:37 ` Joe Perches
0 siblings, 1 reply; 6+ messages in thread
From: Fabian Frederick @ 2014-06-27 20:10 UTC (permalink / raw)
To: linux-kernel
Cc: Fabian Frederick, Theodore Ts'o, Joe Perches, Andrew Morton
Avoid automatic k[mz]alloc with multiplies conversions
Inspired-by: "Theodore Ts'o" <tytso@mit.edu>
Cc: "Theodore Ts'o" <tytso@mit.edu>
Cc: Joe Perches <joe@perches.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Fabian Frederick <fabf@skynet.be>
---
scripts/checkpatch.pl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 182be0f..cf58a7a 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4427,7 +4427,7 @@ sub process {
$newfunc = "kcalloc" if ($oldfunc eq "kzalloc");
if ($a1 =~ /^sizeof\s*\S/ || $a2 =~ /^sizeof\s*\S/) {
if (WARN("ALLOC_WITH_MULTIPLY",
- "Prefer $newfunc over $oldfunc with multiply\n" . $herecurr) &&
+ "Prefer $newfunc over $oldfunc with multiply when arguments are not fixed or come from unvalidated source\n" . $herecurr) &&
$fix) {
my $r1 = $a1;
my $r2 = $a2;
--
1.8.4.5
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] scripts/checkpatch.pl: update kmalloc_array/kcalloc conversion warning
2014-06-27 20:10 [PATCH 1/1] scripts/checkpatch.pl: update kmalloc_array/kcalloc conversion warning Fabian Frederick
@ 2014-06-27 21:37 ` Joe Perches
2014-06-29 9:21 ` Fabian Frederick
0 siblings, 1 reply; 6+ messages in thread
From: Joe Perches @ 2014-06-27 21:37 UTC (permalink / raw)
To: Fabian Frederick; +Cc: linux-kernel, Theodore Ts'o, Andrew Morton
On Fri, 2014-06-27 at 22:10 +0200, Fabian Frederick wrote:
> Avoid automatic k[mz]alloc with multiplies conversions
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -4427,7 +4427,7 @@ sub process {
> $newfunc = "kcalloc" if ($oldfunc eq "kzalloc");
> if ($a1 =~ /^sizeof\s*\S/ || $a2 =~ /^sizeof\s*\S/) {
> if (WARN("ALLOC_WITH_MULTIPLY",
> - "Prefer $newfunc over $oldfunc with multiply\n" . $herecurr) &&
> + "Prefer $newfunc over $oldfunc with multiply when arguments are not fixed or come from unvalidated source\n" . $herecurr) &&
> $fix) {
> my $r1 = $a1;
> my $r2 = $a2;
I'm not sure of the value of this as I think at some point
if not already today, the compiler will optimize the multiply
away.
But it's probably better to look at the non-sizeof variable
and emit the warning only when it's not $Constant or some
upper-case only macro #define like "\b[A-Z_]+\b" is used.
Maybe something like this:
scripts/checkpatch.pl | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 6c7cbaf..e0d0ead 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4486,22 +4486,23 @@ sub process {
# check for k[mz]alloc with multiplies that could be kmalloc_array/kcalloc
if ($^V && $^V ge 5.10.0 &&
- $line =~ /\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*(k[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)/) {
+ $line =~ /\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*(k[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)\s*,/) {
my $oldfunc = $3;
my $a1 = $4;
my $a2 = $10;
my $newfunc = "kmalloc_array";
$newfunc = "kcalloc" if ($oldfunc eq "kzalloc");
- if ($a1 =~ /^sizeof\s*\S/ || $a2 =~ /^sizeof\s*\S/) {
+ my $r1 = $a1;
+ my $r2 = $a2;
+ if ($a1 =~ /^sizeof\s*\S/) {
+ $r1 = $a2;
+ $r2 = $a1;
+ }
+ if ($r1 !~ /^sizeof\b/ && $r2 =~ /^sizeof\s*\S/ &&
+ !($r1 =~ /^$Constant$/ || $r1 =~ /^[A-Z_]+$/)) {
if (WARN("ALLOC_WITH_MULTIPLY",
"Prefer $newfunc over $oldfunc with multiply\n" . $herecurr) &&
$fix) {
- my $r1 = $a1;
- my $r2 = $a2;
- if ($a1 =~ /^sizeof\s*\S/) {
- $r1 = $a2;
- $r2 = $a1;
- }
$fixed[$linenr - 1] =~ s/\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*(k[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)/$1 . ' = ' . "$newfunc(" . trim($r1) . ', ' . trim($r2)/e;
}
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] scripts/checkpatch.pl: update kmalloc_array/kcalloc conversion warning
2014-06-27 21:37 ` Joe Perches
@ 2014-06-29 9:21 ` Fabian Frederick
2014-06-29 9:33 ` Joe Perches
0 siblings, 1 reply; 6+ messages in thread
From: Fabian Frederick @ 2014-06-29 9:21 UTC (permalink / raw)
To: Joe Perches; +Cc: linux-kernel, Theodore Ts'o, Andrew Morton
On Fri, 27 Jun 2014 14:37:04 -0700
Joe Perches <joe@perches.com> wrote:
> On Fri, 2014-06-27 at 22:10 +0200, Fabian Frederick wrote:
> > Avoid automatic k[mz]alloc with multiplies conversions
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -4427,7 +4427,7 @@ sub process {
> > $newfunc = "kcalloc" if ($oldfunc eq "kzalloc");
> > if ($a1 =~ /^sizeof\s*\S/ || $a2 =~ /^sizeof\s*\S/) {
> > if (WARN("ALLOC_WITH_MULTIPLY",
> > - "Prefer $newfunc over $oldfunc with multiply\n" . $herecurr) &&
> > + "Prefer $newfunc over $oldfunc with multiply when arguments are not fixed or come from unvalidated source\n" . $herecurr) &&
> > $fix) {
> > my $r1 = $a1;
> > my $r2 = $a2;
>
> I'm not sure of the value of this as I think at some point
> if not already today, the compiler will optimize the multiply
> away.
>
> But it's probably better to look at the non-sizeof variable
> and emit the warning only when it's not $Constant or some
> upper-case only macro #define like "\b[A-Z_]+\b" is used.
>
> Maybe something like this:
>
> scripts/checkpatch.pl | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 6c7cbaf..e0d0ead 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -4486,22 +4486,23 @@ sub process {
>
> # check for k[mz]alloc with multiplies that could be kmalloc_array/kcalloc
> if ($^V && $^V ge 5.10.0 &&
> - $line =~ /\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*(k[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)/) {
> + $line =~ /\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*(k[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)\s*,/) {
> my $oldfunc = $3;
> my $a1 = $4;
> my $a2 = $10;
> my $newfunc = "kmalloc_array";
> $newfunc = "kcalloc" if ($oldfunc eq "kzalloc");
> - if ($a1 =~ /^sizeof\s*\S/ || $a2 =~ /^sizeof\s*\S/) {
> + my $r1 = $a1;
> + my $r2 = $a2;
> + if ($a1 =~ /^sizeof\s*\S/) {
> + $r1 = $a2;
> + $r2 = $a1;
> + }
> + if ($r1 !~ /^sizeof\b/ && $r2 =~ /^sizeof\s*\S/ &&
> + !($r1 =~ /^$Constant$/ || $r1 =~ /^[A-Z_]+$/)) {
> if (WARN("ALLOC_WITH_MULTIPLY",
> "Prefer $newfunc over $oldfunc with multiply\n" . $herecurr) &&
> $fix) {
> - my $r1 = $a1;
> - my $r2 = $a2;
> - if ($a1 =~ /^sizeof\s*\S/) {
> - $r1 = $a2;
> - $r2 = $a1;
> - }
> $fixed[$linenr - 1] =~ s/\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*(k[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)/$1 . ' = ' . "$newfunc(" . trim($r1) . ', ' . trim($r2)/e;
>
> }
>
>
Hello Joe,
Thanks, it looks great. Already tested ? If not I can do it and give you some feedback ...
Fabian
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] scripts/checkpatch.pl: update kmalloc_array/kcalloc conversion warning
2014-06-29 9:21 ` Fabian Frederick
@ 2014-06-29 9:33 ` Joe Perches
2014-07-01 20:07 ` Fabian Frederick
0 siblings, 1 reply; 6+ messages in thread
From: Joe Perches @ 2014-06-29 9:33 UTC (permalink / raw)
To: Fabian Frederick; +Cc: linux-kernel, Theodore Ts'o, Andrew Morton
On Sun, 2014-06-29 at 11:21 +0200, Fabian Frederick wrote:
> On Fri, 27 Jun 2014 14:37:04 -0700 Joe Perches <joe@perches.com> wrote:
[]
> > I'm not sure of the value of this as I think at some point
> > if not already today, the compiler will optimize the multiply
> > away.
> >
> > But it's probably better to look at the non-sizeof variable
> > and emit the warning only when it's not $Constant or some
> > upper-case only macro #define like "\b[A-Z_]+\b" is used.
[]
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> > + }
> > + if ($r1 !~ /^sizeof\b/ && $r2 =~ /^sizeof\s*\S/ &&
> > + !($r1 =~ /^$Constant$/ || $r1 =~ /^[A-Z_]+$/)) {
This last test should be
/^[A-Z_][A-Z0-9_]*$/
to allow upper case macros with digits too
> Already tested ? If not I can do it and give you some feedback ...
Lightly. Test away.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] scripts/checkpatch.pl: update kmalloc_array/kcalloc conversion warning
2014-06-29 9:33 ` Joe Perches
@ 2014-07-01 20:07 ` Fabian Frederick
2014-07-01 21:33 ` [PATCH] checkpatch: Emit fewer kmalloc_array/kcalloc conversion warnings Joe Perches
0 siblings, 1 reply; 6+ messages in thread
From: Fabian Frederick @ 2014-07-01 20:07 UTC (permalink / raw)
To: Joe Perches; +Cc: Andrew Morton, linux-kernel, Theodore Ts'o
> On 29 June 2014 at 11:33 Joe Perches <joe@perches.com> wrote:
>
>
> On Sun, 2014-06-29 at 11:21 +0200, Fabian Frederick wrote:
> > On Fri, 27 Jun 2014 14:37:04 -0700 Joe Perches <joe@perches.com> wrote:
> []
> > > I'm not sure of the value of this as I think at some point
> > > if not already today, the compiler will optimize the multiply
> > > away.
> > >
> > > But it's probably better to look at the non-sizeof variable
> > > and emit the warning only when it's not $Constant or some
> > > upper-case only macro #define like "\b[A-Z_]+\b" is used.
> []
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > > + }
> > > + if ($r1 !~ /^sizeof\b/ && $r2 =~ /^sizeof\s*\S/ &&
> > > + !($r1 =~ /^$Constant$/ || $r1 =~ /^[A-Z_]+$/)) {
>
> This last test should be
>
> /^[A-Z_][A-Z0-9_]*$/
>
> to allow upper case macros with digits too
>
> > Already tested ? If not I can do it and give you some feedback ...
>
> Lightly. Test away.
Tested on rc3: 766 warnings against 976 with original version.
No more warnings for things like:
mm/huge_memory.c
pages = kmalloc(sizeof(struct page *) * HPAGE_PMD_NR, GFP_KERNEL);
net/ipv6/ip6_fib.c
mp = kzalloc(sizeof(u32) * RTAX_MAX, GFP_KERNEL);
lib/sort.c
a = kmalloc(1000 * sizeof(int), GFP_KERNEL);
Looks good.
Fabian
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH] checkpatch: Emit fewer kmalloc_array/kcalloc conversion warnings
2014-07-01 20:07 ` Fabian Frederick
@ 2014-07-01 21:33 ` Joe Perches
0 siblings, 0 replies; 6+ messages in thread
From: Joe Perches @ 2014-07-01 21:33 UTC (permalink / raw)
To: Andrew Morton; +Cc: Fabian Frederick, Theodore Ts'o, linux-kernel
Avoid matching allocs that appear to be known
small multiplications of a sizeof with a constant
because gcc as of 4.8 cannot optimize the code in
a calloc() exactly the same way as an alloc().
Look for numeric constants or what appear to be
upper case only macro #defines.
Signed-off-by: Joe Perches <joe@perches.com>
Noticed-by: Theodore Ts'o <tytso@mit.edu>
Original-patch-by: Fabian Frederick <fabf@skynet.be>
---
scripts/checkpatch.pl | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 6c7cbaf..e0d0ead 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4486,22 +4486,23 @@ sub process {
# check for k[mz]alloc with multiplies that could be kmalloc_array/kcalloc
if ($^V && $^V ge 5.10.0 &&
- $line =~ /\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*(k[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)/) {
+ $line =~ /\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*(k[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)\s*,/) {
my $oldfunc = $3;
my $a1 = $4;
my $a2 = $10;
my $newfunc = "kmalloc_array";
$newfunc = "kcalloc" if ($oldfunc eq "kzalloc");
- if ($a1 =~ /^sizeof\s*\S/ || $a2 =~ /^sizeof\s*\S/) {
+ my $r1 = $a1;
+ my $r2 = $a2;
+ if ($a1 =~ /^sizeof\s*\S/) {
+ $r1 = $a2;
+ $r2 = $a1;
+ }
+ if ($r1 !~ /^sizeof\b/ && $r2 =~ /^sizeof\s*\S/ &&
+ !($r1 =~ /^$Constant$/ || $r1 =~ /^[A-Z_][A-Z0-9_]*$/)) {
if (WARN("ALLOC_WITH_MULTIPLY",
"Prefer $newfunc over $oldfunc with multiply\n" . $herecurr) &&
$fix) {
- my $r1 = $a1;
- my $r2 = $a2;
- if ($a1 =~ /^sizeof\s*\S/) {
- $r1 = $a2;
- $r2 = $a1;
- }
$fixed[$linenr - 1] =~ s/\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*(k[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)/$1 . ' = ' . "$newfunc(" . trim($r1) . ', ' . trim($r2)/e;
}
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-07-01 21:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-27 20:10 [PATCH 1/1] scripts/checkpatch.pl: update kmalloc_array/kcalloc conversion warning Fabian Frederick
2014-06-27 21:37 ` Joe Perches
2014-06-29 9:21 ` Fabian Frederick
2014-06-29 9:33 ` Joe Perches
2014-07-01 20:07 ` Fabian Frederick
2014-07-01 21:33 ` [PATCH] checkpatch: Emit fewer kmalloc_array/kcalloc conversion warnings Joe Perches
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®