Remove unnecessary null pointer checks?

Hello,

I have tried the following short semantic patch out on a source file directory for Linux 3.13.1.

@Remove_unnecessary_pointer_checks@
expression x;
@@
-if (x)

    kfree(x);

Some update candidates were found. I imagine that this search pattern can be extended a bit more.
How do think about the applicability of corresponding changes for current kernel source code?
What will be a good way to discuss such fixes on other mailing lists eventually?

Regards,
Markus

Not sure what you mean by extended. There are indeed some other functions in Linux that perform NULL tests before doing anything, and thus they don't really need a null test around them. Actually, it could be possible to find such functions using Coccinelle.

On the other hand, I think that one should study the code carefully before making such a change. There are several ways to make error handling code. One is to have one label at the end of the function, to have all error cases jump to there, and then to have if tests to decide what should be done. Another way is to have lots of labels, and jump directly to the right place, without any tests.

I think that the second way is nicer. So in removing such an if, one can consider whether the code could be further improved by adding more labels, and jumping to the right place directly, rather tha executing unnecessary code.

If you want to propose Linux related fixes, you may want to look at the kernel janitors mailing list.

http://vger.kernel.org/vger-lists.html#kernel-janitors

julia

I have tried the following search pattern.

@Show_functions_with_input_pointer_validation@
identifier fun, x;
type t;
@@
*fun(..., t* x, ...)
{
...
if (!x) return;
...
}

Will your current tool version "1.0.0-rc20" find also the kfree() functions?
https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/mm/slab.c#n3641

Should I add any variant of "unlikely(ZERO_OR_NULL_PTR(...))" to my filter pattern eventually?

Would it make sense to add the shown semantic patches to a specific script directory?
https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/scripts/coccinelle

Regards,
Markus

If you want to match a call to ZERO_OR_NULL_PTR, then you need to include that in the semantic patch. Coccinelle does not expand macros, so if doesn't see the relation to a NULL test.

Not sure to understand what the "shown semantic patches" refers to. What you have proposed is a semntic patch that finds functions that test their argument for NULL and return if the test is satisfied. That gives the user some information, but the rules in the Linux kernel are for finding bugs or making transformations.

Personally, I rather like null tests, if they are really needed, ie not implied to be always false by the context, because they give some information, eg that the value can be null at this point, and it is important to take that into account when considerin the following operation. So I'm not very eager to go around removing them.

julia

I have tried a command like the following a moment ago.

elfring at Sonne:~/Projekte/Coccinelle/janitor> MY_PATTERN=find_input_pointer_validation2.cocci && cat $MY_PATTERN && echo '-----' && spatch --version && spatch --sp-file $MY_PATTERN /usr/src/linux-stable/mm/slab.c --recursive-includes -I /usr/src/linux-stable/include/linux -I /usr/src/linux-stable/include/uapi -I /usr/src/linux-stable/arch/ia64/include -I /usr/src/linux-stable/include/asm-generic -I /usr/src/linux-stable/include/asm-generic/bitops -I /usr/local/include/c++/4.8.2 -I /usr/local/include/c++/4.8.2/x86_64-unknown-linux-gnu -I /usr/local/include -I /usr/include
@Show_functions_with_input_pointer_validation@
identifier fun, x;
type t;
@@
*void fun(..., t* x, ...)
{
...
if (unlikely(ZERO_OR_NULL_PTR(x)))

     return;

...
}
-----
spatch version 1.0.0-rc20 with Python support and with PCRE support
init_defs_builtins: ...
(ONCE) TYPE: header trace/events/kmem.h not found

previous modification:
MINUS
According to environment 2:
Show_functions_with_input_pointer_validation.t -> const
void
Show_functions_with_input_pointer_validation.fun -> id kfree

current modification:
MINUS
According to environment 2:
Show_functions_with_input_pointer_validation.t -> const void Show_functions_with_input_pointer_validation.fun -> id kfree

