[Cocci] Finding missing return value checks for some function calls with SmPL

[Cocci] Finding missing return value checks for some function calls with SmPL

[SF Markus Elfring]

Hello,

I would like to try another search pattern out with the semantic patch language
from the software ?Coccinelle 1.0.6-00022-g862f5b29 (OCaml 4.03)?.


SmPL script example:

@find_too_late_checking@
expression ex1, ex2;
identifier action1 =~ "^(?x)
(?:
   kzalloc
|  kmalloc_array
)$",
           action2 =~ "^(?x)
(?:
   kzalloc
|  kmalloc_array
)$",
           work;
type return_type;
@@
 return_type work(...)
 {
 ... when any
*ex1 = action1(...);
 ex2 = action2(...);
 ... when any
 }


Source code example:

static void __init cpg_mstp_clocks_init(struct device_node *np)
{
	struct mstp_clock_group *group;
	const char *idxname;
	struct clk **clks;
	unsigned int i;

	group = kzalloc(sizeof(*group), GFP_KERNEL);
	clks = kmalloc(MSTP_MAX_CLOCKS * sizeof(*clks), GFP_KERNEL);
	if (group == NULL || clks == NULL) {
		kfree(group);
		kfree(clks);
		pr_err("%s: failed to allocate group\n", __func__);
		return;
	}
}


elfring at Sonne:~/Projekte/Coccinelle/janitor> spatch.opt show_too_late_checking2.cocci ../Probe/clk-mstp-excerpt1.c
init_defs_builtins: /usr/local/lib64/coccinelle/standard.h
HANDLING: ../Probe/clk-mstp-excerpt1.c


Now I wonder why the first assignment is not marked together with the function call
for further considerations by my source code analysis approach so far.
I would appreciate your advices.

Regards,
Markus

[Cocci] Finding missing return value checks for some function calls with SmPL

[SF Markus Elfring]

> Now I wonder why the first assignment is not marked together with the function call
> for further considerations by my source code analysis approach so far.

I should have adjusted one of the used regular expressions a bit more like the following.


@find_too_late_checking@
expression ex1, ex2;
identifier action1 =~ "^(?x)
(?:
   kzalloc
|  kmalloc_array
)$",
           action2 =~ "^(?x)
(?:
   kzalloc
|  kmalloc(?:_array)?
)$",
           work;
type return_type;
@@
 return_type work(...)
 {
 ... when any
*ex1 = action1(...);
 ex2 = action2(...);
 ... when any
 }


