[Cocci] [PATCH] Coccinelle: alloc_cast: Add more memory allocating functions to the list

[Himanshu Jha]

Add more memory allocating functions that are frequently used in the kernel
code to the existing list and remove the useless casts where it is
unnecessary.

But preserve those casts having __attribute__ such as __force, __iomem,
etc which are used by Sparse in the static analysis of the code.

Also remove two blank lines at EOF.

Signed-off-by: Himanshu Jha <***@gmail.com>
---
scripts/coccinelle/api/alloc/alloc_cast.cocci | 92 +++++++++++++++++++++------
1 file changed, 71 insertions(+), 21 deletions(-)

diff --git a/scripts/coccinelle/api/alloc/alloc_cast.cocci b/scripts/coccinelle/api/alloc/alloc_cast.cocci
index 6c308ee..408ee38 100644
--- a/scripts/coccinelle/api/alloc/alloc_cast.cocci
+++ b/scripts/coccinelle/api/alloc/alloc_cast.cocci
@@ -5,10 +5,11 @@
//# kmalloc, kzalloc, kcalloc, kmem_cache_alloc, kmem_cache_zalloc,
//# kmem_cache_alloc_node, kmalloc_node and kzalloc_node and removes
//# the casting as it is not required. The result in the patch case may
-//#need some reformatting.
+//# need some reformatting.
//
// Confidence: High
-// Copyright: 2014, Himangi Saraogi GPLv2.
+// Copyright: (C) 2014 Himangi Saraogi GPLv2.
+// Copyright: (C) 2017 Himanshu Jha GPLv2.
// Comments:
// Options: --no-includes --include-headers
//
@@ -18,55 +19,104 @@ virtual patch
virtual org
virtual report

+@initialize:python@
+@@
+import re
+pattern = '__'
+m = re.compile(pattern)
+
+@r1 depends on context || patch@
+type T;
+@@
+
+ (T *)
+ \(kmalloc\|kzalloc\|kcalloc\|kmem_cache_alloc\|kmem_cache_zalloc\|
+ kmem_cache_alloc_node\|kmalloc_node\|kzalloc_node\|vmalloc\|vzalloc\|
+ dma_alloc_coherent\|dma_zalloc_coherent\|devm_kmalloc\|devm_kzalloc\|
+ kvmalloc\|kvzalloc\|kvmalloc_node\|kvzalloc_node\|pci_alloc_consistent\|
+ pci_zalloc_consistent\|kmem_alloc\|kmem_zalloc\|kmem_zone_alloc\|
+ kmem_zone_zalloc\|vmalloc_node\|vzalloc_node\)(...)
+
//----------------------------------------------------------
// For context mode
//----------------------------------------------------------

-@depends on context@
-type T;
+@script:python depends on context@
+t << r1.T;
+@@
+
+if m.search(t) != None:
+ cocci.include_match(False)
+
+@depends on context && r1@
+type r1.T;
@@

* (T *)
\(kmalloc\|kzalloc\|kcalloc\|kmem_cache_alloc\|kmem_cache_zalloc\|
- kmem_cache_alloc_node\|kmalloc_node\|kzalloc_node\)(...)
+ kmem_cache_alloc_node\|kmalloc_node\|kzalloc_node\|vmalloc\|vzalloc\|
+ dma_alloc_coherent\|dma_zalloc_coherent\|devm_kmalloc\|devm_kzalloc\|
+ kvmalloc\|kvzalloc\|kvmalloc_node\|kvzalloc_node\|pci_alloc_consistent\|
+ pci_zalloc_consistent\|kmem_alloc\|kmem_zalloc\|kmem_zone_alloc\|
+ kmem_zone_zalloc\|vmalloc_node\|vzalloc_node\)(...)

//----------------------------------------------------------
// For patch mode
//----------------------------------------------------------

-@depends on patch@
-type T;
+@script:python depends on patch@
+t << r1.T;
+@@
+
+if m.search(t) != None:
+ cocci.include_match(False)
+
+@depends on patch && r1@
+type r1.T;
@@