Fatal error: exception Failure("Show_functions_with_input_pointer_validation:
already tagged token:
C code context
File "/usr/src/linux-stable/mm/slab.c", line 3650, column 11, charpos = 94972
around = 'const', whole content = void kfree(const void *objp)")

How can I achieve a better result here?

I try to collect all names for functions which handle passed null pointers in a known way. I hope that such a function list can be converted into a constraint for a metavariable in another semantic patch variant.

Regards,
Markus

I would also like to try another source code search approach like the following.

elfring at Sonne:~/Projekte/Coccinelle/janitor> LINE='-----' && echo $LINE && PAT=list_input_parameter_validation1.cocci && cat $PAT && echo $LINE && SP=/usr/local/bin/spatch && $SP --version && $SP --sp-file $PAT /usr/src/linux-stable/fs/btrfs/extent_map.c
-----
@initialize:python@
@@
import sys
result = []
mark = ['"', '', '"']
delimiter = '|'

def store_positions(fun, typ, point, places):

    """Add source code positions to an internal list."""
    for place in places:
        fields = []
        fields.append(fun)

        mark[1] = typ
        fields.append(''.join(mark))

        fields.append(point)

        mark[1] = place.file.replace('"', '""')
        fields.append(''.join(mark))

        fields.append(place.line)
        fields.append(str(int(place.column) + 1))
        result.append(delimiter.join(fields))

@safety_check@
identifier work, input;
type data_type;
position pos;
@@
void work at pos(...,data_type input,...)
{
...
(
if (!input) return;
|
if (input) { ... }
|
if (input) { ... } else { ... }
|
switch (input) { case 0: return; ... }
)
...
}

@script:python collection depends on safety_check@ typ << safety_check.data_type;
fun << safety_check.work;
point << safety_check.input;
places << safety_check.pos;
@@
store_positions(fun, typ, point, places)

@finalize:python@
@@
if result:
result.insert(0, delimiter.join(("function", '"data type"', '"parameter"',
'"source file"', "line", "column")))
print("\r\n".join(result))
else:
sys.stderr.write("No result for this analysis!\n")
-----
spatch version 1.0.0-rc20 with Python support and with PCRE support
init_defs_builtins: /usr/local/share/coccinelle/standard.h
HANDLING: /usr/src/linux-stable/fs/btrfs/extent_map.c
No result for this analysis!

Why is the function "free_extent_map" not displayed here? Do I need to adjust my SmPL rule "safety_check" anyhow?

Regards,
Markus

Something goes wrong with the switch pattern. I would have to look into why. But I think that a switch is highly improbable for making such a test, so you could just drop it. You may also want to specify that input has pointer type, since you are actually looking for NULL tests, not zero tests.

julia

Thanks for your quick feedback.

I just try to make the discussed filter patterns as complete as possible.

This is an important implementation detail which I would like to generalise so that also functions like "btrfsic_process_written_block" will be safely found by further source code searches.
https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/fs/btrfs/check-integrity.c?id=878a876b2e10888afe53766dcca33f723ae20edc#n1835

This one was found by the previous simple pattern for example despite I expected it to handle primarily pointer data types instead of an "unsigned int" in this case.

Regards,
Markus

As you like. As a general principle, I think it is better to work on fewer things at once. There are many reason why values may be null or 0, and if you lump too many of them together, you will end up with more information than is manageable. No bugs are involved here, you are only making the code simpler, and if you miss some possibilities for making the code simpler, it doesn't matter. You can just work on them later.

julia

I have got another idea for a bit of fine-tuning. I got the impression from the manual that the SmPL syntax element "singleton" might be applicable.

elfring at Sonne:~/Projekte/Coccinelle/janitor> LINE='-----' && echo $LINE && PAT=list_input_parameter_validation2.cocci && cat $PAT && echo $LINE && SP=/usr/local/bin/spatch && $SP --version && $SP --sp-file $PAT /usr/src/linux-stable/fs/btrfs/extent_map.c
-----

@initialize:python@
@@
import sys
result = []
mark = ['"', '', '"']
delimiter = '|'

def store_positions(fun, typ, point, places):

    """Add source code positions to an internal list."""
    for place in places:
        fields = []
        fields.append(fun)

        mark[1] = typ
        fields.append(''.join(mark))

        fields.append(point)

        mark[1] = place.file.replace('"', '""')
        fields.append(''.join(mark))

        fields.append(place.line)
        fields.append(str(int(place.column) + 1))
        result.append(delimiter.join(fields))

@safety_check@
identifier work, input;
type data_type;
position pos;
statement is, es;
@@
void work at pos(...,data_type input,...)
{
...
( if (!input) return;
| if (input) is
? else es
;
)
...
}

@script:python collection depends on safety_check@ typ << safety_check.data_type;
fun << safety_check.work;
point << safety_check.input;
places << safety_check.pos;
@@
store_positions(fun, typ, point, places)

@finalize:python@
@@
if result:
result.insert(0, delimiter.join(("function", '"data type"', '"parameter"',
'"source file"', "line", "column")))
print("\r\n".join(result))
else:
sys.stderr.write("No result for this analysis!\n")
-----
spatch version 1.0.0-rc20 with Python support and with PCRE support
init_defs_builtins: /usr/local/share/coccinelle/standard.h
warning: incompatible arity found on line 36
?safety_check:es
Fatal error: exception Failure("get_before_e: not supported")

Can this situation be improved?

Regards,
Markus

Please don't put all of this noise in your messages. The only thing that is interesting is the file on which you have run Coccinelle.

There is no need to put the ? else es. If you put if (intput) is else es then an isomorphism will consider the case where else es is not there.

Also, the semicolon after is/es does not look right. Notmally one does not put an extra semicolon after an if.

julia

Thanks for your explanation.

I get a "surprise" if I try out the following SmPL variant.

@safety_check@
identifier function, input;
type data_type;
position pos;
statement is, es;
@@
void function at pos(...,data_type input,...)
{
...
( if (!input) return;
| if (input) is else es
)
...
}

@script:python collection depends on safety_check@ typ << safety_check.data_type;
fun << safety_check.function;
point << safety_check.input;
places << safety_check.pos;
@@
store_positions(fun, typ, point, places)

Response:
875 883
Fatal error: exception Failure("scriptmeta: parse error:
= File "list_input_parameter_validation3.cocci", line 43, column 20, charpos = 875

    around = 'function', whole content = fun << safety_check.function;

Can such names be reused which are key words@other places?

Regards,
Markus

Apparently there is a problem in this case. I will make a note of it. In the meantime, just use another name.

julia

I get the following result for the adjusted search pattern.

void work at pos(...,data_type input,...)
{
...
( if (!input) return;
| if (input) is else es
)
...
}

elfring at Sonne:~/Projekte/Coccinelle/janitor> spatch --sp-file list_input_parameter_validation2.cocci /usr/src/linux-stable/arch/um/kernel/sysrq.c
...
function|"data type"|"parameter"|"source file"|line|column show_stack|"unsigned long *"|stack|"/usr/src/linux-stable/arch/um/kernel/sysrq.c"|67|6

This example does not fit to my expectation because it seems that the function implementation does not refer to the passed values.
Do you get any idea for this potential mismatch?

Regards,
Markus

I'm not sure, but I think it has to do with the fact that the entire function is an if where both branches leave the function. It seems to be considering that to be error-handling code. It does not require to find the complete pattern on execution paths that correspond to error-handling code. So it doesn't require to find the pattern at all.

In your case, I think you want to be completely sure that regardless of the execution path chosen, the test is performed. So you should put when string after the first ... (ie the one after the first { ).

julia

I must correct my conclusion here. One function parameter is actually used in the condition "!stack" while the second parameter is forwarded to other function calls.
Is this source code line 83 handled by a SmPL isomorphism?

I'm sorry. I do not really understand your suggestion.

Regards,
Markus

Sorry, I had been looking at the wrong function when I wrote my previous response.

Indeed, there are some uses of isomorphisms at work here. First the

if (input) is else es

is changed to

if (!input) es else is

and then that is changed to

if (!input) es

which matches the code at line 83.

I'm not sure what is the point of the if (input) is else es pattern. I would think you only want to find cases where the function does nothing until it tests input and aborts the function is the value is 0. So you only want the return case. Before it, you would want

... when != S

where S is a statement typed metavariable. That way the ... will only match declarations.

julia

Now it seems that I added it because of too few considerations for isomorphisms.

We have got different expectations about the amount of source code before the statement "return". A bit more fine-tuning with the SmPL construct "when" might be needed to achieve further constraints.

Regards,
Markus

I'm sorry that I overlooked information somehow there.

The implementations of the function "show_stack" can be nice source code examples for further fine-tuning of proposed filter patterns.
https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/arch/um/kernel/sysrq.c?id=8ed12fcc194d93c6a17714120a7027ee4d76a881#n67

Its interface contains two pointer parameters.

I can also adjust the search approach like the following.

@safety_check@
identifier work, input;
type data_type;
position pos;
statement is, es;
@@
void work at pos(...,data_type input,...)
{
...
( if (input) is
| if (likely(input)) is
)
else es
...
}

I guess that the interpretation of "nothing relevant" will need further considerations, won't it?

Not "only" this one ...

I have got a feeling for a need to introspect the else branch for useful properties. I am unsure about better analysis of the metavariable "es" with the semantic patch language.

Regards,
Markus

This is not allowed. Inside a disjunction you need to have complete terms.

I'm not sure to understand your goal. If the code currently has:

if (x != NULL)
call(x);

then the developer does not want to execute any of the code within call if x is NULL. If you just check that there is a NULL test on x somewhere within the definition of call, then that is not enough to ensure that nothing is executed within call. If you remove the NULL test, you could drastically change the behavior of the program.

julia

Thanks for your information. I guess that another adjustment will fit to the SmPL syntax then.

( if (input) is else es
| if (likely(input)) is else es
)

Yes. - This is one of the use cases I am trying to improve. It depends on the knowledge if something unwanted will happen if a null pointer (or zero) would be passed. If it is documented for example that an implementation of the function "call" checks the condition "!= NULL", I find the same check by the caller redundant.

Would you like to suggest any more fine-tuning?

I agree in principle.

But I would like to delete redundant checks from some source files.

Regards,
Markus

I get an interesting result for example if I try the following source code search pattern out. Do you find it useful?

@Delete_unnecessary_checks@
expression x;
identifier release =~ "^(?x)
(?:
(?:kz?|slob_)free
|
(?:
abc
| xyz
)
)$";
@@
-if (x)

    release(...,x,...);

elfring at Sonne:~/Projekte/Coccinelle/janitor> spatch --sp-file delete_unnecessary_checks1.cocci /usr/src/linux-stable/fs/btrfs/inode.c
init_defs_builtins: /usr/local/share/coccinelle/standard.h
HANDLING: /usr/src/linux-stable/fs/btrfs/inode.c

diff =
@@ -5137,8 +5137,7 @@ static int btrfs_dentry_delete(const str
 
 static void btrfs_dentry_release(struct dentry *dentry)
 {
-       if (dentry->d_fsdata)
-               kfree(dentry->d_fsdata);
+       kfree(dentry->d_fsdata);
 }
 
 

But it seems that I stumble on a software debug challenge after I replaced the dummy alternation in the regular expression above by the real list of 5142 function names which were found by the other discussed pattern.

elfring at Sonne:~/Projekte/Coccinelle/janitor> spatch --sp-file delete_unnecessary_checks2.cocci /usr/src/linux-stable/fs/btrfs/inode.c
init_defs_builtins: /usr/local/share/coccinelle/standard.h Fatal error: exception Pcre.Error(_)

Regards,
Markus

I don't think it is a good approach to do anything related to 5142 different functions at once. It would be better to pick a small set of functions, and work on them carefully.

julia

A source code search with the following pattern found only six functions for further consideration.

@is_unnecessary_check@
expression data;
identifier work;
identifier release =~ "^kz?free$";
position pos;
type t;
@@
t work at pos(...)
{
...
( if (data) release(data);
| if (likely(data)) release(data);
)
...
}

I wonder a bit about this analysis result because the SmPL pattern I started this discussion thread with showed a few more update candidates.

I would prefer to handle a more complete fix pattern.

By the way: I read about corresponding software update approaches in an article "Best practices for a big patch series" by Wolfram Sang.

How are the chances to resolve the message "Fatal error: exception Pcre.Error(_)" with a proposed long alternation?

Regards,
Markus

Remember that ... matches the shortest path between what is before and what is after. So if there is another if test on the same data in the function, that will cause a failure. If yo udon't care to have this constraint, you can put when any on a ...

As you like, but I think that no Linux maintainer will accept a patch that makes more than a few changes, so doing them all at once is a waste of time. Anyway, you would have to split up the patches according to maintainer.

Pcre is a standard library. I know nothing about its implementation. But it is not surprising that a regular expression with over 5000 options exceeds its capacity.

julia

Are there any chances to find the source code place from which the exception "Pcre.Error(_)" was thrown?

Regards,
Markus

Set the OCAMLRUNPARAM environment variable to b in your shell. I don't know if it is necessary to recompile coccinelle. Then when it crashes you will get a backtrace.

julia

elfring at Sonne:~/Projekte/Coccinelle/janitor> export OCAMLRUNPARAM=b && spatch --sp-file delete_unnecessary_checks2.cocci /usr/src/linux-stable/fs/btrfs/inode.cinit_defs_builtins:
/usr/local/share/coccinelle/standard.h
Fatal error: exception Pcre.Error(_)
Called from file "engine.ml", line 206, characters 1-27
Called from file "engine.ml", line 359, characters 23-36

How much can this information help us here?

Regards,
Markus

Those two lines at least are not from Coccinelle, so I guess that they are from the PCRE implementation. But I don't think there is anything to do. The regular expression engine is just not likely to be designed to support such a large regular expression.

If you like you can give each of the function names one by one as a command line argument. Eg

spatch file.cocci file.c -D fn=whatever

and then in your semantic patch

@@
identifier virtual.fn;
@@

... use fn as an identifier ...

julia

Are there any chances to retrieve a more detailed error information?

I would try to compare the processing of big patterns with similar interfaces from other software.

I imagine that this approach will result also in different opinions about a useful patch granularity.
http://lwn.net/Articles/585782/

Regards,
Markus

In my opinion, the change you are proposing is controversial. Particular code instances may be corrected in a better way. If you try to do everything at once then the kernel maintainer will have a big block that he has to either accept or reject. If there is one change that is not made in the best way, then he will have to reject it.

On the other hand, if you do a few things at a time, then most of the individual things you do will perhaps be noncontroversial.

julia

Which SmPL "when variant" would you recommend here?

Regards,
Markus

when any

I imagine that it would be nice if parts from an if statement could be written in the shown way. Would such an extension be useful for the semantic patch language?

I can easily adjust my simple pattern here because each if branch will be matched by a metavariable. I hope that the corresponding repetition of bigger subpatterns can be avoided in the future.

Regards,
Markus

The following pattern variant does not show a different analysis result.

@is_unnecessary_check@
expression data;
identifier work;
identifier release =~ "^kz?free$";
position pos;
type t;
@@
t work at pos(...)
{
... when any
( if (data) release(data);
| if (likely(data)) release(data);
)
... when any
}

Regards,
Markus

No, I think that simple rules are better. Always use complete terms is a simple rule.

julia

A source code search with the following pattern found 893 functions which check their single parameter.

@safety_check@
identifier work, input;
type data_type;
position pos;
statement is, es;
@@
void work at pos(data_type input)
{
... when any
( if (input) is else es
| if (likely(input)) is else es
)
... when any
}

Such a name list could be integrated into the following pattern variant.

@Delete_unnecessary_checks@
expression x;
identifier release =~ "^(?x)
(?:
(?:kz?|slob_)free
|
(?:

      # alternation/disjunction of 893 strings ...
   )
)$";
@@
-if (x)
    release(x);

I get an interesting result for example. Do you find it useful?

elfring at Sonne:~/Projekte/Coccinelle/janitor> spatch.opt --sp-file delete_unnecessary_checks3.cocci /usr/src/linux-stable/fs/btrfs/inode.c
init_defs_builtins: /usr/local/share/coccinelle/standard.h
HANDLING: /usr/src/linux-stable/fs/btrfs/inode.c

diff =
@@ -819,8 +819,7 @@ static u64 get_extent_allocation_hint(st
                        em = search_extent_mapping(em_tree, 0, 0);
                        if (em && em->block_start < EXTENT_MAP_LAST_BYTE)
                                alloc_hint = em->block_start;
-                       if (em)
-                               free_extent_map(em);
+                       free_extent_map(em);
                } else {
                        alloc_hint = em->block_start;
                        free_extent_map(em);

@@ -5137,8 +5136,7 @@ static int btrfs_dentry_delete(const str
 
 static void btrfs_dentry_release(struct dentry *dentry)
 {
-       if (dentry->d_fsdata)
-               kfree(dentry->d_fsdata);
+       kfree(dentry->d_fsdata);
 }
 
 static struct dentry *btrfs_lookup(struct inode *dir, struct dentry *dentry,

@@ -6362,8 +6360,7 @@ out:
 
        trace_btrfs_get_extent(root, em);
 
-       if (path)
-               btrfs_free_path(path);
+       btrfs_free_path(path);
        if (trans) {
                ret = btrfs_end_transaction(trans, root);
                if (!err)

Does the following pattern variant show also around 70 update candidates on your software development system?

@is_unnecessary_check@
expression data;
identifier work;
identifier release =~ "^(?x)
(?:
# Big regular expression for a metavariable constraint ...
)$";
position pos;
type t;
@@
t work at pos(...)
{
... when any
( if (data) release(data);
| if (likely(data)) release(data);
)
... when any
}

elfring at Sonne:~/Projekte/Coccinelle/janitor> date && spatch.opt --sp-file list_functions_with_unnecessary_checks1.cocci --dir /usr/src/linux-stable > list_functions_with_unnecessary_checks1.txt 2> list_functions_with_unnecessary_checks1-errors.txt && date
Mi 26. Feb 11:38:14 CET 2014
Mi 26. Feb 12:13:47 CET 2014

The log file contains messages like 'EXN:Invalid_argument("equal: abstract value")'. Do they need further considerations?

Is this intermediate source code analysis result good enough to expand the constructive discussion to a mailing list like "kernel janitors"?

Regards,
Markus

I checks on the three examples. They seem technically correct, in that the called function does immediately do a null test on the argument value. Personally, I still think that the null test at the call site conveys useful information, so I would rather have it there, but others may think differently.

On the other hand, you should not be using linux-stable when working on the Linux kernel. Patches should apply to linux-next, ie the very latest version of all of the code.

If you are convinced that dropping the null tests is a good idea, then you can submit the patch that makes the change to the relevant maintainers and mailing lists. Use scripts/get_maintainers.pl in the Linux kernel to find out who to sent it to. And study the documentation on submitting patches very carefully.

This is an unfortunate constraint on the use of PCRE regular expressions. I will try to track it down. Thanks for the report.

I don't think people on the mailing list want to discuss the issue in a general sense. They just want patches, and may comment on them.

julia

The discussed semantic patches can be tried out with every version a software developer or reviewer might be interested in. I am going to retry them with the newest source files as you suggest.

Thanks for your feedback.

- Do you need any excerpt from my log files?
- Does the message 'EXN:Invalid_argument("equal: abstractvalue")' indicate that the desired source code analysis is influenced in unwanted ways?

Regards,
Markus

No, the semantic patch should be enough to reproduce the problem. If I need more information, I will ask. The problem has no impact on the quality of the answers actually obtained.

julia

Would it be useful that the shown regular expression could be included from an other source file?
How do you think about an extension for the semantic patch language so that such a string could be assigned to a special metavariable with a type like "regex"? Can it be passed from a "virtual" parameter?

Regards,
Markus

A priori, I don't think regular expressions are a good idea at all. It reflects a lac of control over what is matched. Furthermore, Coccinelle doesn't do any optimizations based on regular expressions. If you put a regular expression match in your rule, Coccinelle is likely to have to try (parse etc) every file, because it doesn't know whether the regular expression matches or not. It would be better to use virtual identifiers. That way, if you also use some kind of indexing (glimpse or id-utils), then very little work will be done in most cases.

julia

Can this SmPL interface be marked and used in the way that an identifier does not contain "plain text" but should be handled as a long regular expression?

Regards,
Markus

No.

julia

Hello,

A couple of functions perform input parameter validation before their implementations will try further actions with side effects. Some calling functions perform similar safety checks.

Functions which release a system resource are often documented in the way that they tolerate the passing of a null pointer for example. I do not see a need because of this fact that a function caller repeats a corresponding check.

Now I would like to propose such a change again.

1. Extension of the infrastructure for the analysis tool "coccicheck" Semantic patch patterns can help to identify update candidates also in the
Linux source file hierarchy.

https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/scripts/coccinelle?id=79f0345fefaafb7cde301a830471edd21a37989b

2. Clarification for some automated update suggestions
My source code search approach found seventy functions at least which might
need another review and corresponding corrections for Linux 3.14-rc5. Further software development will point out even more potentially open issues.

Regards,
Markus

From: Markus Elfring <[email protected]>
Date: Wed, 5 Mar 2014 18:15:34 +0100
Subject: [PATCH 1/5] Addition of a semantic patch file for showing unnecessary
checks before a few known functions

This semantic patch pattern tries to find source code places where a check is performed for an expression that is passed to a function (like "kfree") which checks this single parameter itself for usability. Redundant value or pointer checks can be avoided here.

The pattern contains a special comment in a regular expression for a SmPL constraint which supports extensions.

---
 .../deletions/delete_unnecessary_checks_template1.cocci     | 13 +++++++++++++
 1 file changed, 13 insertions(+)
 create mode 100644

scripts/coccinelle/deletions/delete_unnecessary_checks_template1.cocci

diff --git a/scripts/coccinelle/deletions/delete_unnecessary_checks_template1.cocci b/scripts/coccinelle/deletions/delete_unnecessary_checks_template1.cocci new file mode 100644
index 0000000..b092051

@@ -0,0 +1,13 @@
+ at Delete_unnecessary_checks@
+expression x;
+identifier release =~ "^(?x)
+(?:
+   (?:kz?|slob_)free
+|
+   (?:
+# Alternation placeholder
+   )
+)$";
+@@
+-if (x)
+    release(x);

1.9.0

From: Markus Elfring <[email protected]>
Date: Wed, 5 Mar 2014 18:38:43 +0100
Subject: [PATCH 2/5] Addition of a semantic patch file for listing of
functions that check their single parameter

This semantic patch pattern tries to find functions that check their single parameter for usability.

Example:
Null pointer checks are often performed as input parameter validation.

It uses Python statements to write information about the found source code places in a data format that is a variant of a CSV text file.

---
 .../list_input_parameter_validation1.cocci         | 55 ++++++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644

scripts/coccinelle/deletions/list_input_parameter_validation1.cocci

b/scripts/coccinelle/deletions/list_input_parameter_validation1.cocci new file mode 100644
index 0000000..b0a5a52

'"source file"', "line", "column")))
+ print("\r\n".join(result))
+else:
+ sys.stderr.write("No result for this analysis!\n")

1.9.0

From: Markus Elfring <[email protected]>
Date: Wed, 5 Mar 2014 18:58:30 +0100
Subject: [PATCH 3/5] Addition of a SQLite script for a text file import

A script was added so that a text file which was previously generated can be imported into a SQLite data base table.
http://sqlite.org/sqlite.html

The shown file name can be adjusted by a make file for example.

---
 .../coccinelle/deletions/handle_function_list_template.sqlite    | 9 +++++++++
 1 file changed, 9 insertions(+)
 create mode 100644

scripts/coccinelle/deletions/handle_function_list_template.sqlite

b/scripts/coccinelle/deletions/handle_function_list_template.sqlite new file mode 100644
index 0000000..bec366c

@@ -0,0 +1,9 @@
+create table positions(function text,
+                       data_type text,
+                       parameter text,
+                       source_file text,
+                       line integer,
+                       column integer);
+.separator "|"
+.import list_input_pointer_validation1.txt positions
+.header OFF

1.9.0

From: Markus Elfring <[email protected]>
Date: Wed, 5 Mar 2014 19:10:32 +0100
Subject: [PATCH 4/5] Addition of a semantic patch file for listing of
unnecessary checks before a few known functions

This semantic patch pattern tries to find source code places where a check is performed for an expression that is passed to a function (like "kfree") which checks this single parameter itself for usability. Redundant value or pointer checks can be avoided here.

The pattern contains a special comment in a regular expression for a SmPL constraint which supports extensions.

It uses Python statements to write information about the found places in a data format that is a variant of a CSV text file.

---
 ...nctions_with_unnecessary_checks_template1.cocci | 59 ++++++++++++++++++++++
 1 file changed, 59 insertions(+)
 create mode 100644

scripts/coccinelle/deletions/list_functions_with_unnecessary_checks_template1.cocci

diff --git a/scripts/coccinelle/deletions/list_functions_with_unnecessary_checks_template1.cocci b/scripts/coccinelle/deletions/list_functions_with_unnecessary_checks_template1.cocci new file mode 100644
index 0000000..d2637e8
--- /dev/null
+++
b/scripts/coccinelle/deletions/list_functions_with_unnecessary_checks_template1.cocci @@ -0,0 +1,59 @@
+ at initialize:python@
+@@
+import sys
+result = []
+mark = ['"', '', '"']
+delimiter = '|'
+
+def store_positions(fun, point, places):
+ """Add source code positions to an internal list."""
+ for place in places:
+ fields = []
+ fields.append(fun)
+
+ fields.append(point)
+
+ mark[1] = place.file.replace('"', '""')
+ fields.append(''.join(mark))
+

+        fields.append(place.line)
+        fields.append(str(int(place.column) + 1))
+        result.append(delimiter.join(fields))

+
+ at is_unnecessary_check@
+expression data;
+identifier work;
+identifier release =~ "^(?x)
+(?:
+ (?:kz?|slob_)free
+|
+ (?:
+# Alternation placeholder
+ )
+)$";
+position pos;
+type t;
+@@
+ t work at pos(...)
+ {
+ ... when any
+( if (data) release(data);
+| if (likely(data)) release(data);
+)
+ ... when any
+ }
+
+ at script:python collection depends on is_unnecessary_check@ +fun << is_unnecessary_check.work;
+point << is_unnecessary_check.data;
+places << is_unnecessary_check.pos;
+@@
+store_positions(fun, point, places)
+
+@finalize:python@
+@@
+if result:
+ result.insert(0, delimiter.join(("function", '"parameter"', '"source file"',
"line", "column")))
+ print("\r\n".join(result))
+else:
+ sys.stderr.write("No result for this analysis!\n")

1.9.0

From: Markus Elfring <[email protected]>
Date: Wed, 5 Mar 2014 19:40:43 +0100
Subject: [PATCH 5/5] Addition of a make file for build automation

This script can be used to combine some input files for the desired data output which will eventually show update candidates for further source code review. Some build targets were defined. Values for the used make variables can be adjusted by parameters on the command line as usual.

A few implementation details might need more fine-tuning.
- Automatic determination of the Linux source directory from a calling make process

- Setting of an extra output directory for the generated files

---
 scripts/coccinelle/deletions/makefile | 126 ++++++++++++++++++++++++++++++++++
 1 file changed, 126 insertions(+)
 create mode 100644 scripts/coccinelle/deletions/makefile

b/scripts/coccinelle/deletions/makefile
new file mode 100644
index 0000000..6464bae

@@ -0,0 +1,126 @@
+SED=sed
+SPATCH=spatch.opt --sp-file
+RM=rm -f
+LINUX_SOURCE_DIR=/usr/src/linux-stable
+EXTRACT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER=list_input_parameter_validation1.cocci
+SQLITE=sqlite3
+SQLITE_IMPORT_SCRIPT=handle_function_list.sqlite
+SQLITE_IMPORT_SCRIPT_TEMPLATE=handle_function_list_template.sqlite
+RESULT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER=list_input_parameter_validation1.txt
+RESULT_SQL_DATA_BASE=result.db
+RESULT_FUNCTIONS_WITH_PREFIX=add_prefix-SQL.txt
+RESULT_FUNCTIONS_WITH_UNNECESSARY_CHECKS_AS_LIST=list_functions_with_unnecessary_checks1.txt
+RESULT_FUNCTIONS_WITH_UNNECESSARY_CHECKS_AS_PATCH=functions_with_unnecessary_checks1.diff
+LOG_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER=list_input_parameter_validation1-errors.txt
+LOG_LIST_FUNCTIONS_WITH_UNNECESSARY_CHECKS=list_functions_with_unnecessary_checks1-errors.txt
+LOG_PATCH_FUNCTIONS_WITH_UNNECESSARY_CHECKS=functions_with_unnecessary_checks1-errors.txt
+LIST_PATTERN_TEMPLATE=list_functions_with_unnecessary_checks_template1.cocci
+LIST_PATTERN=list_functions_with_unnecessary_checks1.cocci
+PATCH_PATTERN_TEMPLATE=delete_unnecessary_checks_template1.cocci
+PATCH_PATTERN=delete_unnecessary_checks1.cocci
+ESCAPING=XY=$$(< $(RESULT_FUNCTIONS_WITH_PREFIX)) \
+         && XY=$${XY/|/ } \
+         && XY=$${XY//%/\\%} \
+         && $(SED) "s%\# Alternation placeholder%$${XY//$$'\n'/$$'\\\\\n'}%"
+TEXT1=A pattern file was built from which a
+TEXT2=was generated. Good luck with source code review!
+
+default: build_update_candidate_list
+
+$(RESULT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER): \
+$(EXTRACT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER)
+	$(SPATCH) $(EXTRACT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER) \
+-dir $(LINUX_SOURCE_DIR) \
+> $@ 2> $(LOG_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER)
+
+# This replacement action is needed for the use case that the corresponding
+# variable was overridden.
+$(SQLITE_IMPORT_SCRIPT): $(SQLITE_IMPORT_SCRIPT_TEMPLATE)
+	$(SED) "s%import list_input_pointer_validation1\.txt%import $(subst

%,\%,$(RESULT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER))%" \
+$(SQLITE_IMPORT_SCRIPT_TEMPLATE) > $@
+
+$(RESULT_SQL_DATA_BASE) $(RESULT_FUNCTIONS_WITH_PREFIX): \
+$(RESULT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER) \
+$(SQLITE_IMPORT_SCRIPT)
+ @$(RM) $(RESULT_SQL_DATA_BASE)
+ $(SQLITE) -init $(SQLITE_IMPORT_SCRIPT) $(RESULT_SQL_DATA_BASE) \
+'select '\'' | '\'' || function from positions group by function order by
function desc;' \
+> $(RESULT_FUNCTIONS_WITH_PREFIX)
+
+$(LIST_PATTERN): $(RESULT_FUNCTIONS_WITH_PREFIX)
+ $(ESCAPING) $(LIST_PATTERN_TEMPLATE) > $@
+
+$(PATCH_PATTERN): $(RESULT_FUNCTIONS_WITH_PREFIX)
+ $(ESCAPING) $(PATCH_PATTERN_TEMPLATE) > $@
+
+$(RESULT_FUNCTIONS_WITH_UNNECESSARY_CHECKS_AS_LIST): $(LIST_PATTERN)
+ $(SPATCH) $(LIST_PATTERN) -dir $(LINUX_SOURCE_DIR) \
+> $@ 2> $(LOG_LIST_FUNCTIONS_WITH_UNNECESSARY_CHECKS)
+
+$(RESULT_FUNCTIONS_WITH_UNNECESSARY_CHECKS_AS_PATCH): $(PATCH_PATTERN)
+ $(SPATCH) $(PATCH_PATTERN) -dir $(LINUX_SOURCE_DIR) \
+> $@ 2> $(LOG_PATCH_FUNCTIONS_WITH_UNNECESSARY_CHECKS)
+
+build_check_list generate_list_of_functions_which_check_their_single_parameter: \
+$(RESULT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER)
+ @echo 'The list of functions which check their single parameter was generated.'
+
+build_import_script: $(SQLITE_IMPORT_SCRIPT)
+ @echo 'A script was generated which should contain appropriate parameters for
a data base.'
+
+build_data_base: $(RESULT_SQL_DATA_BASE)
+ @echo 'A SQL data base was built.'
+
+build_alternation add_prefix_to_functions: build_data_base
+ @echo 'The function name list was converted to a component for a regular
expression.'
+
+build_list_pattern: $(LIST_PATTERN)
+ @echo '$(TEXT1) list can be generated.'
+
+build_patch_pattern: $(PATCH_PATTERN)
+ @echo '$(TEXT1) patch can be generated.'
+
+build_update_candidate_list show_list_of_update_candidates: build_list_pattern \
+$(RESULT_FUNCTIONS_WITH_UNNECESSARY_CHECKS_AS_LIST)
+ @echo 'The list of update candidates $(TEXT2)'
+
+build_patch show_update_suggestion: build_patch_pattern \
+$(RESULT_FUNCTIONS_WITH_UNNECESSARY_CHECKS_AS_PATCH)
+ @echo 'A patch file $(TEXT2)'
+
+all: build_update_candidate_list build_patch
+
+clean:
+ $(RM) *-errors.txt \
+$(LIST_PATTERN) \
+$(PATCH_PATTERN) \
+$(RESULT_FUNCTIONS_WITH_PREFIX) \
+$(RESULT_SQL_DATA_BASE) \
+$(RESULT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER) \
+$(SQLITE_IMPORT_SCRIPT)
+
+delete_data_base:
+ $(RM) $(RESULT_SQL_DATA_BASE) \
+$(RESULT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER)
+
+.PHONY: all \
+build_check_list \
+build_data_base \
+build_import_script \
+build_list_pattern \
+build_patch \
+build_patch_pattern \
+build_update_candidate_list \
+clean \
+default \
+delete_data_base \
+generate_list_of_functions_which_check_their_single_parameter \
+show_list_of_update_candidates \
+show_update_suggestion
+
+
+# The following input files should not need further actions here. +$(EXTRACT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER) \
+$(SQLITE_IMPORT_SCRIPT_TEMPLATE) \
+$(LIST_PATTERN_TEMPLATE) \
+$(PATCH_PATTERN_TEMPLATE): ;

1.9.0

I have looked into my log files once more so that I can give you another feedback for a "regexp problem".

Now I make a different observation for the processing of SmPL constraints with a long function name list in the regular expression.

elfring at Sonne:~/Projekte/Coccinelle/janitor> OCAMLRUNPARAM=b spatch.opt --sp-file delete_unnecessary_checks3.cocci /usr/src/linux-stable/fs/proc/proc_sysctl.c
init_defs_builtins: /usr/local/share/coccinelle/standard.h
HANDLING: /usr/src/linux-stable/fs/proc/proc_sysctl.c

diff =
@@ -470,8 +470,7 @@ static struct dentry *proc_sys_lookup(st
        d_add(dentry, inode);
 
 out:
-       if (h)
-               sysctl_head_finish(h);
+       sysctl_head_finish(h);
        sysctl_head_finish(head);
        return err;
 }

elfring at Sonne:~/Projekte/Coccinelle/janitor> OCAMLRUNPARAM=b spatch.opt --sp-file list_functions_with_unnecessary_checks1.cocci /usr/src/linux-stable/fs/proc/proc_sysctl.c
init_defs_builtins: /usr/local/share/coccinelle/standard.h
HANDLING: /usr/src/linux-stable/fs/proc/proc_sysctl.c
Fatal error: exception Invalid_argument("equal: abstract value") Called from file "list.ml", line 86, characters 24-34
Called from file "list.ml", line 86, characters 24-34
Called from file "list.ml", line 86, characters 24-34
Called from file "list.ml", line 57, characters 20-23

Regards,
Markus

I can't reproduce this. I need the semantic patch.

julia

How do you think about to try out the scripts which I published yesterday evening?

Regards,
Markus

I am also interested to generate statistics for the declaration of function parameters and their use.
Is it possible to analyse the function parameter list by interfaces from the semantic patch language? Can any useful numbers be extracted at this place?

Regards,
Markus

The question is not very precise. I don't know what you mean by analyse, nor what you mean by interfaces, nor what you mean by useful. You can write things like:

f(...,T i,...) { ... }
where T is a type and i is an identifier

or

f(...,p,...) { ... }
where p is a parameter

or

f(ps,p,...) { ... }
where ps is a parameter list and p is a parameter.

In declaratin a parameter list, you can also say;

parameter list[n] ps;

and then n is the number of parameters that are matched.

julia

Thanks for your interesting details.

I find the use of the element "n" (the name in square brackets) not obvious from the Coccinelle manual so far.

But I would like to try more analysis with the data that is passed by the metavariable type "parameter list". I have started to look a bit into it by the function "pickle.dumps". This kind of display is not so useful for me yet. I am missing documentation for corresponding data structures I can depend on in further reports.

Regards,
Markus

The display from the function "pprint.pformat" shows another implementation detail in my simple script.
http://docs.python.org/2/library/pprint.html

Would you like to clarify data analysis according to the current situation around "coccilib.elems.TermList instance"?

Regards,
Markus

I know nothing about this. Maybe look at the python code and see what it does. But I am not sure that using it is the best way to do whjat you want, although I also don't know what you want to do.

julia

It is the number of elements that have been matched. You can use it in python code (as a string, I think), in ocaml code (as an integer), or inherit it into another list-typed metavariable.

What analysis do you want to do? If you write an ocaml script, you can have access to the comple abstract syntax trees. Python code only receives a string representation.

julia

A part of my difficulty with interpretation of the grammar for the semantic patch language came from this notation.

"metadecl ::= ...

            | parameter [list] ids ;
            | parameter list [ id ] ids ;

..."

Do square brackets indicate in one line that an information is optional while in the other line they should be used as literals?

If I try out the following script ...

@initialize:python@
@@
import sys
result = []
mark = ['"', '', '"']
delimiter = '|'

def store_positions(fun, count, places):
"""Add source code positions to an internal list.""" for place in places:
fields = []
fields.append(fun)

fields.append(count)

mark[1] = place.file.replace('"', '""')
fields.append(''.join(mark))

fields.append(place.line)
fields.append(str(int(place.column) + 1))
result.append(delimiter.join(fields))

@skeleton@
identifier fun;
parameter list [count] pl;
position pos;
@@
fun at pos(pl)
{
...
}

@script:python collection depends on skeleton@
fun << skeleton.fun;
count << skeleton.count;
places << skeleton.pos;
@@
store_positions(fun, count, places)

@finalize:python@
@@
if result:
result.insert(0, delimiter.join(("function", '"parameter count"', '"source
file"', "line", "column")))
print("\r\n".join(result))
else:
sys.stderr.write("No result for this analysis!\n")

... on this source code example, ...

void my_log(char const * text)
{
/* Write something ... */
}

int my_safe_log(char const * text)
{
if (!text)

    return 1;

my_log(text);
return 0;
}

char const * const my_message(void)
{
return "Surprise!";
}

int my_status(void)
{
return 1;
}

int my_addition(char a, char b)
{
return a + b;
}

int main(void)
{
struct my_operations

  {
    void (*log)(char const * t);
    int (*safe_log)(char const * t);
    char const * const (*get_message)(void);
    int (*is_ready)(void);
    int (*add)(char a, char b);

} mo = {my_log, my_safe_log, my_message, my_status, my_addition}, * mop = &mo;

char const * const x = mo.get_message();
int y = mop->is_ready();
y = mop->add(1, 2);
y = mo.add(3, 4);
mo.log(mo.get_message());
mo.is_ready();
my_safe_log(0);
mo.safe_log(0);
mop->safe_log(0);
}

... I get a result that is a bit interesting.

elfring at Sonne:~/Projekte/Coccinelle/janitor> spatch --sp-file list_function_parameters2.cocci ../Probe/f-ptr-test1.c
init_defs_builtins: /usr/local/share/coccinelle/standard.h
HANDLING: ../Probe/f-ptr-test1.c
function|"parameter count"|"source file"|line|column main|1|"../Probe/f-ptr-test1.c"|30|5 my_log|1|"../Probe/f-ptr-test1.c"|1|6 my_message|1|"../Probe/f-ptr-test1.c"|15|20

my_safe_log|1|"../Probe/f-ptr-test1.c"|6|5
my_status|1|"../Probe/f-ptr-test1.c"|20|5
my_addition|2|"../Probe/f-ptr-test1.c"|25|5

I guess that I want to exclude matches for my use case where where the parameter type is "void". I would like to show the distribution of parameter numbers in a source code base.

Regards,
Markus

Yes. The spaces around the [ ] means that they are tokens.

julia

I would appreciate if the distinction will become easier for such characters.

Regards,
Markus

I am surprised by this feedback.

I assume that a corresponding interface description would be nice if I look at "man 3cocci Coccilib". How are the chances to improve the mapping from the OCaml API to the Python binding?

The tool "spatch" counts all elements in a metavariable of the type "parameter list". I would like to treat list elements that have got the function parameter type "void" or no name differently.
- How can I assign the number "zero" (instead of "1") in this case?
- Should I generally exclude parameters without names from another report variant?

I am also interested a bit in run time optimisation. A passed list should be sufficient for further computations. I do not really need a count in another variable which was specified between square brackets of a SmPL rule.

Regards,
Markus

I didn't implement it, and I never use python. The thing you are citing was implemented by a masters student for his masters thesis project, and it contains whatever he found useful.

If you would like to have such a thing you are welcome to implement. In my experience the AST is very rarely useful.

Have a special rule for such functions and use position variables to prevent the rule that matches interesting parameter lists from matching functions that have satisfied this case:

@r@
position p;
identifier f;
@@

f at p(void} { ... }

@goodfunctions@
position p != r.p;
parameter list[n] ps;
identifier f;
@@

f at p(ps) { ... }

Or you can just use your python code to see if ps is just the string "void".

It is hard to imagine why converting all the elements of a list to strings as is done in the interface to python would be more efficient than just passing a single number.

Anyway, the runtime cost is not here, in either case. The runtime cost will be that you have to parse every function of every file.

julia

I would struggle with OCaml script development more so far.

Thanks for your SmPL pattern suggestion.

I would prefer to specify a meaningful constraint for the metavariable "ps" somehow.

I thought also in a similar direction. But I would like to be sure about the passed data structures. I imagine that I would need to inspect the class "coccilib.elems.TermList" if the corresponding documentation is unclear.

I would like to try out two reports at least.

1. What is the maximum number of named function parameters in a source code base? I need only the value "n" (without the additional data from the related
metavariable).

2. Comparison for the incidence of unnamed function parameters to all of them The parameter count could be determined from the list of parameter names
alone, couldn't it?

Regards,
Markus

I have adjusted your suggestion a little bit here.

@no_input@
identifier fun;
position pos;
@@
fun at pos(void) { ... }

@input depends on no_input@
identifier fun;
parameter list [count] pl;
position pos != no_input.pos;
@@
fun at pos(pl) { ... }

@script:python collection depends on input@
fun << input.fun;
count << input.count;
places << input.pos;
@@
store_positions(fun, count, places)

This pattern variant works as expected for a source code example that was already mentioned before.

elfring@Sonne:~/Projekte/Coccinelle/janitor> spatch.opt --sp-file list_function_parameters2.cocci ../Probe/f-ptr-test1.c
init_defs_builtins: /usr/local/share/coccinelle/standard.h
HANDLING: ../Probe/f-ptr-test1.c
function|"parameter count"|"source file"|line|column my_log|1|"../Probe/f-ptr-test1.c"|1|6

my_safe_log|1|"../Probe/f-ptr-test1.c"|6|5
my_addition|2|"../Probe/f-ptr-test1.c"|25|5

But I wonder about the processing for the following small source file.

void my_log(char const * format, ...)
{
/* Write something ... */
}

elfring at Sonne:~/Projekte/Coccinelle/janitor> spatch.opt --sp-file list_function_parameters2.cocci ../Probe/ellipsis-test1.c
init_defs_builtins: /usr/local/share/coccinelle/standard.h
HANDLING: ../Probe/ellipsis-test1.c
No result for this analysis!

I would appreciate your advices.

Regards,
Markus

No need for depends on input. For a python rule, all metavariables have to be bound before anything happens.

I have no idea. I would have sort of expected that it would return a length of 2, but I guess it is reasonable, and even desirable, that it does not. What answer would you like?

julia

I would expect that it will be treated by the SmPL pattern as a function with a single named parameter at least. I am unsure how the "ellipsis" should be counted and matched in the signature eventually.

Does this small source file indicate that there might be further difficulties for data analysis of variadic functions?

Regards,
Markus

Without knowing what you mean by "data analysis" it is impossible to tell. It is clear that there is some problem with ... matching a parameter-typed metavariable. This could be considered reasonable, because ... is not a (single) parameter.

julia

I mean the existence check for a parameter like "format" or extraction of a position for the function signature.

Thanks that you agree to this detail. Would you like to test the handling of variadic macros and functions also in other situations?

Is there any special mapping of the "ellipsis" at this place in the semantic patch language needed?

Regards,
Markus

Would you like to suggest any references for class libraries which work in OCaml with popular data structures in a way I am used to from other programming languages? Is the "convenience" and software speed similar there?

Regards,
Markus

I have no idea what you are asking for.

julia

The programming language "OCaml" has got the property that values from standard data types are immutable. Now I am looking for a well-known OCaml class library which supports the modification of data structures like strings and lists in place. I would prefer to reuse an existing one instead of developing another variation for my experiments.

Regards,
Markus

Why do you want to do this? There is a String.set function that allows you to modify a character of a string. For a list, if you need to modify an element then it should be a list of reference cells. But I think that in practice it would be strange for either operation to be necessary.

julia

I would like to avoid unwanted run time behaviour because of unnecessary data copying when I will append text lines to a growing list.
http://ocaml.org/learn/tutorials/comparison_of_standard_containers.html#Listsimmutablesinglylinkedlists

I imagine that in-place modification will be a more efficient approach for my use case. Is a kind of list buffer available?

Regards,
Markus

You can just add information to the front of the list, and then reverse it at the end if that is needed.

julia

I would like to avoid even insertion at some list heads and a corresponding element reordering because of software efficiency considerations. Is a data structure available for OCaml which is similar to a standard buffer?
http://ocaml.org/learn/tutorials/comparison_of_standard_containers.html#Bufferextensiblestrings

Regards,
Markus

I am trying to convert Python statements into similar OCaml functionality for one of my semantic patch examples.

@initialize:ocaml@
@@
let result = Queue.create ()

let current_function = ref ""
let current_count = ref ""
let empty = ""
let delimiter = "|"
let quote = "\""
let quote2 = "\"\""
let mark text = String.concat empty [quote; text; quote]

let replace text =
let length = String.length text in
let part = Buffer.create length in
for x = 0 to length - 1 do
if text.[x] = '"' then
Buffer.add_string part quote2
else
Buffer.add_char part text.[x]
done;
Buffer.contents part

(* Add a source code position to an internal list. *) let append place =

    Queue.add (String.concat delimiter [!current_function;
                                        !current_count;
                                        mark (replace place.file);
                                        string_of_int place.line;
                                        string_of_int (place.column + 1)]) result

let store_positions fun count places =

    current_function := fun;
    current_count := string_of_int count;
    List.iter append places

@no_input@
identifier fun;
position pos;
@@
fun at pos(void) { ... }

@input depends on no_input@
identifier fun;
parameter list [count] pl;
position pos != no_input.pos;
@@
fun at pos(pl) { ... }

@script:ocaml collection@
fun << input.fun;
count << input.count;
places << input.pos;
@@
store_positions fun count places

@finalize:ocaml@
@@
if Queue.is_empty result then
Printf.eprintf "No result for this analysis!\n"
else
let output text = Printf.printf "%s\r\n" text in

   Printf.printf "%s\r\n" (String.concat delimiter ["function";
                                                    "\"parameter count\"";
                                                    "\"source file\"";
                                                    "line";
                                                    "column"]);

Queue.iter output result

The source code from this SmPL finalisation rule works as expected in a OCaml command line interface. Now I wonder about the following error message.

elfring@Sonne:~/Projekte/Coccinelle/janitor> spatch.opt --sp-file list_function_parameters3.cocci ../Probe/f-ptr-test1.c
init_defs_builtins: /usr/local/share/coccinelle/standard.h
File "list_function_parameters3.cocci", line 69, column 27, charpos = 1914
around = '', whole content = Queue.iter output result
Fatal error: exception Failure("lexing: empty token")

I would appreciate your advices.

Regards,
Markus

Does OCaml not like my name selection here?
Do I accidentally stumble on another software development "surprise" with reserved key words?
http://ocaml.org/learn/faq.html

  let store_positions fu count places =
      current_fu := fu;
      current_count := string_of_int count;
      List.iter append places

Regards,
Markus

It's not related to the problem, but I'm not sure to understand "let output text = ..." Normally the left hand side of a let has only one variable.

julia

I'm sorry. That was a stupid comment :) It is the definition fo a function.

julia

I think that the problem is in the use of \" and perhaps especially '"'. I'm not sure that what you are doing is the normal ocaml way to do what you want to do. Could you explain in words what replace is supposed to do, for example? I never use either for loops or Buffers.

julia

Well, it is not really a surprise. Fun is indeed a reserved keyword in ocaml.

julia

Yes, of course.

It should replace each quotation mark in a string by two of this character. (Quotes should become duplicated.)

Which elements would you prefer to reuse from the programming language "OCaml" for the shown use case?

Regards,
Markus

The string in question is the file name from a position variable. A file name does not contain a quote character. When you print a string, it appears as eg "foo", but the only characters in the string are f o and o. If you want to put a quote character before and after, The simplest would
be: Printf.sprintf "\"%s\"" place.file.

julia

I would prefer to handle also the general case that such strings contain quotation marks eventually.

It depends on the analysis area if corresponding safety checks for special characters will matter, doesn't it?

Other example:
https://github.com/coccinelle/coccinelle/issues/5

Regards,
Markus

Why would a file name contain a quote character. And if it does, what good will adding exxtra ones do you. If the name is

foo\"bar

you will have

"foo\""bar"

and your string will be misparsed by your database.

julia

I do not really know how often this case happens in the world of detailed data processing.

I hope to ensure constraints for the used data formats.

I do not think so. I try to achieve quoting by the discussed function implementation so that a column/field in a CSV file will be generally treated as a text string.

Regards,
Markus

I am trying out a bit more preparation with OCaml script development for this report variant.

@initialize:ocaml@
@@
module My_map = Map.Make(Int64)

let m = ref My_map.empty

let counting dir =
let d = Unix.opendir dir in
try while true do
let name = Unix.readdir d in
let length = Int64.of_int (String.length name) in

if My_map.mem length (! m) then
ignore (m := My_map.add length (Int64.add (My_map.find length (! m))
Int64.one) (! m))

        else
           ignore (m := My_map.add length Int64.one (! m))
        done
    with End_of_file -> Unix.closedir d

@find@
identifier fu;
@@
*fu(...)
{ ... }

@script:ocaml collection@
area << virtual.folder;
@@
if area = "" then
counting "/"
else
counting area

@finalize:ocaml@
@@
if My_map.is_empty (! m) then
prerr_endline "No result for this analysis!"
else
let delimiter = '|' in
let output key count = Printf.printf "%Li%c%Li\r\n" key delimiter count in Printf.printf "length%cincidence\r\n" delimiter;
My_map.iter output (! m)

elfring@Sonne:~/Projekte/Coccinelle/janitor> spatch.opt --sp-file list_file_name_lengths1.cocci ../Probe/f-ptr-test1.c
init_defs_builtins: /usr/local/share/coccinelle/standard.h Using native version of ocamlc/ocamlopt/ocamldep ocamlopt.opt -shared -o /tmp/list_file_name_lengths1f3bae5.cmxs -g -I /usr/lib64/ocaml -I /usr/local/share/coccinelle/ocaml /tmp/list_file_name_lengths1f3bae5.ml
Compilation OK!
Loading ML code of the SP...
HANDLING: ../Probe/f-ptr-test1.c
...
No result for this analysis!

How do you think about such a semantic patch approach?

Regards,
Markus

I have got another concern for a corresponding implementation detail. How can a constraint be specified for the filter in the semantic patch so that the expression refers only to non-volatile data?
Is it a software development challenge to exclude accesses on volatile data elements eventually from the suggested deletion of condition checks?
https://en.wikipedia.org/wiki/Volatile_variable#In_C_and_C.2B.2B

Regards,
Markus

Perhaps it is possible to write:

@@
volatile x;
@@

* x == NULL

Or maybe

@@
type T;
volatile T x;
@@

* x == NULL

Or at worst:

@@
type T;
identifier i;
@@

volatile T i;
...
*i == NULL

I don't know if this would find much. Intuitively, one might expect such problems to be detected easily by testing or developers to know better in thw first place, but who knows.

julia

I see the software development challenge in the analysis of the expression which is used in a condition check. Such an expression could be constructed out of several elements.
I am still unsure on how a data type property like "volatile" can be safely determined by the semantic patch language for each used element. I would be able to specify a filter for a single element as you suggest. But is the combination of some elements more interesting for the general use case?

Regards,
Markus

Coccinelle makes an effort to infer types. YOu may need to use eg --recursive-includes and --relax-include-path to get the most possible type information.

julia

It is nice when a simple filter pattern works in principle. A filter pattern which needs the knowledge about all data type details to consider relevant constraints will need more time for the data analysis. I hope that the corresponding execution speed can also be improved.

Regards,
Markus

How do you think about to make it possible to check attributes for expressions in SmPL constraints?

Regards,
Markus

I tried to do this at one point, but it introduced a lot of parsing problems, because attributes can appear in a variety of places. The benefit didn't seem to be worth the risk.

julia

Which kind of syntax had you got in mind for such a functionality?

Is there a need to distinguish property checks from support for extensions from a popular compiler implementation?
http://gcc.gnu.org/onlinedocs/gcc-4.8.2/gcc/Type-Attributes.html

Can we clarify your concerns a bit more here?

Regards,
Markus

To my recollection, there were problems of ambiguity in the parser. Also, I think it made the abstract syntax tree unwieldy, because attributes had to be taken into account in many positions.

julia

Would you like to give it another try?

Regards,
Markus

It seems that you could fix this issue with the software release "1.0.0-rc22".

I have tried out the discussed approach on the source files for the operating system "Linux 3.16.3" once more. Now I stumble on another error message.

elfring at Sonne:~/Projekte/Coccinelle/janitor> spatch.opt --version && OCAMLRUNPARAM=b spatch.opt --sp-file list_functions_with_unnecessary_checks1.cocci /usr/src/linux-stable/crypto/fcrypt.c spatch version 1.0.0-rc22 with Python support and with PCRE support
init_defs_builtins: /usr/local/share/coccinelle/standard.h
HANDLING: /usr/src/linux-stable/crypto/fcrypt.c
Fatal error: exception Failure("lexing: empty token")
Raised at file "lexing.ml", line 65, characters 15-37

Can this detail also be improved?

Regards,
Markus

Hello,

A couple of functions perform input parameter validation before their implementations will try further actions with side effects. Some calling functions perform similar safety checks.

Functions which release a system resource are occasionally documented in the way that they tolerate the passing of a null pointer for example.
I do not see a need because of this fact that a function caller repeats a corresponding check.

Now I would like to propose such a change again.

1. Extension of the infrastructure for the analysis tool "coccicheck" Semantic patch patterns can help to identify update candidates also in the
Linux source file hierarchy.

https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/scripts/coccinelle?id=79f0345fefaafb7cde301a830471edd21a37989b

Would you like to reconsider an approach which was discussed with a subject
like "scripts/coccinelle/free: Delete NULL test before freeing functions?" a while ago?
https://lkml.org/lkml/2014/8/9/36
https://groups.google.com/d/msg/linux.kernel/rIWfYsRRW6I/cTs6y0STf2cJ

2. Clarification for some automated update suggestions
My source code search approach found 227 functions with the help of the
software "Coccinelle 1.0.0-rc22" at least which might need another review and corresponding corrections for Linux 3.16.3. Further software development will point out even more potentially open issues.

Regards,
Markus

From: Markus Elfring <[email protected]>
Date: Wed, 5 Mar 2014 18:15:34 +0100
Subject: [PATCH 1/5] Addition of a semantic patch file for showing unnecessary
checks before a few known functions

This semantic patch pattern tries to find source code places where a check is performed for an expression that is passed to a function (like "kfree") which checks this single parameter itself for usability. Redundant value or pointer checks can be avoided here.

The pattern contains a special comment in a regular expression for a SmPL constraint which supports extensions.

---
 .../deletions/delete_unnecessary_checks_template1.cocci     | 13 +++++++++++++
 1 file changed, 13 insertions(+)
 create mode 100644

scripts/coccinelle/deletions/delete_unnecessary_checks_template1.cocci

diff --git a/scripts/coccinelle/deletions/delete_unnecessary_checks_template1.cocci b/scripts/coccinelle/deletions/delete_unnecessary_checks_template1.cocci new file mode 100644
index 0000000..b092051

@@ -0,0 +1,13 @@
+ at Delete_unnecessary_checks@
+expression x;
+identifier release =~ "^(?x)
+(?:
+   (?:kz?|slob_)free
+|
+   (?:
+# Alternation placeholder
+   )
+)$";
+@@
+-if (x)
+    release(x);

1.9.0

From: Markus Elfring <[email protected]>
Date: Wed, 5 Mar 2014 18:38:43 +0100
Subject: [PATCH 2/5] Addition of a semantic patch file for listing of
functions that check their single parameter

This semantic patch pattern tries to find functions that check their single parameter for usability.

Example:
Null pointer checks are often performed as input parameter validation.

It uses Python statements to write information about the found source code places in a data format that is a variant of a CSV text file.

---
 .../list_input_parameter_validation1.cocci         | 55 ++++++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644

scripts/coccinelle/deletions/list_input_parameter_validation1.cocci

b/scripts/coccinelle/deletions/list_input_parameter_validation1.cocci new file mode 100644
index 0000000..b0a5a52

'"source file"', "line", "column")))
+ print("\r\n".join(result))
+else:
+ sys.stderr.write("No result for this analysis!\n")

1.9.0

From: Markus Elfring <[email protected]>
Date: Wed, 5 Mar 2014 18:58:30 +0100
Subject: [PATCH 3/5] Addition of a SQLite script for a text file import

A script was added so that a text file which was previously generated can be imported into a SQLite data base table.
http://sqlite.org/sqlite.html

The shown file name can be adjusted by a make file for example.

---
 .../coccinelle/deletions/handle_function_list_template.sqlite    | 9 +++++++++
 1 file changed, 9 insertions(+)
 create mode 100644

scripts/coccinelle/deletions/handle_function_list_template.sqlite

b/scripts/coccinelle/deletions/handle_function_list_template.sqlite new file mode 100644
index 0000000..bec366c

@@ -0,0 +1,9 @@
+create table positions(function text,
+                       data_type text,
+                       parameter text,
+                       source_file text,
+                       line integer,
+                       column integer);
+.separator "|"
+.import list_input_pointer_validation1.txt positions
+.header OFF

1.9.0

From: Markus Elfring <[email protected]>
Date: Wed, 5 Mar 2014 19:10:32 +0100
Subject: [PATCH 4/5] Addition of a semantic patch file for listing of
unnecessary checks before a few known functions

This semantic patch pattern tries to find source code places where a check is performed for an expression that is passed to a function (like "kfree") which checks this single parameter itself for usability. Redundant value or pointer checks can be avoided here.

The pattern contains a special comment in a regular expression for a SmPL constraint which supports extensions.

It uses Python statements to write information about the found places in a data format that is a variant of a CSV text file.

---
 ...nctions_with_unnecessary_checks_template1.cocci | 59 ++++++++++++++++++++++
 1 file changed, 59 insertions(+)
 create mode 100644

scripts/coccinelle/deletions/list_functions_with_unnecessary_checks_template1.cocci

diff --git a/scripts/coccinelle/deletions/list_functions_with_unnecessary_checks_template1.cocci b/scripts/coccinelle/deletions/list_functions_with_unnecessary_checks_template1.cocci new file mode 100644
index 0000000..d2637e8
--- /dev/null
+++
b/scripts/coccinelle/deletions/list_functions_with_unnecessary_checks_template1.cocci @@ -0,0 +1,59 @@
+ at initialize:python@
+@@
+import sys
+result = []
+mark = ['"', '', '"']
+delimiter = '|'
+
+def store_positions(fun, point, places):
+ """Add source code positions to an internal list."""
+ for place in places:
+ fields = []
+ fields.append(fun)
+
+ fields.append(point)
+
+ mark[1] = place.file.replace('"', '""')
+ fields.append(''.join(mark))
+

+        fields.append(place.line)
+        fields.append(str(int(place.column) + 1))
+        result.append(delimiter.join(fields))

+
+ at is_unnecessary_check@
+expression data;
+identifier work;
+identifier release =~ "^(?x)
+(?:
+ (?:kz?|slob_)free
+|
+ (?:
+# Alternation placeholder
+ )
+)$";
+position pos;
+type t;
+@@
+ t work at pos(...)
+ {
+ ... when any
+( if (data) release(data);
+| if (likely(data)) release(data);
+)
+ ... when any
+ }
+
+ at script:python collection depends on is_unnecessary_check@ +fun << is_unnecessary_check.work;
+point << is_unnecessary_check.data;
+places << is_unnecessary_check.pos;
+@@
+store_positions(fun, point, places)
+
+@finalize:python@
+@@
+if result:
+ result.insert(0, delimiter.join(("function", '"parameter"', '"source file"',
"line", "column")))
+ print("\r\n".join(result))
+else:
+ sys.stderr.write("No result for this analysis!\n")

1.9.0

From: Markus Elfring <[email protected]>
Date: Wed, 5 Mar 2014 19:40:43 +0100
Subject: [PATCH 5/5] Addition of a make file for build automation

This script can be used to combine some input files for the desired data output which will eventually show update candidates for further source code review. Some build targets were defined. Values for the used make variables can be adjusted by parameters on the command line as usual.

A few implementation details might need more fine-tuning.
- Automatic determination of the Linux source directory from a calling make process

- Setting of an extra output directory for the generated files

---
 scripts/coccinelle/deletions/makefile | 126 ++++++++++++++++++++++++++++++++++
 1 file changed, 126 insertions(+)
 create mode 100644 scripts/coccinelle/deletions/makefile

b/scripts/coccinelle/deletions/makefile
new file mode 100644
index 0000000..6464bae

@@ -0,0 +1,126 @@
+SED=sed
+SPATCH=spatch.opt --sp-file
+RM=rm -f
+LINUX_SOURCE_DIR=/usr/src/linux-stable
+EXTRACT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER=list_input_parameter_validation1.cocci
+SQLITE=sqlite3
+SQLITE_IMPORT_SCRIPT=handle_function_list.sqlite
+SQLITE_IMPORT_SCRIPT_TEMPLATE=handle_function_list_template.sqlite
+RESULT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER=list_input_parameter_validation1.txt
+RESULT_SQL_DATA_BASE=result.db
+RESULT_FUNCTIONS_WITH_PREFIX=add_prefix-SQL.txt
+RESULT_FUNCTIONS_WITH_UNNECESSARY_CHECKS_AS_LIST=list_functions_with_unnecessary_checks1.txt
+RESULT_FUNCTIONS_WITH_UNNECESSARY_CHECKS_AS_PATCH=functions_with_unnecessary_checks1.diff
+LOG_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER=list_input_parameter_validation1-errors.txt
+LOG_LIST_FUNCTIONS_WITH_UNNECESSARY_CHECKS=list_functions_with_unnecessary_checks1-errors.txt
+LOG_PATCH_FUNCTIONS_WITH_UNNECESSARY_CHECKS=functions_with_unnecessary_checks1-errors.txt
+LIST_PATTERN_TEMPLATE=list_functions_with_unnecessary_checks_template1.cocci
+LIST_PATTERN=list_functions_with_unnecessary_checks1.cocci
+PATCH_PATTERN_TEMPLATE=delete_unnecessary_checks_template1.cocci
+PATCH_PATTERN=delete_unnecessary_checks1.cocci
+ESCAPING=XY=$$(< $(RESULT_FUNCTIONS_WITH_PREFIX)) \
+         && XY=$${XY/|/ } \
+         && XY=$${XY//%/\\%} \
+         && $(SED) "s%\# Alternation placeholder%$${XY//$$'\n'/$$'\\\\\n'}%"
+TEXT1=A pattern file was built from which a
+TEXT2=was generated. Good luck with source code review!
+
+default: build_update_candidate_list
+
+$(RESULT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER): \
+$(EXTRACT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER)
+	$(SPATCH) $(EXTRACT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER) \
+-dir $(LINUX_SOURCE_DIR) \
+> $@ 2> $(LOG_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER)
+
+# This replacement action is needed for the use case that the corresponding
+# variable was overridden.
+$(SQLITE_IMPORT_SCRIPT): $(SQLITE_IMPORT_SCRIPT_TEMPLATE)
+	$(SED) "s%import list_input_pointer_validation1\.txt%import $(subst

%,\%,$(RESULT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER))%" \
+$(SQLITE_IMPORT_SCRIPT_TEMPLATE) > $@
+
+$(RESULT_SQL_DATA_BASE) $(RESULT_FUNCTIONS_WITH_PREFIX): \
+$(RESULT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER) \
+$(SQLITE_IMPORT_SCRIPT)
+ @$(RM) $(RESULT_SQL_DATA_BASE)
+ $(SQLITE) -init $(SQLITE_IMPORT_SCRIPT) $(RESULT_SQL_DATA_BASE) \
+'select '\'' | '\'' || function from positions group by function order by
function desc;' \
+> $(RESULT_FUNCTIONS_WITH_PREFIX)
+
+$(LIST_PATTERN): $(RESULT_FUNCTIONS_WITH_PREFIX)
+ $(ESCAPING) $(LIST_PATTERN_TEMPLATE) > $@
+
+$(PATCH_PATTERN): $(RESULT_FUNCTIONS_WITH_PREFIX)
+ $(ESCAPING) $(PATCH_PATTERN_TEMPLATE) > $@
+
+$(RESULT_FUNCTIONS_WITH_UNNECESSARY_CHECKS_AS_LIST): $(LIST_PATTERN)
+ $(SPATCH) $(LIST_PATTERN) -dir $(LINUX_SOURCE_DIR) \
+> $@ 2> $(LOG_LIST_FUNCTIONS_WITH_UNNECESSARY_CHECKS)
+
+$(RESULT_FUNCTIONS_WITH_UNNECESSARY_CHECKS_AS_PATCH): $(PATCH_PATTERN)
+ $(SPATCH) $(PATCH_PATTERN) -dir $(LINUX_SOURCE_DIR) \
+> $@ 2> $(LOG_PATCH_FUNCTIONS_WITH_UNNECESSARY_CHECKS)
+
+build_check_list generate_list_of_functions_which_check_their_single_parameter: \
+$(RESULT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER)
+ @echo 'The list of functions which check their single parameter was generated.'
+
+build_import_script: $(SQLITE_IMPORT_SCRIPT)
+ @echo 'A script was generated which should contain appropriate parameters for
a data base.'
+
+build_data_base: $(RESULT_SQL_DATA_BASE)
+ @echo 'A SQL data base was built.'
+
+build_alternation add_prefix_to_functions: build_data_base
+ @echo 'The function name list was converted to a component for a regular
expression.'
+
+build_list_pattern: $(LIST_PATTERN)
+ @echo '$(TEXT1) list can be generated.'
+
+build_patch_pattern: $(PATCH_PATTERN)
+ @echo '$(TEXT1) patch can be generated.'
+
+build_update_candidate_list show_list_of_update_candidates: build_list_pattern \
+$(RESULT_FUNCTIONS_WITH_UNNECESSARY_CHECKS_AS_LIST)
+ @echo 'The list of update candidates $(TEXT2)'
+
+build_patch show_update_suggestion: build_patch_pattern \
+$(RESULT_FUNCTIONS_WITH_UNNECESSARY_CHECKS_AS_PATCH)
+ @echo 'A patch file $(TEXT2)'
+
+all: build_update_candidate_list build_patch
+
+clean:
+ $(RM) *-errors.txt \
+$(LIST_PATTERN) \
+$(PATCH_PATTERN) \
+$(RESULT_FUNCTIONS_WITH_PREFIX) \
+$(RESULT_SQL_DATA_BASE) \
+$(RESULT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER) \
+$(SQLITE_IMPORT_SCRIPT)
+
+delete_data_base:
+ $(RM) $(RESULT_SQL_DATA_BASE) \
+$(RESULT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER)
+
+.PHONY: all \
+build_check_list \
+build_data_base \
+build_import_script \
+build_list_pattern \
+build_patch \
+build_patch_pattern \
+build_update_candidate_list \
+clean \
+default \
+delete_data_base \
+generate_list_of_functions_which_check_their_single_parameter \
+show_list_of_update_candidates \
+show_update_suggestion
+
+
+# The following input files should not need further actions here. +$(EXTRACT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER) \
+$(SQLITE_IMPORT_SCRIPT_TEMPLATE) \
+$(LIST_PATTERN_TEMPLATE) \
+$(PATCH_PATTERN_TEMPLATE): ;

1.9.0

Would you like to integrate the following proposal into your source code repository?

Regards,
Markus

From: Markus Elfring <[email protected]>
Date: Wed, 22 Oct 2014 15:45:22 +0200
Subject: [PATCH] GPU-DRM-nouveau: Deletion of unnecessary checks before two
function calls

A semantic patch approach was proposed with the subject "[PATCH with Coccinelle?] Deletion of unnecessary checks before specific function calls" on 2014-03-05.
https://lkml.org/lkml/2014/3/5/344
http://article.gmane.org/gmane.comp.version-control.coccinelle/3513/

This patch pattern application was repeated with the help of the software "Coccinelle 1.0.0-rc22" on the source files for Linux 3.17.1. An extract of the automatically generated update suggestions is shown here.

It was determined that the affected source code places call functions which perform input parameter validation already. It is therefore not needed that a similar safety check is repeated at the call site.

---
 drivers/gpu/drm/nouveau/core/core/handle.c | 3 +--
 drivers/gpu/drm/nouveau/nouveau_drm.c      | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

b/drivers/gpu/drm/nouveau/core/core/handle.c
index a490b80..75d0c2c 100644

@@ -219,8 +219,7 @@ nouveau_handle_get_cinst(struct nouveau_object *engctx, u32

cinst)
void
nouveau_handle_put(struct nouveau_handle *handle) {
- if (handle)
- nouveau_namedb_put(handle);
+ nouveau_namedb_put(handle);
}

int

b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 5723807..5c29079 100644

@@ -512,8 +512,7 @@ nouveau_drm_unload(struct drm_device *dev)
 	nouveau_vga_fini(drm);
 
 	nvif_device_fini(&drm->device);
-	if (drm->hdmi_device)
-		pci_dev_put(drm->hdmi_device);
+	pci_dev_put(drm->hdmi_device);
 	nouveau_cli_destroy(&drm->client);
 	return 0;
 }

2.1.2

Would you like to integrate the following proposal into your source code repository?

Regards,
Markus

From: Markus Elfring <[email protected]>
Date: Wed, 22 Oct 2014 18:28:12 +0200
Subject: [PATCH] GPU-DRM-GMA500: Deletion of unnecessary checks before two
function calls

A semantic patch approach was proposed with the subject "[PATCH with Coccinelle?] Deletion of unnecessary checks before specific function calls" on 2014-03-05.
https://lkml.org/lkml/2014/3/5/344
http://article.gmane.org/gmane.comp.version-control.coccinelle/3513/

This patch pattern application was repeated with the help of the software "Coccinelle 1.0.0-rc22" on the source files for Linux 3.17.1. An extract of the automatically generated update suggestions is shown here.

It was determined that the affected source code places call functions which perform input parameter validation already. It is therefore not needed that a similar safety check is repeated at the call site.

---
 drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 3 +--
 drivers/gpu/drm/gma500/cdv_intel_lvds.c | 9 +++------
 drivers/gpu/drm/gma500/oaktrail_lvds.c  | 3 +--
 drivers/gpu/drm/gma500/psb_drv.c        | 3 +--
 drivers/gpu/drm/gma500/psb_intel_lvds.c | 9 +++------
 5 files changed, 9 insertions(+), 18 deletions(-)

b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
index 4268bf2..0d69624 100644

@@ -246,8 +246,7 @@ static void cdv_hdmi_destroy(struct drm_connector *connector)
 {
 	struct gma_encoder *gma_encoder = gma_attached_encoder(connector);
 
-	if (gma_encoder->i2c_bus)
-		psb_intel_i2c_destroy(gma_encoder->i2c_bus);
+	psb_intel_i2c_destroy(gma_encoder->i2c_bus);
 	drm_connector_unregister(connector);
 	drm_connector_cleanup(connector);
 	kfree(connector);

b/drivers/gpu/drm/gma500/cdv_intel_lvds.c
index 0b77039..8f24013 100644

@@ -444,8 +444,7 @@ static void cdv_intel_lvds_destroy(struct drm_connector

*connector)
{
struct gma_encoder *gma_encoder = gma_attached_encoder(connector);

- if (gma_encoder->i2c_bus)
- psb_intel_i2c_destroy(gma_encoder->i2c_bus);
+ psb_intel_i2c_destroy(gma_encoder->i2c_bus);
drm_connector_unregister(connector);
drm_connector_cleanup(connector);
kfree(connector);
@@ -780,12 +779,10 @@ out:
failed_find:
mutex_unlock(&dev->mode_config.mutex);
printk(KERN_ERR "Failed find\n");
- if (gma_encoder->ddc_bus)
- psb_intel_i2c_destroy(gma_encoder->ddc_bus);
+ psb_intel_i2c_destroy(gma_encoder->ddc_bus);
failed_ddc:
printk(KERN_ERR "Failed DDC\n");
- if (gma_encoder->i2c_bus)
- psb_intel_i2c_destroy(gma_encoder->i2c_bus);
+ psb_intel_i2c_destroy(gma_encoder->i2c_bus);
failed_blc_i2c:
printk(KERN_ERR "Failed BLC\n");
drm_encoder_cleanup(encoder);

b/drivers/gpu/drm/gma500/oaktrail_lvds.c
index 0d39da6..49c5c415 100644

@@ -411,8 +411,7 @@ failed_find:
 	mutex_unlock(&dev->mode_config.mutex);
 
 	dev_dbg(dev->dev, "No LVDS modes found, disabling.\n");
-	if (gma_encoder->ddc_bus)
-		psb_intel_i2c_destroy(gma_encoder->ddc_bus);
+	psb_intel_i2c_destroy(gma_encoder->ddc_bus);
 
 /* failed_ddc: */
 

index 6ec3a90..0efe165 100644
@@ -210,8 +210,7 @@ static int psb_driver_unload(struct drm_device *dev)
 			iounmap(dev_priv->aux_reg);
 			dev_priv->aux_reg = NULL;
 		}
-		if (dev_priv->aux_pdev)
-			pci_dev_put(dev_priv->aux_pdev);
+		pci_dev_put(dev_priv->aux_pdev);
 
 		/* Destroy VBT data */
 		psb_intel_destroy_bios(dev);

b/drivers/gpu/drm/gma500/psb_intel_lvds.c
index 88aad95..e73c3f9 100644

@@ -561,8 +561,7 @@ void psb_intel_lvds_destroy(struct drm_connector *connector)
 	struct gma_encoder *gma_encoder = gma_attached_encoder(connector);
 	struct psb_intel_lvds_priv *lvds_priv = gma_encoder->dev_priv;
 
-	if (lvds_priv->ddc_bus)
-		psb_intel_i2c_destroy(lvds_priv->ddc_bus);
+	psb_intel_i2c_destroy(lvds_priv->ddc_bus);
 	drm_connector_unregister(connector);
 	drm_connector_cleanup(connector);
 	kfree(connector);

@@ -834,11 +833,9 @@ out:
 
 failed_find:
 	mutex_unlock(&dev->mode_config.mutex);
-	if (lvds_priv->ddc_bus)
-		psb_intel_i2c_destroy(lvds_priv->ddc_bus);
+	psb_intel_i2c_destroy(lvds_priv->ddc_bus);
 failed_ddc:
-	if (lvds_priv->i2c_bus)
-		psb_intel_i2c_destroy(lvds_priv->i2c_bus);
+	psb_intel_i2c_destroy(lvds_priv->i2c_bus);
 failed_blc_i2c:
 	drm_encoder_cleanup(encoder);
 	drm_connector_cleanup(connector);

2.1.2

From: Markus Elfring <[email protected]>
Date: Wed, 22 Oct 2014 19:39:21 +0200
Subject: [PATCH] IOMMU-MSM: Deletion of unnecessary checks before the function
call "clk_disable"

A semantic patch approach was proposed with the subject "[PATCH with Coccinelle?] Deletion of unnecessary checks before specific function calls" on 2014-03-05.
https://lkml.org/lkml/2014/3/5/344
http://article.gmane.org/gmane.comp.version-control.coccinelle/3513/

This patch pattern application was repeated with the help of the software "Coccinelle 1.0.0-rc22" on the source files for Linux 3.17.1. An extract of the automatically generated update suggestions is shown here.

It was determined that the affected source code places call functions which perform input parameter validation already. It is therefore not needed that a similar safety check is repeated at the call site.

---
 drivers/iommu/msm_iommu.c     | 3 +--
 drivers/iommu/msm_iommu_dev.c | 6 ++----
 2 files changed, 3 insertions(+), 6 deletions(-)

index 6e3dcc28..3e4d888 100644
@@ -73,8 +73,7 @@ fail:
 
 static void __disable_clocks(struct msm_iommu_drvdata *drvdata)
 {
-	if (drvdata->clk)
-		clk_disable(drvdata->clk);
+	clk_disable(drvdata->clk);
 	clk_disable(drvdata->pclk);
 }
 

index 61def7cb..9574d21 100644
@@ -224,8 +224,7 @@ static int msm_iommu_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, drvdata);
 
-	if (iommu_clk)
-		clk_disable(iommu_clk);
+	clk_disable(iommu_clk);
 
 	clk_disable(iommu_pclk);
 

@@ -323,8 +322,7 @@ static int msm_iommu_ctx_probe(struct platform_device *pdev)
 		SET_NSCFG(drvdata->base, mid, 3);
 	}
 
-	if (drvdata->clk)
-		clk_disable(drvdata->clk);
+	clk_disable(drvdata->clk);
 	clk_disable(drvdata->pclk);
 
 	dev_info(&pdev->dev, "context %s using bank %d\n", c->name, c->num);

2.1.2

From: Markus Elfring <[email protected]>
Date: Wed, 22 Oct 2014 20:40:31 +0200
Subject: [PATCH] SCSI-QLA2XXX: Deletion of unnecessary checks before the
function call "vfree"

A semantic patch approach was proposed with the subject "[PATCH with Coccinelle?] Deletion of unnecessary checks before specific function calls" on 2014-03-05.
https://lkml.org/lkml/2014/3/5/344
http://article.gmane.org/gmane.comp.version-control.coccinelle/3513/

This patch pattern application was repeated with the help of the software "Coccinelle 1.0.0-rc22" on the source files for Linux 3.17.1. An extract of the automatically generated update suggestions is shown here.

It was determined that the affected source code places call functions which perform input parameter validation already. It is therefore not needed that a similar safety check is repeated at the call site.

---
 drivers/scsi/qla2xxx/qla_attr.c |  6 ++----
 drivers/scsi/qla2xxx/qla_init.c | 18 ++++++------------
 drivers/scsi/qla2xxx/qla_os.c   |  6 ++----
 3 files changed, 10 insertions(+), 20 deletions(-)

index 82b92c4..95c4c09 100644
@@ -175,10 +175,8 @@ qla2x00_sysfs_write_fw_dump_template(struct file *filp,

struct kobject *kobj,
uint32_t size;

if (off == 0) {
- if (ha->fw_dump)
- vfree(ha->fw_dump);
- if (ha->fw_dump_template)
- vfree(ha->fw_dump_template);
+ vfree(ha->fw_dump);
+ vfree(ha->fw_dump_template);

ha->fw_dump = NULL;
ha->fw_dump_len = 0;

index a4dde7e..8da3d4f 100644
@@ -5256,8 +5256,7 @@ qla24xx_load_risc_flash(scsi_qla_host_t *vha, uint32_t

*srisc_addr,
if (!IS_QLA27XX(ha))
return rval;

- if (ha->fw_dump_template)
- vfree(ha->fw_dump_template);
+ vfree(ha->fw_dump_template);
ha->fw_dump_template = NULL;
ha->fw_dump_template_len = 0;

@@ -5307,8 +5306,7 @@ qla24xx_load_risc_flash(scsi_qla_host_t *vha, uint32_t *srisc_addr,

default_template:
ql_log(ql_log_warn, vha, 0x0168, "Using default fwdump template\n");
- if (ha->fw_dump_template)
- vfree(ha->fw_dump_template);
+ vfree(ha->fw_dump_template);
ha->fw_dump_template = NULL;
ha->fw_dump_template_len = 0;

@@ -5342,8 +5340,7 @@ default_template:

failed_template:
ql_log(ql_log_warn, vha, 0x016d, "Failed default fwdump template\n");
- if (ha->fw_dump_template)
- vfree(ha->fw_dump_template);
+ vfree(ha->fw_dump_template);
ha->fw_dump_template = NULL;
ha->fw_dump_template_len = 0;
return rval;
@@ -5559,8 +5556,7 @@ qla24xx_load_risc_blob(scsi_qla_host_t *vha, uint32_t *srisc_addr)
if (!IS_QLA27XX(ha))
return rval;

- if (ha->fw_dump_template)
- vfree(ha->fw_dump_template);
+ vfree(ha->fw_dump_template);
ha->fw_dump_template = NULL;
ha->fw_dump_template_len = 0;

@@ -5609,8 +5605,7 @@ qla24xx_load_risc_blob(scsi_qla_host_t *vha, uint32_t *srisc_addr)

default_template:
ql_log(ql_log_warn, vha, 0x0178, "Using default fwdump template\n");
- if (ha->fw_dump_template)
- vfree(ha->fw_dump_template);
+ vfree(ha->fw_dump_template);
ha->fw_dump_template = NULL;
ha->fw_dump_template_len = 0;

@@ -5644,8 +5639,7 @@ default_template:

failed_template:
ql_log(ql_log_warn, vha, 0x017d, "Failed default fwdump template\n");
- if (ha->fw_dump_template)
- vfree(ha->fw_dump_template);
+ vfree(ha->fw_dump_template);
ha->fw_dump_template = NULL;
ha->fw_dump_template_len = 0;
return rval;

index db3dbd9..0f9c378 100644
@@ -3676,10 +3676,8 @@ qla2x00_free_fw_dump(struct qla_hw_data *ha)
 		dma_free_coherent(&ha->pdev->dev,
 		    EFT_SIZE, ha->eft, ha->eft_dma);
 
-	if (ha->fw_dump)
-		vfree(ha->fw_dump);
-	if (ha->fw_dump_template)
-		vfree(ha->fw_dump_template);
+	vfree(ha->fw_dump);
+	vfree(ha->fw_dump_template);
 
 	ha->fce = NULL;
 	ha->fce_dma = 0;

2.1.2

I resent the request once more because another "Triple-X" software development adventure might follow ...?

Regards,
Markus

From: Markus Elfring <[email protected]>
Date: Wed, 22 Oct 2014 20:40:31 +0200
Subject: [PATCH] SCSI-QLA2XXX: Deletion of unnecessary checks before the
function call "vfree"

A semantic patch approach was proposed with the subject "[PATCH with Coccinelle?] Deletion of unnecessary checks before specific function calls" on 2014-03-05.
https://lkml.org/lkml/2014/3/5/344
http://article.gmane.org/gmane.comp.version-control.coccinelle/3513/

This patch pattern application was repeated with the help of the software "Coccinelle 1.0.0-rc22" on the source files for Linux 3.17.1. An extract of the automatically generated update suggestions is shown here.

It was determined that the affected source code places call functions which perform input parameter validation already. It is therefore not needed that a similar safety check is repeated at the call site.

---
 drivers/scsi/qla2xxx/qla_attr.c |  6 ++----
 drivers/scsi/qla2xxx/qla_init.c | 18 ++++++------------
 drivers/scsi/qla2xxx/qla_os.c   |  6 ++----
 3 files changed, 10 insertions(+), 20 deletions(-)

index 82b92c4..95c4c09 100644
@@ -175,10 +175,8 @@ qla2x00_sysfs_write_fw_dump_template(struct file *filp,

struct kobject *kobj,
uint32_t size;

if (off == 0) {
- if (ha->fw_dump)
- vfree(ha->fw_dump);
- if (ha->fw_dump_template)
- vfree(ha->fw_dump_template);
+ vfree(ha->fw_dump);
+ vfree(ha->fw_dump_template);

ha->fw_dump = NULL;
ha->fw_dump_len = 0;

index a4dde7e..8da3d4f 100644
@@ -5256,8 +5256,7 @@ qla24xx_load_risc_flash(scsi_qla_host_t *vha, uint32_t

*srisc_addr,
if (!IS_QLA27XX(ha))
return rval;

- if (ha->fw_dump_template)
- vfree(ha->fw_dump_template);
+ vfree(ha->fw_dump_template);
ha->fw_dump_template = NULL;
ha->fw_dump_template_len = 0;

@@ -5307,8 +5306,7 @@ qla24xx_load_risc_flash(scsi_qla_host_t *vha, uint32_t *srisc_addr,

default_template:
ql_log(ql_log_warn, vha, 0x0168, "Using default fwdump template\n");
- if (ha->fw_dump_template)
- vfree(ha->fw_dump_template);
+ vfree(ha->fw_dump_template);
ha->fw_dump_template = NULL;
ha->fw_dump_template_len = 0;

@@ -5342,8 +5340,7 @@ default_template:

failed_template:
ql_log(ql_log_warn, vha, 0x016d, "Failed default fwdump template\n");
- if (ha->fw_dump_template)
- vfree(ha->fw_dump_template);
+ vfree(ha->fw_dump_template);
ha->fw_dump_template = NULL;
ha->fw_dump_template_len = 0;
return rval;
@@ -5559,8 +5556,7 @@ qla24xx_load_risc_blob(scsi_qla_host_t *vha, uint32_t *srisc_addr)
if (!IS_QLA27XX(ha))
return rval;

- if (ha->fw_dump_template)
- vfree(ha->fw_dump_template);
+ vfree(ha->fw_dump_template);
ha->fw_dump_template = NULL;
ha->fw_dump_template_len = 0;

@@ -5609,8 +5605,7 @@ qla24xx_load_risc_blob(scsi_qla_host_t *vha, uint32_t *srisc_addr)

default_template:
ql_log(ql_log_warn, vha, 0x0178, "Using default fwdump template\n");
- if (ha->fw_dump_template)
- vfree(ha->fw_dump_template);
+ vfree(ha->fw_dump_template);
ha->fw_dump_template = NULL;
ha->fw_dump_template_len = 0;

@@ -5644,8 +5639,7 @@ default_template:

failed_template:
ql_log(ql_log_warn, vha, 0x017d, "Failed default fwdump template\n");
- if (ha->fw_dump_template)
- vfree(ha->fw_dump_template);
+ vfree(ha->fw_dump_template);
ha->fw_dump_template = NULL;
ha->fw_dump_template_len = 0;
return rval;

index db3dbd9..0f9c378 100644
@@ -3676,10 +3676,8 @@ qla2x00_free_fw_dump(struct qla_hw_data *ha)
 		dma_free_coherent(&ha->pdev->dev,
 		    EFT_SIZE, ha->eft, ha->eft_dma);
 
-	if (ha->fw_dump)
-		vfree(ha->fw_dump);
-	if (ha->fw_dump_template)
-		vfree(ha->fw_dump_template);
+	vfree(ha->fw_dump);
+	vfree(ha->fw_dump_template);
 
 	ha->fce = NULL;
 	ha->fce_dma = 0;

2.1.2

What platforms have you tested the code on at this point ?

Alan

Applied to arm/msm, thanks.

From: Markus Elfring <[email protected]>
Date: Thu, 23 Oct 2014 20:55:13 +0200
Subject: [PATCH] staging - rtl8188eu: Deletion of unnecessary checks before
three function calls

The functions kfree(), rtw_free_netdev() and vfree() test whether their argument is NULL and then return immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/staging/rtl8188eu/core/rtw_efuse.c   | 3 +--
 drivers/staging/rtl8188eu/core/rtw_mlme.c    | 3 +--
 drivers/staging/rtl8188eu/core/rtw_sta_mgt.c | 3 +--
 drivers/staging/rtl8188eu/core/rtw_xmit.c    | 6 ++----
 drivers/staging/rtl8188eu/os_dep/usb_intf.c  | 5 ++---
 5 files changed, 7 insertions(+), 13 deletions(-)

b/drivers/staging/rtl8188eu/core/rtw_efuse.c
index 7006088..77f7552 100644

@@ -212,8 +212,7 @@ efuse_phymap_to_logical(u8 *phymap, u16 _offset, u16

_size_byte, u8 *pbuf)
exit:
kfree(efuseTbl);

- if (eFuseWord)
- kfree(eFuseWord);
+ kfree(eFuseWord);
}

static void efuse_read_phymap_from_txpktbuf(

b/drivers/staging/rtl8188eu/core/rtw_mlme.c
index 149c271..df54350 100644

@@ -122,8 +122,7 @@ void rtw_free_mlme_priv(struct mlme_priv *pmlmepriv)
 	rtw_free_mlme_priv_ie_data(pmlmepriv);
 
 	if (pmlmepriv) {
-		if (pmlmepriv->free_bss_buf)
-			vfree(pmlmepriv->free_bss_buf);
+		vfree(pmlmepriv->free_bss_buf);
 	}
 }
 

b/drivers/staging/rtl8188eu/core/rtw_sta_mgt.c
index e1dc8fa..af1de9c 100644

@@ -201,8 +201,7 @@ u32	_rtw_free_sta_priv(struct	sta_priv *pstapriv)
 
 		rtw_mfree_sta_priv_lock(pstapriv);
 
-		if (pstapriv->pallocated_stainfo_buf)
-			vfree(pstapriv->pallocated_stainfo_buf);
+		vfree(pstapriv->pallocated_stainfo_buf);
 	}
 
 	return _SUCCESS;

b/drivers/staging/rtl8188eu/core/rtw_xmit.c
index 639ace0..011c9cf 100644

@@ -246,11 +246,9 @@ void _rtw_free_xmit_priv (struct xmit_priv *pxmitpriv)
 		pxmitbuf++;
 	}
 
-	if (pxmitpriv->pallocated_frame_buf)
-		vfree(pxmitpriv->pallocated_frame_buf);
+	vfree(pxmitpriv->pallocated_frame_buf);
 
-	if (pxmitpriv->pallocated_xmitbuf)
-		vfree(pxmitpriv->pallocated_xmitbuf);
+	vfree(pxmitpriv->pallocated_xmitbuf);
 
 	/*  free xmit extension buff */
 	pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmit_extbuf;

b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
index 407a318..cdb70e4 100644

@@ -456,7 +456,7 @@ free_adapter:
 	if (status != _SUCCESS) {
 		if (pnetdev)
 			rtw_free_netdev(pnetdev);
-		else if (padapter)
+		else
 			vfree(padapter);
 		padapter = NULL;
 	}

@@ -487,8 +487,7 @@ static void rtw_usb_if1_deinit(struct adapter *if1)
 	DBG_88E("+r871xu_dev_remove, hw_init_completed=%d\n",
 		if1->hw_init_completed);
 	rtw_free_drv_sw(if1);
-	if (pnetdev)
-		rtw_free_netdev(pnetdev);
+	rtw_free_netdev(pnetdev);
 }
 
 static int rtw_drv_init(struct usb_interface *pusb_intf, const struct

usb_device_id *pdid)

2.1.2

[...]

I tried this small source code search out again.

elfring at Sonne:~/Projekte/Coccinelle/janitor> date \

So 26. Okt 07:02:18 CET 2014
@Show_functions_with_input_pointer_validation@
identifier fun, x;
type t;
@@
*void fun(..., t* x, ...)
{
...
if (unlikely(ZERO_OR_NULL_PTR(x)))

     return;

...
}
-----
spatch version 1.0.0-rc22 with Python support and with PCRE support
init_defs_builtins: /usr/local/share/coccinelle/standard.h
HANDLING: /usr/src/linux-stable/mm/slab.c
(ONCE) TYPE: header process.h not found
[...]
(ONCE) TYPE: header trace/events/kmem.h not found
(ONCE) already tagged but only removed, so safe
diff =
--- /usr/src/linux-stable/mm/slab.c
@@ -3598,7 +3598,6 @@ EXPORT_SYMBOL(kmem_cache_free);
* Don't free memory not originally allocated by kmalloc()
* or you will run into trouble.
*/
-void kfree(const void *objp)

 {
        struct kmem_cache *c;
        unsigned long flags;

Note: processing took 43.5s: /usr/src/linux-stable/mm/slab.c
starting: Common.group_assoc_bykey_eff
ending: Common.group_assoc_bykey_eff, 0.000005s
---------------------
profiling result
---------------------

Main total                               :     43.552 sec          1 count
Main.outfiles computation                :     43.490 sec          1 count
full_engine                              :     43.481 sec          1 count
C parsing                                :     17.765 sec        866 count
TOTAL                                    :     17.763 sec        866 count
HACK                                     :      6.017 sec       2001 count
C parsing.fix_cpp                        :      5.559 sec       1134 count
C parsing.tokens                         :      3.528 sec        867 count
LEXING                                   :      3.506 sec        866 count
Parsing: multi pass                      :      2.818 sec       2369 count
Parsing: 1st pass                        :      2.258 sec      42256 count
MACRO managment                          :      1.725 sec       2671 count
YACC                                     :      1.634 sec      40007 count
TAC.annotate_program                     :      0.983 sec        433 count
show_xxx                                 :      0.855 sec        310 count
C parsing.fix_define                     :      0.761 sec       1733 count
MACRO mgmt prep 2                        :      0.494 sec        866 count
C consistencycheck                       :      0.461 sec        866 count
C parsing.lookahead                      :      0.407 sec     455401 count
Common.full_charpos_to_pos_large         :      0.389 sec        867 count
flow                                     :      0.371 sec      20946 count
C parsing.mk_info_item                   :      0.361 sec      42256 count
C parsing.lex_ident                      :      0.314 sec     313227 count
Type_c.type_of_s                         :      0.234 sec      10649 count
fix_flow                                 :      0.154 sec      15138 count
TAC.add_binding                          :      0.075 sec      18856 count
MACRO mgmt prep 1                        :      0.064 sec        866 count
pre_engine                               :      0.036 sec          1 count
parse cocci                              :      0.032 sec          1 count
bigloop                                  :      0.026 sec          1 count
Show_functions_with_input_pointer_validation :      0.023 sec          1 count
process_a_ctl_a_env_a_toplevel           :      0.023 sec        300 count
mysat                                    :      0.022 sec        300 count
Common.info_from_charpos                 :      0.018 sec         82 count
Common.=~                                :      0.013 sec       9107 count
worth_trying                             :      0.009 sec          1 count
Common.full_charpos_to_pos               :      0.005 sec          2 count
TAC.typedef_fix                          :      0.005 sec      14621 count
C unparsing                              :      0.005 sec          2 count
asttoctl2                                :      0.003 sec          1 count
ctl                                      :      0.000 sec          3 count
Transformation3.transform                :      0.000 sec          1 count
get_glimpse_constants                    :      0.000 sec          1 count
Main.result analysis                     :      0.000 sec          1 count
check_duplicate                          :      0.000 sec          1 count
Common.group_assoc_bykey_eff             :      0.000 sec          1 count
C_vs_c                                   :      0.000 sec          1 count
merge_env                                :      0.000 sec          1 count
post_engine                              :      0.000 sec          1 count
Main.infiles computation                 :      0.000 sec          1 count

So 26. Okt 07:03:01 CET 2014

Do you find this analysis result interesting?

Regards,
Markus

None. - My "test computer" does not provide the corresponding hardware for the affected driver source files.

Regards,
Markus

I would like to show again that the version 1.0.0-rc22 works better than 1.0.0-rc20 for another use case.

Interesting ...

Does your feedback refer also to the message "(ONCE) already tagged but only removed, so safe" eventually?

I am curious how the shown profiling results will change over time because of software improvements.

Regards,
Markus

I still have the hardware in use and I am willing to test.

If necessary, please send me a URL to the snapshot of the kernel with the modified code. I can compile this (takes about 8 hours ;-) on the high speed atom n2600) and run a couple of random tests, such as starting the WM, browsing some webpages, changing resolution, and run a 2D benchmark.

Greetings,
Arthur Borsboom

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://systeme.lip6.fr/pipermail/cocci/attachments/20141026/a28726a1/attachment-0001.html>

Why is this here? Please use git send-email to send a patch, so I don't have to hand-edit the text in it.

Also, your Subject is a bit odd, it has a "Re:" in it for no reason, and your From: doesn't match the name you used here.

Please fix up and resend.

thanks,

greg k-h

The functions debug_unregister() and kfree_fsm() test whether their argument is NULL and then return immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/s390/net/claw.c      |  6 ++----
 drivers/s390/net/ctcm_main.c |  6 ++----
 drivers/s390/net/lcs.c       |  6 ++----
 drivers/s390/net/netiucv.c   | 12 ++++--------
 4 files changed, 10 insertions(+), 20 deletions(-)

index 213e54e..d609ca0 100644
@@ -109,10 +109,8 @@ static debug_info_t *claw_dbf_trace;
 static void
 claw_unregister_debug_facility(void)
 {
-	if (claw_dbf_setup)
-		debug_unregister(claw_dbf_setup);
-	if (claw_dbf_trace)
-		debug_unregister(claw_dbf_trace);
+	debug_unregister(claw_dbf_setup);
+	debug_unregister(claw_dbf_trace);
 }
 
 static int

index e056dd4..34dc0f3 100644
@@ -1074,8 +1074,7 @@ static void ctcm_free_netdevice(struct net_device *dev)
 	if (priv) {
 		grp = priv->mpcg;
 		if (grp) {
-			if (grp->fsm)
-				kfree_fsm(grp->fsm);
+			kfree_fsm(grp->fsm);
 			if (grp->xid_skb)
 				dev_kfree_skb(grp->xid_skb);
 			if (grp->rcvd_xid_skb)

@@ -1672,8 +1671,7 @@ static int ctcm_shutdown_device(struct ccwgroup_device *cgdev)
 		ctcm_free_netdevice(dev);
 	}
 
-	if (priv->fsm)
-		kfree_fsm(priv->fsm);
+	kfree_fsm(priv->fsm);
 
 	ccw_device_set_offline(cgdev->cdev[1]);
 	ccw_device_set_offline(cgdev->cdev[0]);

index 0a7d87c..5dfa7dd 100644
@@ -88,10 +88,8 @@ static debug_info_t *lcs_dbf_trace;
 static void
 lcs_unregister_debug_facility(void)
 {
-	if (lcs_dbf_setup)
-		debug_unregister(lcs_dbf_setup);
-	if (lcs_dbf_trace)
-		debug_unregister(lcs_dbf_trace);
+	debug_unregister(lcs_dbf_setup);
+	debug_unregister(lcs_dbf_trace);
 }
 
 static int

index 0a87809..bdcc3fe 100644
@@ -487,12 +487,9 @@ DEFINE_PER_CPU(char[256], iucv_dbf_txt_buf);
 
 static void iucv_unregister_dbf_views(void)
 {
-	if (iucv_dbf_setup)
-		debug_unregister(iucv_dbf_setup);
-	if (iucv_dbf_data)
-		debug_unregister(iucv_dbf_data);
-	if (iucv_dbf_trace)
-		debug_unregister(iucv_dbf_trace);
+	debug_unregister(iucv_dbf_setup);
+	debug_unregister(iucv_dbf_data);
+	debug_unregister(iucv_dbf_trace);
 }
 static int iucv_register_dbf_views(void)
 {

@@ -1975,8 +1972,7 @@ static void netiucv_free_netdevice(struct net_device *dev)
 	if (privptr) {
 		if (privptr->conn)
 			netiucv_remove_connection(privptr->conn);
-		if (privptr->fsm)
-			kfree_fsm(privptr->fsm);
+		kfree_fsm(privptr->fsm);
 		privptr->conn = NULL; privptr->fsm = NULL;
 		/* privptr gets freed by free_netdev() */
 	}

2.1.3

The functions kfree(), rtw_free_netdev() and vfree() test whether their argument is NULL and then return immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/staging/rtl8188eu/core/rtw_efuse.c   | 3 +--
 drivers/staging/rtl8188eu/core/rtw_mlme.c    | 3 +--
 drivers/staging/rtl8188eu/core/rtw_sta_mgt.c | 3 +--
 drivers/staging/rtl8188eu/core/rtw_xmit.c    | 6 ++----
 drivers/staging/rtl8188eu/os_dep/usb_intf.c  | 5 ++---
 5 files changed, 7 insertions(+), 13 deletions(-)

b/drivers/staging/rtl8188eu/core/rtw_efuse.c
index 7006088..77f7552 100644

@@ -212,8 +212,7 @@ efuse_phymap_to_logical(u8 *phymap, u16 _offset, u16

_size_byte, u8 *pbuf)
exit:
kfree(efuseTbl);

- if (eFuseWord)
- kfree(eFuseWord);
+ kfree(eFuseWord);
}

static void efuse_read_phymap_from_txpktbuf(

b/drivers/staging/rtl8188eu/core/rtw_mlme.c
index 149c271..df54350 100644

@@ -122,8 +122,7 @@ void rtw_free_mlme_priv(struct mlme_priv *pmlmepriv)
 	rtw_free_mlme_priv_ie_data(pmlmepriv);
 
 	if (pmlmepriv) {
-		if (pmlmepriv->free_bss_buf)
-			vfree(pmlmepriv->free_bss_buf);
+		vfree(pmlmepriv->free_bss_buf);
 	}
 }
 

b/drivers/staging/rtl8188eu/core/rtw_sta_mgt.c
index e1dc8fa..af1de9c 100644

@@ -201,8 +201,7 @@ u32	_rtw_free_sta_priv(struct	sta_priv *pstapriv)
 
 		rtw_mfree_sta_priv_lock(pstapriv);
 
-		if (pstapriv->pallocated_stainfo_buf)
-			vfree(pstapriv->pallocated_stainfo_buf);
+		vfree(pstapriv->pallocated_stainfo_buf);
 	}
 
 	return _SUCCESS;

b/drivers/staging/rtl8188eu/core/rtw_xmit.c
index 639ace0..011c9cf 100644

@@ -246,11 +246,9 @@ void _rtw_free_xmit_priv (struct xmit_priv *pxmitpriv)
 		pxmitbuf++;
 	}
 
-	if (pxmitpriv->pallocated_frame_buf)
-		vfree(pxmitpriv->pallocated_frame_buf);
+	vfree(pxmitpriv->pallocated_frame_buf);
 
-	if (pxmitpriv->pallocated_xmitbuf)
-		vfree(pxmitpriv->pallocated_xmitbuf);
+	vfree(pxmitpriv->pallocated_xmitbuf);
 
 	/*  free xmit extension buff */
 	pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmit_extbuf;

b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
index 407a318..cdb70e4 100644

@@ -456,7 +456,7 @@ free_adapter:
 	if (status != _SUCCESS) {
 		if (pnetdev)
 			rtw_free_netdev(pnetdev);
-		else if (padapter)
+		else
 			vfree(padapter);
 		padapter = NULL;
 	}

@@ -487,8 +487,7 @@ static void rtw_usb_if1_deinit(struct adapter *if1)
 	DBG_88E("+r871xu_dev_remove, hw_init_completed=%d\n",
 		if1->hw_init_completed);
 	rtw_free_drv_sw(if1);
-	if (pnetdev)
-		rtw_free_netdev(pnetdev);
+	rtw_free_netdev(pnetdev);
 }
 
 static int rtw_drv_init(struct usb_interface *pusb_intf, const struct

usb_device_id *pdid)

2.1.2

Are there any chances to achieve the suggested fine-tuning for jump labels also with another semantic patch approach?

Regards,
Markus

No, I don't think so. The pattern is not regular enough.

julia

The following functions test whether their argument is NULL and then return immediately.
* btrfs_free_path()
* free_extent_buffer()
* free_extent_map()
* free_extent_state()
* iput()
* kfree()

Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 fs/btrfs/dev-replace.c       |  3 +--
 fs/btrfs/extent_io.c         | 12 ++++--------
 fs/btrfs/file.c              |  6 ++----
 fs/btrfs/free-space-cache.c  |  7 +++----
 fs/btrfs/inode.c             |  6 ++----
 fs/btrfs/reada.c             |  3 +--
 fs/btrfs/relocation.c        |  3 +--
 fs/btrfs/tests/btrfs-tests.c |  3 +--
 fs/btrfs/tree-defrag.c       |  3 +--
 fs/btrfs/tree-log.c          |  6 ++----
 10 files changed, 18 insertions(+), 34 deletions(-)

index 6f662b3..3465029 100644
@@ -183,8 +183,7 @@ no_valid_dev_replace_entry_found:
 	}
 
 out:
-	if (path)
-		btrfs_free_path(path);
+	btrfs_free_path(path);
 	return ret;
 }
 

index bf3f424..cfbf00a 100644
@@ -704,8 +704,7 @@ next:
 
 out:
 	spin_unlock(&tree->lock);
-	if (prealloc)
-		free_extent_state(prealloc);
+	free_extent_state(prealloc);
 
 	return 0;
 

@@ -1006,8 +1005,7 @@ hit_next:
 
 out:
 	spin_unlock(&tree->lock);
-	if (prealloc)
-		free_extent_state(prealloc);
+	free_extent_state(prealloc);
 
 	return err;
 

@@ -1223,8 +1221,7 @@ hit_next:
 
 out:
 	spin_unlock(&tree->lock);
-	if (prealloc)
-		free_extent_state(prealloc);
+	free_extent_state(prealloc);
 
 	return err;
 

@@ -4146,8 +4143,7 @@ int extent_readpages(struct extent_io_tree *tree,
 		__extent_readpages(tree, pagepool, nr, get_extent, &em_cached,
 				   &bio, 0, &bio_flags, READ);
 
-	if (em_cached)
-		free_extent_map(em_cached);
+	free_extent_map(em_cached);
 
 	BUG_ON(!list_empty(pages));
 	if (bio)

index a18ceab..add07ce8 100644
@@ -677,10 +677,8 @@ next:
 		/* once for the tree*/
 		free_extent_map(em);
 	}
-	if (split)
-		free_extent_map(split);
-	if (split2)
-		free_extent_map(split2);
+	free_extent_map(split);
+	free_extent_map(split2);
 }
 
 /*

index 3384819..11883e2 100644
@@ -1943,8 +1943,7 @@ new_bitmap:
 
 out:
 	if (info) {
-		if (info->bitmap)
-			kfree(info->bitmap);
+		kfree(info->bitmap);
 		kmem_cache_free(btrfs_free_space_cachep, info);
 	}
 

@@ -3322,8 +3321,8 @@ again:
 
 	if (info)
 		kmem_cache_free(btrfs_free_space_cachep, info);
-	if (map)
-		kfree(map);
+
+	kfree(map);
 	return 0;
 }
 

index d23362f..7301b99 100644
@@ -857,8 +857,7 @@ static u64 get_extent_allocation_hint(struct inode *inode,

u64 start,
em = search_extent_mapping(em_tree, 0, 0); if (em && em->block_start < EXTENT_MAP_LAST_BYTE)
alloc_hint = em->block_start;
- if (em)
- free_extent_map(em);
+ free_extent_map(em);
} else {
alloc_hint = em->block_start; free_extent_map(em);
@@ -6573,8 +6572,7 @@ out:

trace_btrfs_get_extent(root, em);

- if (path)
- btrfs_free_path(path);
+ btrfs_free_path(path);
if (trans) {
ret = btrfs_end_transaction(trans, root); if (!err)

index b63ae20..ec8eb49 100644
@@ -731,8 +731,7 @@ static int reada_start_machine_dev(struct btrfs_fs_info

*fs_info,
else if (eb)
__readahead_hook(fs_info->extent_root, eb, eb->start, ret);

- if (eb)
- free_extent_buffer(eb);
+ free_extent_buffer(eb);

return 1;

index 74257d6..f87a5ee 100644
@@ -4158,8 +4158,7 @@ out:
 	btrfs_end_transaction(trans, root);
 	btrfs_btree_balance_dirty(root);
 	if (err) {
-		if (inode)
-			iput(inode);
+		iput(inode);
 		inode = ERR_PTR(err);
 	}
 	return inode;

index 9626252..7fb123f 100644
@@ -162,8 +162,7 @@ void btrfs_free_dummy_root(struct btrfs_root *root)
 {
 	if (!root)
 		return;
-	if (root->node)
-		free_extent_buffer(root->node);
+	free_extent_buffer(root->node);
 	if (root->fs_info)
 		btrfs_free_dummy_fs_info(root->fs_info);
 	kfree(root);

index a63719c..c74f106 100644
@@ -118,8 +118,7 @@ int btrfs_defrag_leaves(struct btrfs_trans_handle *trans,
 		ret = -EAGAIN;
 	}
 out:
-	if (path)
-		btrfs_free_path(path);
+	btrfs_free_path(path);
 	if (ret == -EAGAIN) {
 		if (root->defrag_max.objectid > root->defrag_progress.objectid)
 			goto done;

index 1475979..70926a9 100644
@@ -736,8 +736,7 @@ static noinline int replay_one_extent(struct