Avoid automatic k[mz]alloc with multiplies conversions
Inspired-by: "Theodore Ts'o" <[email protected]>
Cc: "Theodore Ts'o" <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Andrew Morton <[email protected]>
Signed-off-by: Fabian Frederick <[email protected]>
---
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
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;
}
On Fri, 27 Jun 2014 14:37:04 -0700
Joe Perches <[email protected]> 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
On Sun, 2014-06-29 at 11:21 +0200, Fabian Frederick wrote:
> On Fri, 27 Jun 2014 14:37:04 -0700 Joe Perches <[email protected]> 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.
> On 29 June 2014 at 11:33 Joe Perches <[email protected]> wrote:
>
>
> On Sun, 2014-06-29 at 11:21 +0200, Fabian Frederick wrote:
> > On Fri, 27 Jun 2014 14:37:04 -0700 Joe Perches <[email protected]> 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
>
>
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 <[email protected]>
Noticed-by: Theodore Ts'o <[email protected]>
Original-patch-by: Fabian Frederick <[email protected]>
---
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;
}