* [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 related [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 related [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 related [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.