elfring at Sonne:~/Projekte/Coccinelle/janitor> spatch.opt show_too_late_checking2.cocci ../Probe/clk-mstp-excerpt1.c
init_defs_builtins: /usr/local/lib64/coccinelle/standard.h
HANDLING: ../Probe/clk-mstp-excerpt1.c
diff = 
--- ../Probe/clk-mstp-excerpt1.c
+++ /tmp/cocci-output-20109-52f048-clk-mstp-excerpt1.c
@@ -5,7 +5,6 @@ static void __init cpg_mstp_clocks_init(
 	struct clk **clks;
 	unsigned int i;
 
-	group = kzalloc(sizeof(*group), GFP_KERNEL);
 	clks = kmalloc(MSTP_MAX_CLOCKS * sizeof(*clks), GFP_KERNEL);
 	if (group == NULL || clks == NULL) {
 		kfree(group);


I imagine that there are further software improvements to consider around
such use cases.

Regards,
Markus

[Cocci] Finding missing return value checks for some function calls with SmPL

[Julia Lawall]

On Fri, 2 Dec 2016, SF Markus Elfring wrote:

> > Now I wonder why the first assignment is not marked together with the function call
> > for further considerations by my source code analysis approach so far.
>
> I should have adjusted one of the used regular expressions a bit more like the following.
>
>
> @find_too_late_checking@
> expression ex1, ex2;
> identifier action1 =~ "^(?x)
> (?:
>    kzalloc
> |  kmalloc_array
> )$",
>            action2 =~ "^(?x)
> (?:
>    kzalloc
> |  kmalloc(?:_array)?
> )$",
>            work;
> type return_type;
> @@
>  return_type work(...)
>  {
>  ... when any
> *ex1 = action1(...);
>  ex2 = action2(...);
>  ... when any
>  }
>
>
> elfring at Sonne:~/Projekte/Coccinelle/janitor> spatch.opt show_too_late_checking2.cocci ../Probe/clk-mstp-excerpt1.c
> init_defs_builtins: /usr/local/lib64/coccinelle/standard.h
> HANDLING: ../Probe/clk-mstp-excerpt1.c
> diff =
> --- ../Probe/clk-mstp-excerpt1.c
> +++ /tmp/cocci-output-20109-52f048-clk-mstp-excerpt1.c
> @@ -5,7 +5,6 @@ static void __init cpg_mstp_clocks_init(
>  	struct clk **clks;
>  	unsigned int i;
>
> -	group = kzalloc(sizeof(*group), GFP_KERNEL);
>  	clks = kmalloc(MSTP_MAX_CLOCKS * sizeof(*clks), GFP_KERNEL);
>  	if (group == NULL || clks == NULL) {
>  		kfree(group);
>
>
> I imagine that there are further software improvements to consider around
> such use cases.

Not sure what you are asking about here.  Note that it is unlikely that
anyone would accept a patch on the above code.  Group is tested before it
is used.

julia

[Cocci] Finding missing return value checks for some function calls with SmPL

[SF Markus Elfring]

>> elfring at Sonne:~/Projekte/Coccinelle/janitor> spatch.opt show_too_late_checking2.cocci ../Probe/clk-mstp-excerpt1.c
>> init_defs_builtins: /usr/local/lib64/coccinelle/standard.h
>> HANDLING: ../Probe/clk-mstp-excerpt1.c
>> diff =
>> --- ../Probe/clk-mstp-excerpt1.c
>> +++ /tmp/cocci-output-20109-52f048-clk-mstp-excerpt1.c
>> @@ -5,7 +5,6 @@ static void __init cpg_mstp_clocks_init(
>>  	struct clk **clks;
>>  	unsigned int i;
>>
>> -	group = kzalloc(sizeof(*group), GFP_KERNEL);
>>  	clks = kmalloc(MSTP_MAX_CLOCKS * sizeof(*clks), GFP_KERNEL);
>>  	if (group == NULL || clks == NULL) {
>>  		kfree(group);
>>
>>
>> I imagine that there are further software improvements to consider around
>> such use cases.
> 
> Not sure what you are asking about here.

A bit more explanation can be found in my update suggestion
?[PATCH 3/4] clk/Renesas-MSTP: Less function calls in cpg_mstp_clocks_init()
after error detection? from 2016-09-14.

https://patchwork.kernel.org/patch/9332367/
https://lkml.kernel.org/r/<b945de99-815a-b380-e13c-17b01e0febad@users.sourceforge.net>


> Note that it is unlikely that anyone would accept a patch on the above code.

I would agree because I used the asterisk functionality from the Coccinelle software
just to try another source code search pattern out a bit more for test purposes.

I guess that corresponding syntax elements can be clarified further.


> Group is tested before it is used.

But can it be that this assignment is checked a bit too late?

Regards,
Markus

[Cocci] Finding missing return value checks for some function calls with SmPL

[SF Markus Elfring]

>> But can it be that this assignment is checked a bit too late?
> 
> Yes,

Thanks that you can agree to such a view.


> but no one will care.

I am curious if further software development opinions will appear around
similar situations.


> The code is more concise as is.

I have got an other opinion for such an implementation detail.


> If the alloc fails, the machine is already in a very bad state, so one more alloc
> attempt doesn't matter.

Would it be nicer to avoid a questionable function call (and a corresponding
variable assignment) when it was determined that the desired task could not be
completed for a moment?

Regards,
Markus

[Cocci] Comparing SmPL script constraints with direct regular expression interface

[SF Markus Elfring]

Hello,

I tried another source code search pattern out with the semantic patch language
from the software ?Coccinelle 1.0.6-00022-g862f5b29 (OCaml 4.03)?.


SmPL script example:

@find_too_late_checking@
expression assign1, assign2, ex1, ex2;
identifier action1 =~ "^(?x)
(?:
   k(?:[cmz]alloc|(?:m(?:alloc_array|em_cache_alloc(?:_node)?)|zalloc_node))
|  of_find_matching_node
# Alternation placeholder
)$",
           action2 =~ "^(?x)
(?:
   k(?:[cmz]alloc|(?:m(?:alloc_array|em_cache_alloc(?:_node)?)|zalloc_node))
|  of_find_matching_node
# Alternation placeholder
)$",
           work;
statement is, es;
type return_type;
@@
 return_type work(...)
 {
 ... when any
*ex1 = action1(...);
 ex2 = action2(...);
 ... when any
     when != (ex1 = assign1)
     when != (ex2 = assign2)
 if (
*      \( !(ex1) \| (ex1) == NULL \)
    || \( !(ex2) \| (ex2) == NULL \)
    || ...)
    is
 else
    es
 ... when any
 }


elfring at Sonne:~/Projekte/Coccinelle/janitor> spatch.opt show_too_late_checking1.cocci ../Probe/clk-mstp-excerpt1.c
?
diff = 
--- ../Probe/clk-mstp-excerpt1.c
+++ /tmp/cocci-output-23612-4723e1-clk-mstp-excerpt1.c
@@ -5,9 +5,7 @@ static void __init cpg_mstp_clocks_init(
 	struct clk **clks;
 	unsigned int i;
 
-	group = kzalloc(sizeof(*group), GFP_KERNEL);
 	clks = kmalloc(MSTP_MAX_CLOCKS * sizeof(*clks), GFP_KERNEL);
-	if (group == NULL || clks == NULL) {
 		kfree(group);
 		kfree(clks);
 		pr_err("%s: failed to allocate group\n", __func__);


This source code analysis approach is incomplete so far. It demonstrates a few
aspects where further software extensions might help.

1. The semantic patch language supports data processing with regular expressions
   to some degree as direct constraints for metavariables.
   A ?regexp? can be specified for several metavariables. Such regular expressions
   can become very detailed and big so that I got the desire to avoid
   code duplication there as much as possible.
   So it would be nice if SmPL variables can share a specific compiled ?regexp?
   from a single specification place.

2. The support for script constraints was recently mentioned.
   https://github.com/coccinelle/coccinelle/commit/cbc751b30d9e02390d60ebed643c8e4a3fa0bb2b

   But I am unsure about their current development and documentation status.
   Are there any more imaginations floating around for the notation and usage
   of named (or ad hoc) predicates?

Regards,
Markus

[Cocci] Comparing SmPL script constraints with direct regular expression interface

[Julia Lawall]

On Fri, 2 Dec 2016, SF Markus Elfring wrote:

> Hello,
>
> I tried another source code search pattern out with the semantic patch language
> from the software ?Coccinelle 1.0.6-00022-g862f5b29 (OCaml 4.03)?.
>
>
> SmPL script example:
>
> @find_too_late_checking@
> expression assign1, assign2, ex1, ex2;
> identifier action1 =~ "^(?x)
> (?:
>    k(?:[cmz]alloc|(?:m(?:alloc_array|em_cache_alloc(?:_node)?)|zalloc_node))
> |  of_find_matching_node
> # Alternation placeholder
> )$",
>            action2 =~ "^(?x)
> (?:
>    k(?:[cmz]alloc|(?:m(?:alloc_array|em_cache_alloc(?:_node)?)|zalloc_node))
> |  of_find_matching_node
> # Alternation placeholder
> )$",
>            work;
> statement is, es;
> type return_type;
> @@
>  return_type work(...)
>  {
>  ... when any
> *ex1 = action1(...);
>  ex2 = action2(...);
>  ... when any
>      when != (ex1 = assign1)
>      when != (ex2 = assign2)
>  if (
> *      \( !(ex1) \| (ex1) == NULL \)
>     || \( !(ex2) \| (ex2) == NULL \)
>     || ...)
>     is
>  else
>     es
>  ... when any
>  }
>
>
> elfring at Sonne:~/Projekte/Coccinelle/janitor> spatch.opt show_too_late_checking1.cocci ../Probe/clk-mstp-excerpt1.c
> ?
> diff =
> --- ../Probe/clk-mstp-excerpt1.c
> +++ /tmp/cocci-output-23612-4723e1-clk-mstp-excerpt1.c
> @@ -5,9 +5,7 @@ static void __init cpg_mstp_clocks_init(
>  	struct clk **clks;
>  	unsigned int i;
>
> -	group = kzalloc(sizeof(*group), GFP_KERNEL);
>  	clks = kmalloc(MSTP_MAX_CLOCKS * sizeof(*clks), GFP_KERNEL);
> -	if (group == NULL || clks == NULL) {
>  		kfree(group);
>  		kfree(clks);
>  		pr_err("%s: failed to allocate group\n", __func__);
>
>
> This source code analysis approach is incomplete so far. It demonstrates a few
> aspects where further software extensions might help.
>
> 1. The semantic patch language supports data processing with regular expressions
>    to some degree as direct constraints for metavariables.
>    A ?regexp? can be specified for several metavariables. Such regular expressions
>    can become very detailed and big so that I got the desire to avoid
>    code duplication there as much as possible.
>    So it would be nice if SmPL variables can share a specific compiled ?regexp?
>    from a single specification place.
>
> 2. The support for script constraints was recently mentioned.
>    https://github.com/coccinelle/coccinelle/commit/cbc751b30d9e02390d60ebed643c8e4a3fa0bb2b
>
>    But I am unsure about their current development and documentation status.
>    Are there any more imaginations floating around for the notation and usage
>    of named (or ad hoc) predicates?

I don't anticipate any changes in user-facing functionality in either of
these cases in the short term.

julia

[Cocci] Comparing SmPL script constraints with direct regular expression interface

[SF Markus Elfring]

>>    https://github.com/coccinelle/coccinelle/commit/cbc751b30d9e02390d60ebed643c8e4a3fa0bb2b
>>
>>    But I am unsure about their current development and documentation status.
>>    Are there any more imaginations floating around for the notation and usage
>>    of named (or ad hoc) predicates?
> 
> I don't anticipate any changes in user-facing functionality

I would appreciate a bit more explicit information for open isses.


> in either of these cases in the short term.

Have we got just different views about the relevance of curly brackets around
the specification of a function name (instead of a complete function body)?

Would other delimiters help to make the corresponding data passing a bit shorter?

Regards,
Markus

[Cocci] Comparing SmPL script constraints with direct regular expression interface

[SF Markus Elfring]

> I don't think there is anything to be more explicit about.

Do you find the documentation complete for this functionality at the moment?


> I don't foresee any change in the regular expression syntax,

I imagine a need for advanced configuration possibilities around ?regexp? engines.


> and I don't forsee any change in the constraint script syntax.

Is the current approach the official one already?


> Perhaps the constraint scrpt syntax could be generalized to allow more
> in practice than function calls.

Which software development concerns can hinder progress in the way
I imagine so far?


> That might happen some day, but it is an extremely low priority.

I am curious if the involved dependencies can be clarified further.


>> Have we got just different views about the relevance of curly brackets around
>> the specification of a function name (instead of a complete function body)?
> 
> No idea what this means.  What curly braces?  What function name?

I find this kind of feedback strange.

I hope that communication difficulties can be resolved better around
the provided test case, can't it?
https://github.com/coccinelle/coccinelle/blob/cbc751b30d9e02390d60ebed643c8e4a3fa0bb2b/tests/idcon_ocaml.cocci

Regards,
Markus

[Cocci] Comparing SmPL script constraints with direct regular expression interface

[SF Markus Elfring]

>> Do you find the documentation complete for this functionality at the moment?
> 
> No idea.

How can we achieve a better common understanding on this aspect for example?


>> I imagine a need for advanced configuration possibilities around ?regexp? engines.
> 
> You imagine many things.

Yes, of course.   ;-)


> Changes will only happen when there is a concrete need.

I came along some development needs also together with software experiments
around SmPL scripts.
How do your needs look different over time?


>> Is the current approach the official one already?
> 
> Yes.

Thanks for this information.


>>> Perhaps the constraint scrpt syntax could be generalized to allow more
>>> in practice than function calls.
>>
>> Which software development concerns can hinder progress in the way
>> I imagine so far?
> 
> Lack of time.
> 
>>
>>> That might happen some day, but it is an extremely low priority.
>>
>> I am curious if the involved dependencies can be clarified further.
> 
> I doubt there are any dependencies.

Our software development capacity is limited as usual.
Will future software research activities improve the situation a bit more?


>> https://github.com/coccinelle/coccinelle/blob/cbc751b30d9e02390d60ebed643c8e4a3fa0bb2b/tests/idcon_ocaml.cocci
> 
> In the test case the braces are around a function call, not a function name.

This detail is also clear for me to some degree.


> No idea what could be relevant or irrelevant about it.

I got further ideas for this software area after a delay.


> That is the syntax that has been implemented.

I wonder still why such a variant was chosen.


> It may change when someone when someone motivated enough to work on the
> implementation writes the code to make the change and tests the result.

My motivation could eventually increase if the understanding of the
corresponding OCaml source code could become better somehow besides
other factors.


> It won't change just because someone finds it conceptually inelegant
> and sends lots of emails about it.

So there is some usual change resistance. I dare to provide intensive feedback
in the hope that details can be uncovered which can support more progress.


> It's actually strange that you want to define regexps in other places,

This detail is just another aspect for the involved software evolution.


> but you don't want to define scripts in other places.

I got also further ideas around the placement of script fragments.


> Why not just define a python function that you can call in a script
> to do your big regexp for you?

This approach can only work safely if script constraints will be completely
documented in the manual (besides one test case in OCaml and Python).

Regards,
Markus

[Cocci] Comparing SmPL script constraints with direct regular expression interface

[SF Markus Elfring]

> Why not just define a python function that you can call in a script to do
> your big regexp for you?

Would any more developers and reviewers like to extend a source code analysis
approach with a regular expression (or other data structure) like the following?

SmPL script example:

@initialize:python@
@@
import re
compiled = re.compile("""^
(?:
   k(?:
       [cmz]alloc
    |  (?:m(?:
              alloc_array
           |  em(?:
                   _cache_alloc(?:_node)?
                |  dup
                )
           )
       |  zalloc_node
       )
    |  str(?:
             dup(?:_const)?
          |  ndup
          )
    )
|  of_find_matching_node
# Alternation placeholder
)$""", re.VERBOSE)

def is_selected(id):
    match = compiled.search(id)
    if match:
        return True
    else:
        return False

@find_too_late_checking@
expression assign1, assign2, ex1, ex2;
identifier action1: script:python() { is_selected(action1) },
           action2: script:python() { is_selected(action2) },
           work;
statement is, es;
type return_type;
@@
 return_type work(...)
 {
 ... when any
*ex1 = action1(...);
 ex2 = action2(...);
 ... when any
     when != (ex1 = assign1)
     when != (ex2 = assign2)
 if (
*      \( !(ex1) \| (ex1) == NULL \)
    || \( !(ex2) \| (ex2) == NULL \)
    || ...)
    is
 else
    es
 ... when any
 }


elfring at Sonne:~/Projekte/Coccinelle/janitor> spatch.opt show_too_late_checking-python-20161202.cocci ../Probe/clk-mstp-excerpt1.c
?
diff = 
--- ../Probe/clk-mstp-excerpt1.c
+++ /tmp/cocci-output-20769-f189ac-clk-mstp-excerpt1.c
@@ -5,9 +5,7 @@ static void __init cpg_mstp_clocks_init(
 	struct clk **clks;
 	unsigned int i;
 
-	group = kzalloc(sizeof(*group), GFP_KERNEL);
 	clks = kmalloc(MSTP_MAX_CLOCKS * sizeof(*clks), GFP_KERNEL);
-	if (group == NULL || clks == NULL) {
 		kfree(group);
 		kfree(clks);
 		pr_err("%s: failed to allocate group\n", __func__);


Regards,
Markus

[Cocci] Comparing SmPL script constraints with direct regular expression interface

[Michael Stefaniuc]

On 12/02/2016 10:48 PM, SF Markus Elfring wrote:
>> Why not just define a python function that you can call in a script to do
>> your big regexp for you?
> 
> Would any more developers and reviewers like to extend a source code analysis
> approach with a regular expression (or other data structure) like the following?
Why would you want to use that?
Just use the regexp directly, e.g.
identifier action1 = "regexp";
I'm using big regexp'es that way. Yes, I do find the perl style compact
regexp easier to read.

And for more specialized stuff I'm passing the identifier through a
python rule for further filtering.


bye
	michael

> 
> SmPL script example:
> 
> @initialize:python@
> @@
> import re
> compiled = re.compile("""^
> (?:
>    k(?:
>        [cmz]alloc
>     |  (?:m(?:
>               alloc_array
>            |  em(?:
>                    _cache_alloc(?:_node)?
>                 |  dup
>                 )
>            )
>        |  zalloc_node
>        )
>     |  str(?:
>              dup(?:_const)?
>           |  ndup
>           )
>     )
> |  of_find_matching_node
> # Alternation placeholder
> )$""", re.VERBOSE)
> 
> def is_selected(id):
>     match = compiled.search(id)
>     if match:
>         return True
>     else:
>         return False
> 
> @find_too_late_checking@
> expression assign1, assign2, ex1, ex2;
> identifier action1: script:python() { is_selected(action1) },
>            action2: script:python() { is_selected(action2) },
>            work;
> statement is, es;
> type return_type;
> @@
>  return_type work(...)
>  {
>  ... when any
> *ex1 = action1(...);
>  ex2 = action2(...);
>  ... when any
>      when != (ex1 = assign1)
>      when != (ex2 = assign2)
>  if (
> *      \( !(ex1) \| (ex1) == NULL \)
>     || \( !(ex2) \| (ex2) == NULL \)
>     || ...)
>     is
>  else
>     es
>  ... when any
>  }
> 

[Cocci] Finding missing return value checks for some function calls with SmPL

[SF Markus Elfring]

>> @find_?@
>> expression ex1, ex2, value;
>> identifier action =~ ?;
>> @@
>> *ex1 = action(...);
>>  ex2 = value;
>>
>>
>> I am unsure if such a source code search approach should eventually be split
>> into more SmPL processing steps.
> 
> (
> ex1 = action(...);
> ex2 = <+...ex1...+>;
> |
> *ex1 = action(...);
>  ex2 = value;
> )
> 
> is perhaps what you are looking for.

We discussed further possibilities around advanced usage of SmPL constraints a bit.
The software evolution is unclear in this direction and might be too limited
at the moment.

But the shown SmPL disjunction could also work with the Coccinelle software
for a while. It has got run time characteristics where I got a few concerns.

The asterisk functionality is used there so that source code places
can be marked as update candidates. Unfortunately, it seems that the software
development challenge can be too big for the specification of a generic
change approach.

Is such a small source code search pattern good enough in principle to be
integrated into the evolving script collection for checking of various source files
(including from Linux)?

Regards,
Markus