- (T *)
- (\(kmalloc\|kzalloc\|kcalloc\|kmem_cache_alloc\|kmem_cache_zalloc\|
- kmem_cache_alloc_node\|kmalloc_node\|kzalloc_node\)(...))
+ \(kmalloc\|kzalloc\|kcalloc\|kmem_cache_alloc\|kmem_cache_zalloc\|
+ kmem_cache_alloc_node\|kmalloc_node\|kzalloc_node\|vmalloc\|vzalloc\|
+ dma_alloc_coherent\|dma_zalloc_coherent\|devm_kmalloc\|devm_kzalloc\|
+ kvmalloc\|kvzalloc\|kvmalloc_node\|kvzalloc_node\|pci_alloc_consistent\|
+ pci_zalloc_consistent\|kmem_alloc\|kmem_zalloc\|kmem_zone_alloc\|
+ kmem_zone_zalloc\|vmalloc_node\|vzalloc_node\)(...)

//----------------------------------------------------------
// For org and report mode
//----------------------------------------------------------

-@r depends on org || report@
+@r2 depends on org || report@
type T;
position p;
@@

- (***@p *)\(kmalloc\|kzalloc\|kcalloc\|kmem_cache_alloc\|kmem_cache_zalloc\|
- kmem_cache_alloc_node\|kmalloc_node\|kzalloc_node\)(...)
+ (***@p *)
+ \(kmalloc\|kzalloc\|kcalloc\|kmem_cache_alloc\|kmem_cache_zalloc\|
+ kmem_cache_alloc_node\|kmalloc_node\|kzalloc_node\|vmalloc\|vzalloc\|
+ dma_alloc_coherent\|dma_zalloc_coherent\|devm_kmalloc\|devm_kzalloc\|
+ kvmalloc\|kvzalloc\|kvmalloc_node\|kvzalloc_node\|pci_alloc_consistent\|
+ pci_zalloc_consistent\|kmem_alloc\|kmem_zalloc\|kmem_zone_alloc\|
+ kmem_zone_zalloc\|vmalloc_node\|vzalloc_node\)(...)

@script:python depends on org@
-p << r.p;
-t << r.T;
+p << r2.p;
+t << r2.T;
@@

-coccilib.org.print_safe_todo(p[0], t)
+if m.search(t) != None:
+ cocci.include_match(False)
+else:
+ coccilib.org.print_safe_todo(p[0], t)

@script:python depends on report@
-p << r.p;
-t << r.T;
+p << r2.p;
+t << r2.T;
@@

-msg="WARNING: casting value returned by memory allocation function to (%s *) is useless." % (t)
-coccilib.report.print_report(p[0], msg)
-
-
+if m.search(t) != None:
+ cocci.include_match(False)
+else:
+ msg="WARNING: casting value returned by memory allocation function to (%s *) is useless." % (t)
+ coccilib.report.print_report(p[0], msg)

[Julia Lawall]

Post by Himanshu Jha
Add more memory allocating functions that are frequently used in the kernel
code to the existing list and remove the useless casts where it is
unnecessary.
But preserve those casts having __attribute__ such as __force, __iomem,
etc which are used by Sparse in the static analysis of the code.
Also remove two blank lines at EOF.
---
scripts/coccinelle/api/alloc/alloc_cast.cocci | 92 +++++++++++++++++++++------
1 file changed, 71 insertions(+), 21 deletions(-)
diff --git a/scripts/coccinelle/api/alloc/alloc_cast.cocci b/scripts/coccinelle/api/alloc/alloc_cast.cocci
index 6c308ee..408ee38 100644
--- a/scripts/coccinelle/api/alloc/alloc_cast.cocci
+++ b/scripts/coccinelle/api/alloc/alloc_cast.cocci
@@ -5,10 +5,11 @@
//# kmalloc, kzalloc, kcalloc, kmem_cache_alloc, kmem_cache_zalloc,
//# kmem_cache_alloc_node, kmalloc_node and kzalloc_node and removes
//# the casting as it is not required. The result in the patch case may
-//#need some reformatting.
+//# need some reformatting.
//
// Confidence: High
-// Copyright: 2014, Himangi Saraogi GPLv2.
+// Copyright: (C) 2014 Himangi Saraogi GPLv2.
+// Copyright: (C) 2017 Himanshu Jha GPLv2.
// Options: --no-includes --include-headers
//
@@ -18,55 +19,104 @@ virtual patch
virtual org
virtual report
+import re
+pattern = '__'
+m = re.compile(pattern)
+
+type T;
+
+ (T *)
+ \(kmalloc\|kzalloc\|kcalloc\|kmem_cache_alloc\|kmem_cache_zalloc\|
+ kmem_cache_alloc_node\|kmalloc_node\|kzalloc_node\|vmalloc\|vzalloc\|
+ dma_alloc_coherent\|dma_zalloc_coherent\|devm_kmalloc\|devm_kzalloc\|
+ kvmalloc\|kvzalloc\|kvmalloc_node\|kvzalloc_node\|pci_alloc_consistent\|
+ pci_zalloc_consistent\|kmem_alloc\|kmem_zalloc\|kmem_zone_alloc\|
+ kmem_zone_zalloc\|vmalloc_node\|vzalloc_node\)(...)
+
//----------------------------------------------------------
// For context mode
//----------------------------------------------------------
-type T;
+t << r1.T;
+
+ cocci.include_match(False)
+
+type r1.T;
@@
* (T *)
\(kmalloc\|kzalloc\|kcalloc\|kmem_cache_alloc\|kmem_cache_zalloc\|
- kmem_cache_alloc_node\|kmalloc_node\|kzalloc_node\)(...)
+ kmem_cache_alloc_node\|kmalloc_node\|kzalloc_node\|vmalloc\|vzalloc\|
+ dma_alloc_coherent\|dma_zalloc_coherent\|devm_kmalloc\|devm_kzalloc\|
+ kvmalloc\|kvzalloc\|kvmalloc_node\|kvzalloc_node\|pci_alloc_consistent\|
+ pci_zalloc_consistent\|kmem_alloc\|kmem_zalloc\|kmem_zone_alloc\|
+ kmem_zone_zalloc\|vmalloc_node\|vzalloc_node\)(...)
//----------------------------------------------------------
// For patch mode
//----------------------------------------------------------
-type T;
+t << r1.T;
+
+ cocci.include_match(False)
+
+type r1.T;
@@
- (T *)
- (\(kmalloc\|kzalloc\|kcalloc\|kmem_cache_alloc\|kmem_cache_zalloc\|
- kmem_cache_alloc_node\|kmalloc_node\|kzalloc_node\)(...))
+ \(kmalloc\|kzalloc\|kcalloc\|kmem_cache_alloc\|kmem_cache_zalloc\|
+ kmem_cache_alloc_node\|kmalloc_node\|kzalloc_node\|vmalloc\|vzalloc\|
+ dma_alloc_coherent\|dma_zalloc_coherent\|devm_kmalloc\|devm_kzalloc\|
+ kvmalloc\|kvzalloc\|kvmalloc_node\|kvzalloc_node\|pci_alloc_consistent\|
+ pci_zalloc_consistent\|kmem_alloc\|kmem_zalloc\|kmem_zone_alloc\|
+ kmem_zone_zalloc\|vmalloc_node\|vzalloc_node\)(...)
//----------------------------------------------------------
// For org and report mode
//----------------------------------------------------------
type T;
position p;
@@
- kmem_cache_alloc_node\|kmalloc_node\|kzalloc_node\)(...)
+ \(kmalloc\|kzalloc\|kcalloc\|kmem_cache_alloc\|kmem_cache_zalloc\|
+ kmem_cache_alloc_node\|kmalloc_node\|kzalloc_node\|vmalloc\|vzalloc\|
+ dma_alloc_coherent\|dma_zalloc_coherent\|devm_kmalloc\|devm_kzalloc\|
+ kvmalloc\|kvzalloc\|kvmalloc_node\|kvzalloc_node\|pci_alloc_consistent\|
+ pci_zalloc_consistent\|kmem_alloc\|kmem_zalloc\|kmem_zone_alloc\|
+ kmem_zone_zalloc\|vmalloc_node\|vzalloc_node\)(...)
@script:python depends on org@
-p << r.p;
-t << r.T;
+p << r2.p;
+t << r2.T;
@@
-coccilib.org.print_safe_todo(p[0], t)
+ cocci.include_match(False)
+ coccilib.org.print_safe_todo(p[0], t)
@script:python depends on report@
-p << r.p;
-t << r.T;
+p << r2.p;
+t << r2.T;
@@
-msg="WARNING: casting value returned by memory allocation function to (%s *) is useless." % (t)
-coccilib.report.print_report(p[0], msg)
-
-
+ cocci.include_match(False)
+ msg="WARNING: casting value returned by memory allocation function to (%s *) is useless." % (t)
+ coccilib.report.print_report(p[0], msg)
--
2.7.4

[SF Markus Elfring]

Post by Himanshu Jha
Add more memory allocating functions that are frequently used in the kernel
code to the existing list and remove the useless casts where it is
unnecessary.

Thanks for your approach to extend this SmPL script.

I would find the term “selection” more appropriate than the word “list” here.

Post by Himanshu Jha
+ \(kmalloc\|kzalloc\|kcalloc\|kmem_cache_alloc\|kmem_cache_zalloc\|

I suggest to choose an other code layout for such growing SmPL disjunctions.
How are the chances to specify each function name on a separate script line?


Would you ever like to reduce code duplication by including external files?


Could an alternation (regular expression) be used for a SmPL constraint?

Regards,
Markus

[SF Markus Elfring]

My software development attention was caught by another implementation detail
in this evolving SmPL script.
The added script rule should get a condition.
Would the specification “depends on report” be appropriate there?


Regards,
Markus

[Julia Lawall]

Post by SF Markus Elfring
My software development attention was caught by another implementation detail
in this evolving SmPL script.
The added script rule should get a condition.
Would the specification “depends on report” be appropriate there?

To my recollection, which is defined in the initialized is used in all of
the cases, so there is no need for a depends on.

julia

[SF Markus Elfring]

Post by Julia Lawall

Post by SF Markus Elfring
The added script rule should get a condition.
Would the specification “depends on report” be appropriate there?

To my recollection,

I wonder about this wording …

Post by Julia Lawall
which is defined in the initialized is used in all of the cases,

I imagine that you could know it better.

Post by Julia Lawall
so there is no need for a depends on.

The default usage mode for the affected SmPL script is “report”.
https://bottest.wiki.kernel.org/coccicheck#modes

But can an other mode also be occasionally selected here?
How do you think about to avoid the extra initialisation
when only the operation mode “context” or “patch” would be active?

Regards,
Markus

[Julia Lawall]

Post by Julia Lawall

Post by SF Markus Elfring
The added script rule should get a condition.
Would the specification “depends on report” be appropriate there?

To my recollection,

I wonder about this wording 


Well, you didn't keep the semantic patch in the message.

Post by Julia Lawall
which is defined in the initialized is used in all of the cases,

I imagine that you could know it better.

Post by Julia Lawall
so there is no need for a depends on.

The default usage mode for the affected SmPL script is “report”.
https://bottest.wiki.kernel.org/coccicheck#modes
But can an other mode also be occasionally selected here?
How do you think about to avoid the extra initialisation
when only the operation mode “context” or “patch” would be active?

context and patch use the regular expression too. All forms of the rule
need it, to keep the casts that contain sparse annotations.

julia

[Julia Lawall]

Post by Julia Lawall
Well, you didn't keep the semantic patch in the message.

You can take another look in other information systems (if desired).
https://systeme.lip6.fr/pipermail/cocci/2018-January/004848.html

Post by Julia Lawall

Post by SF Markus Elfring
How do you think about to avoid the extra initialisation
when only the operation mode “context” or “patch” would be active?

context and patch use the regular expression too.

Where?

Look at the script code just under the comment "For context mode" and just
under the comment "For patch mode". Both of them use m. m is declared in
the initialize.

Post by Julia Lawall
All forms of the rule need it, to keep the casts that contain sparse annotations.

How do “annotations” matter here?
Is a specific variable used only by two SmPL rules?



Post by Julia Lawall
+m = re.compile(pattern)




Post by Julia Lawall
@script:python depends on report@





Why are two underscores used as a search pattern in this use case?

How about just removing the script code and looking your self at what
happens in eg the patch case if it is not there.

__ are typically used in sparse annotations.

julia

[SF Markus Elfring]

Post by Julia Lawall
Look at the script code just under the comment "For context mode" and just
under the comment "For patch mode". Both of them use m.

I would like to apologise that I overlooked these places somehow.

Post by Julia Lawall
__ are typically used in sparse annotations.

Is this search pattern worth for a corresponding comment in the Python code?


How do you think about to pass it to the method “compile” directly (without the variable “pattern”)?

Regards,
Markus

[Himanshu Jha]

Post by SF Markus Elfring

Post by Julia Lawall
Look at the script code just under the comment "For context mode" and just
under the comment "For patch mode". Both of them use m.

I would like to apologise that I overlooked these places somehow.

Rather than apologising, start looking the SmPL grammar documentation
first. For eg. You asked about the 'initialize' code so take your time
and read what actually initialize does rather than writing about each
and very change. If its not mentioned there then ask Julia, that is what
I do. But first search around the documentation.

Post by SF Markus Elfring

Post by Julia Lawall
__ are typically used in sparse annotations.

Now, again look for what __attribute__ means and why is it used ? How is
it benefiticial to sparse for static analysis. Even wikipedia explains
that!

Post by SF Markus Elfring
Is this search pattern worth for a corresponding comment in the Python code?
How do you think about to pass it to the method “compile” directly (without the variable “pattern”)?

Would removing that boost performance ?
pattern variable improves readability and I mean people can easily guess
"Ok he searching for a pattern ..."

Whatever you wish to suggest, don't just email directly! Look for
documentation, explore, test the patch with the -next tree and show the
benchmarks to the relevant people. And finally prepare a patch and send
upstream.

Respect the developers, they also have their job.
Don't ask trivial questions to Julia or any other develper before
searching and exploring. She also has her research and other productive
stuff to do and not just sitting idle replying to each and every email.

Also, don't linger on *trivial* stuffs that improve the comment, add a
tabspace etc.

Lastly, you have a habit of saying:
"How about this ... ?' "How about that ... ?"
"What if ... ?"

You have all the tools, documentations, source code, and you can explore
it yourself too before asking. That is what we call *opensource* :-)

BONUS:
https://www.kernel.org/doc/html/latest/dev-tools/coccinelle.html
http://coccinelle.lip6.fr/docs/index.html
https://en.wikipedia.org/wiki/Sparse#Annotations

[Julia Lawall]

4. Is is more efficient to use the Python key word “in” to look for
a short string instead of fiddling with the programming interface
for regular expressions?

If you test it and find that it is significantly more efficient, then
please send a patch accordingly.

5. Please omit the variable “msg” in the last SmPL rule.
The need data can be passed directly instead.

The use of the msg variable is common in the semantic patches in the
kernel. Ampng other things it increases the chance that the line will fit
in 80 characters. It is not completely a success here, but at least it is
more of a success than it would be otherwise.

julia

[Julia Lawall]

Post by Julia Lawall

4. Is is more efficient to use the Python key word “in” to look for
a short string instead of fiddling with the programming interface
for regular expressions?

If you test it and find that it is significantly more efficient,
then please send a patch accordingly.

Should the support for a specific key word in this programming language
be generally faster or just more direct than invoking a component
from another software library?
Do we need to start an official benchmarking session to achieve
another decision in any software design direction?

I have no idea about python. If you find a performance improvement, send
a patch, If you don't find a performance improvement, I don't care about
the issue. In any case, the ball is entirely in your court. There is no
"we".

Post by Julia Lawall

5. Please omit the variable “msg” in the last SmPL rule.
The need data can be passed directly instead.

The use of the msg variable is common in the semantic patches in the kernel.

Can this implementation detail be adjusted?

Post by Julia Lawall
Ampng other things it increases the chance that the line will fit
in 80 characters. It is not completely a success here, but at least it is
more of a success than it would be otherwise.

Such data can also be passed as function parameters (in a scripting language)
without storing them into variables before in some use cases.

But then, if a pleasant indentation scheme is followed, the very long
string will be further to the right and will even more exceed the 80
column boundary. If python is so sensitive that it cares about the
introduction of a variable that is only used immediately afterwards, then
it would seem that the problem is with python.

julia

[SF Markus Elfring]

Post by Himanshu Jha
+// Copyright: (C) 2017 Himanshu Jha GPLv2.

* Is the addition “(C)” required?
* Does the year number need an update?



My software development attention was caught by further implementation details
in this evolving SmPL script.
Will the use of a position variable be helpful here?

Post by Himanshu Jha
+ \(kmalloc\|kzalloc\|kcalloc\|kmem_cache_alloc\|kmem_cache_zalloc\|

Did you consider to handle the relevant function names by a metavariable
instead of a SmPL disjunction?

Regards,
Markus

[Julia Lawall]

Post by SF Markus Elfring
Will the use of a position variable be helpful here?

No. There is no need to repeat the same check for each place where a
given type occurs. If the type always looks the same, checking it once is
sufficient for all uses.

Post by SF Markus Elfring

Post by Himanshu Jha
+ \(kmalloc\|kzalloc\|kcalloc\|kmem_cache_alloc\|kmem_cache_zalloc\|

Did you consider to handle the relevant function names by a metavariable
instead of a SmPL disjunction?

If you put the names into the constraints of a metavariable there will be
no file filtering based on the function names and the semantic patch will
run more slowly.

julia

[Julia Lawall]

Post by Julia Lawall

Post by SF Markus Elfring
Will the use of a position variable be helpful here?

No. There is no need to repeat the same check for each place where a
given type occurs.

I imagine that information from a position which was detected by the SmPL
rule “r1” should be reused by subsequent SmPL rules.

I don't fully understand the above, but if python code inherits a position
variable, its effect only applies to that position. If there are three
different positions, but the other inputs are the same, the python code
will still be run three times. There is no need for this in the context or
patch case, because the behavior of the python code does not depend on the
position in any way.

Post by Julia Lawall
If the type always looks the same, checking it once is sufficient
for all uses.

Can it be safer to take specific source places into account precisely
(by metavariable inheritance)?

There is nothing more or less safe about it.

Post by Julia Lawall

Post by SF Markus Elfring

Post by Himanshu Jha
+ \(kmalloc\|kzalloc\|kcalloc\|kmem_cache_alloc\|kmem_cache_zalloc\|

Did you consider to handle the relevant function names by a metavariable
instead of a SmPL disjunction?

If you put the names into the constraints of a metavariable there will be
no file filtering based on the function names

Why?
Is this a limitation by the Coccinelle software so far?
Can they become more appealing?
* SmPL list constraint
* Advanced regular expression

Constraints can be arbitrarily complicated and don't necessarily fit into
the various filtering options provided (grep, id-utils, glimpse). Now
that you can put script code into constraints, they can also have side
effects. Thus, the user needs to be able to understand how often they
will be run.

Post by Julia Lawall
and the semantic patch will run more slowly.

How did you determine such software behaviour?

If Coccinelle knows the set of words that must be present in a file for
the rule to match, and if an index has been prepared with glimpse or
id-utils, then no time is spent on irrelevant files. Parsing an
irrelevant file is surely more expensive than doing nothing with it. If
no index is available, then Coccinelle will search line by line for the
important words before trying to parse the file. Reading through each
word of a file once is still likely much cheaper than parsing, which may
perform up to four attempts on each function. I don't know what is the
exact tradeoff in teh case of this specific rule, because the functions
involved are very common.

julia

[Julia Lawall]

In the report rule we print the position, so having the position among the
inherited metavariables is useful.

Do position variables influence (in a positive way?) how fast a source code place
is matched in a non-script rule?

To some extent. You are welcome to compare the performance. I expect
that the difference would be small in this case, because there would only
be a few of these calls per file anyway.

If no index is available,

How often will this be the usual case? ;-)

It depends on the user of Coccinelle.

Can SmPL scripts adapt to the situation when an index was selected (or not)?

It doesn't really matter. The point is that constraints are arbitrarily
complex, and the filtering, however it is done, is intended to be a simple
and fast process.

julia

[Masahiro Yamada]

Post by Himanshu Jha
Add more memory allocating functions that are frequently used in the kernel
code to the existing list and remove the useless casts where it is
unnecessary.
But preserve those casts having __attribute__ such as __force, __iomem,
etc which are used by Sparse in the static analysis of the code.
Also remove two blank lines at EOF.
---
scripts/coccinelle/api/alloc/alloc_cast.cocci | 92 +++++++++++++++++++++------
1 file changed, 71 insertions(+), 21 deletions(-)

Applied to linux-kbuild/misc. Thanks!