Re: [PATCH v2] coccinelle: semantic code search formissingof_node_put

Re: [PATCH v2] coccinelle: semantic code search formissingof_node_put

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 6974 bytes --]



On Fri, 5 Jul 2019, wen.yang99@zte.com.cn wrote:

> > > > > > +x = @p1\(of_find_all_nodes\|
> > > > >
> > > > > I would find this SmPL disjunction easier to read without the usage
> > > > > of extra backslashes.
> > > > >
> > > > > +x =
> > > > > +(of_…
> > > > > +|of_…
> > > > > +)@p1(...);
> > > >
> > > > Did you actually test this?  I doubt that a position metavariable can be
> > > > put on a ) of a disjunction.
> > > >
> > > > > > +|
> > > > > > +return x;
> > > > > > +|
> > > > > > +return of_fwnode_handle(x);
> > > > >
> > > > > Can a nested SmPL disjunction be helpful at such places?
> > > > >
> > > > > +|return
> > > > > +(x
> > > > > +|of_fwnode_handle(x)
> > > > > +);
> > > >
> > > > The original code is much more readable.  The internal representation will
> > > > be the same.
> > > >
> > > > > > +    when != v4l2_async_notifier_add_fwnode_subdev(<...x...>)
> > > > >
> > > > > Would the specification variant “<+... x ...+>” be relevant
> > > > > for the parameter selection?
> > > >
> > > > I'm indeed quite surprised that <...x...> would be accepted by the parser..
> > >
> > > Hi julia,
> > >
> > > Thank you for your comments.
> > > We tested and found that both <...x...> and <+... x ...+> variants work fine.
> > > We use <... x ...> instead of <+... x ...+> here to eliminate the following false positives:
> > >
> > > ./drivers/media/platform/qcom/camss/camss.c:504:1-7: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 479, but without a corresponding object release within this function.
> > >
> > > 465 static int camss_of_parse_ports(struct camss *camss)
> > > 466 {
> > > ...
> > > 479 remote = of_graph_get_remote_port_parent(node);
> > > ...
> > > 486 asd = v4l2_async_notifier_add_fwnode_subdev(
> > > 487 &camss->notifier, of_fwnode_handle(remote), ---> v4l2_async_notifier_add_fwnode_subdev will pass remote to camss->notifier.
> > > 488 sizeof(*csd));
> > > ...
> > > 504 return num_subdevs;
> >
> > I suspect that what is happening is that there is a runtime error, but
> > that error is caught somewhere and you don't see it.
>
> Thanks.
> You are right, there is indeed a runtime error.
> Since make coccicheck adds the "-very-quiet" parameter by default, we didn't find it.
>
> $ spatch --sp-file   of_node_put.cocci   -D report drivers/media/platform/am437x/am437x-vpfe.c
> init_defs_builtins: /usr/local/bin/../lib/coccinelle/standard.h
> HANDLING: drivers/media/platform/am437x/am437x-vpfe.c
> exn while in timeout_function
> only handling multi and no when code in a nest expr
>
> >  Could you send me again the entire semantic patch so I can check on this?
> >
>
> Thanks.
> The entire SmPL is as follows:
>
> $ cat of_node_put.cocci
> // SPDX-License-Identifier: GPL-2.0
> /// Find missing of_node_put
> ///
> // Confidence: Moderate
> // Copyright: (C) 2018-2019 Wen Yang, ZTE.
> // Comments:
> // Options: --no-includes --include-headers
>
> virtual report
> virtual org
>
> @initialize:python@
> @@
>
> seen = set()
>
> def add_if_not_present (p1, p2):
>     if (p1, p2) not in seen:
>         seen.add((p1, p2))
>         return True
>     return False

Did you need this?  Normally a script rule is run only once for each set
of bindings for the inherited variables.  I guess that multiple p1s could
lead to the same p2, and you only want to report on one of them?

This set is going to be global to the whole kernel, or at least to all of
the files considered by a given thread, if you use -j.  To clean it up on
each file, you can make another python at the end that depends on r1 or r2
and depends on report.  This rule can clear seen.

Otherwise, it looks fine.

julia


>
> def display_report(p1, p2):
>     if add_if_not_present(p1[0].line, p2[0].line):
>        coccilib.report.print_report(p2[0],
>                                     "ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line "
>                                     + p1[0].line
>                                     + ", but without a corresponding object release within this function.")
>
> def display_org(p1, p2):
>     cocci.print_main("acquired a node pointer with refcount incremented", p1)
>     cocci.print_secs("needed of_node_put", p2)
>
> @r1 exists@
> local idexpression struct device_node *x;
> expression e, e1;
> position p1, p2;
> statement S;
> type T;
> @@
>
> x = @p1\(of_find_all_nodes\|
>          of_get_cpu_node\|
>          of_get_parent\|
>          of_get_next_parent\|
>          of_get_next_child\|
>          of_get_next_cpu_node\|
>          of_get_compatible_child\|
>          of_get_child_by_name\|
>          of_find_node_opts_by_path\|
>          of_find_node_by_name\|
>          of_find_node_by_type\|
>          of_find_compatible_node\|
>          of_find_node_with_property\|
>          of_find_matching_node_and_match\|
>          of_find_node_by_phandle\|
>          of_parse_phandle\|
>          of_find_next_cache_node\|
>          of_get_next_available_child\)(...);
> ...
> if (x == NULL || ...) S
> ... when != e = (T)x
>     when != true x == NULL
>     when != of_node_put(x)
>     when != of_get_next_parent(x)
>     when != of_find_matching_node(x, ...)
>     when != if (x) { ... return x; }
>     when != v4l2_async_notifier_add_fwnode_subdev(<...x...>)
>     when != e1 = of_fwnode_handle(x)
> (
> if (x) { ... when forall
>          of_node_put(x) ... }
> |
> return x;
> |
> return of_fwnode_handle(x);
> |
> return@p2 ...;
> )
>
> @script:python depends on report && r1@

No need to depend on r1.  That is guaranteed by the inheritance on the
metavariables below.

> p1 << r1.p1;
> p2 << r1.p2;
> @@
>
> display_report(p1, p2)
>
> @script:python depends on org && r1@
> p1 << r1.p1;
> p2 << r1.p2;
> @@
>
> display_org(p1, p2)
>
> @r2 exists@
> local idexpression struct device_node *x;
> expression e, e1;
> position p1, p2;
> identifier f;
> statement S;
> type T;
> @@
>
> (
> x = f@p1(...);
> ... when != e = (T)x
>     when != true x == NULL
>     when != of_node_put(x)
>     when != of_get_next_parent(x)
>     when != of_find_matching_node(x, ...)
>     when != if (x) { ... return x; }
>     when != v4l2_async_notifier_add_fwnode_subdev(<...x...>)
>     when != e1 = of_fwnode_handle(x)
> (
> if (x) { ... when forall
>          of_node_put(x) ... }
> |
> return x;
> |
> return of_fwnode_handle(x);
> |
> return@p2 ...;
> )
> &
> x = f(...)
> ...
> if (<+...x...+>) S
> ...
> of_node_put(x);
> )
> @script:python depends on report && r2@
> p1 << r2.p1;
> p2 << r2.p2;
> @@
>
> display_report(p1, p2)
>
> @script:python depends on org && r2@
> p1 << r2.p1;
> p2 << r2.p2;
> @@
>
> display_org(p1, p2)
>
> > I think that what you want is:
> >
> > when != v4l2_async_notifier_add_fwnode_subdev(...,<+...x...+>,...)
> >
> > ie x occurring somewhere within some argument.
>
> Thank you very much for your suggestion.
> Applying it will solve this problem, thank you.
>
> --
> Thanks and regards,
> Wen

Re: [v2] coccinelle: semantic code search for missing of_node_put

[Markus Elfring]

>> @script:python depends on report && r1@
>
> No need to depend on r1.  That is guaranteed by the inheritance on the
> metavariables below.

Will this information become more helpful for the completion of the corresponding
software documentation?

Regards,
Markus

Re: [v2] coccinelle: semantic code search for missing of_node_put

[Markus Elfring]

> We tested and found that both <...x...> and <+... x ...+> variants work fine.

Is the difference in the functionality from this SmPL construct clear already?


> We use <... x ...> instead of <+... x ...+> here to eliminate the following false positives:

Do we stumble on another software design challenge?

For which function parameter will the specified variable be required finally?


> 486 asd = v4l2_async_notifier_add_fwnode_subdev(
> 487 &camss->notifier, of_fwnode_handle(remote), ---> v4l2_async_notifier_add_fwnode_subdev will pass remote to camss->notifier.
> 488 sizeof(*csd));

Should any more special cases be taken better into account?

Regards,
Markus

Re: [v2] coccinelle: semantic code search for missing of_node_put

[Markus Elfring]

> We will also provide an example written in Python later.

Will the code move from the commit description into a file
for your next patch version?


> We first use this script to find out all the function names to be processed,

I am still curious on how the output format selection will become clearer
for the potentially desired automatic data conversion.


> and then copy these function names into r1.

Would this action be performed by another software build script?


>>> +@initialize:python@
>>> +@@
>>> +
>>> +seen = set()
>>> +
>>> +def add_if_not_present (p1, p2):
>>
>> It seems that you would like to use iteration functionality.

I am waiting on another constructive answer for this implementation detail.


>>> +x = @p1\(of_find_all_nodes\|
>>
>> I would find this SmPL disjunction easier to read without the usage
>> of extra backslashes.
>>
>> +x =
>> +(of_…
>> +|of_…
>> +)@p1(...);
>>
>>
>> Which sort criteria were applied for the generation of the shown
>> function name list?
>
> As julia pointed out, your current writing is not compiled.

* It can be needed for a while to specify the mentioned position variable
  at an other place.

* Would you like to adjust the SmPL coding style here?

* Will the application of sort criteria be clarified for such identifier lists?


>>> +if (x == NULL || ...) S
>>> +... when != e = (T)x
>>> +    when != true x == NULL
…
> Our previous version used the "when any" clause, so we need
> "when != true x == NULL".

I suggest to reconsider further aspects for such constraints.


> We can delete this code exclusion specification  for this version.

I would find another assignment exclusion more appropriate at this place.

Regards,
Markus

Re: [v2] coccinelle: semantic code search for missing of_node_put

[Markus Elfring]

>>>> +if (x == NULL || ...) S
>>>> +... when != e = (T)x
>>>> +    when != true x == NULL
…
> I assume that it was added because it was found to be useful.

We can get different software development opinions also on
this implementation detail.


> Please actually try things out before declaring them to be useless.

Further clarification of desirable software behaviour will help.
I dare to express doubts around the SmPL functionality “when != true x == NULL”.

Would any more contributors like to share additional insights for the safer
application of the semantic patch language?

Is a reassignment of such local variable an usual precondition for
the discussed programming concern?

Regards,
Markus

Re: [v2] coccinelle: semantic code search for missing of_node_put

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 1088 bytes --]



On Sat, 29 Jun 2019, Markus Elfring wrote:

> >> +if (x == NULL || ...) S
> >> +... when != e = (T)x
> >> +    when != true x == NULL
> >
> > I wonder if this code exclusion specification is really required
> > after a null pointer was checked before.
>
> I would like to add another view for this implementation detail.
>
> The when constraint can express a software desire which can be reasonable
> to some degree. You would like to be sure that a null pointer will not occur
> after a corresponding check succeeded.

He wants to be sure that the true branch through a NULL pointer check is
not taken.

> * But I feel unsure about the circumstances under which the Coccinelle software
>   can determine this aspect actually.
>
> * I find that it can eventually make sense only after the content of
>   the local variable (which is identified by “x”) was modified.
>   Thus I would find the exclusion of assignments more useful at this place.

I assume that it was added because it was found to be useful.  Please
actually try things out before declaring them to be useless.

julia

Re: [v2] coccinelle: semantic code search for missing of_node_put

[Markus Elfring]

>> +if (x == NULL || ...) S
>> +... when != e = (T)x
>> +    when != true x == NULL
>
> I wonder if this code exclusion specification is really required
> after a null pointer was checked before.

I would like to add another view for this implementation detail.

The when constraint can express a software desire which can be reasonable
to some degree. You would like to be sure that a null pointer will not occur
after a corresponding check succeeded.

* But I feel unsure about the circumstances under which the Coccinelle software
  can determine this aspect actually.

* I find that it can eventually make sense only after the content of
  the local variable (which is identified by “x”) was modified.
  Thus I would find the exclusion of assignments more useful at this place.

Regards,
Markus

Re: [v2] coccinelle: semantic code search for missing of_node_put

[Markus Elfring]

>> +x =
>> +(of_…
>> +|of_…
>> +)@p1(...);
>
> Did you actually test this?  I doubt that a position metavariable can be
> put on a ) of a disjunction.

Would you ever like to support this possibility?


>> +|return
>> +(x
>> +|of_fwnode_handle(x)
>> +);
>
> The original code is much more readable.

We have got different views around such specification variants.


> The internal representation will be the same.

I imagine that the Coccinelle software might evolve into additional directions.


Regards,
Markus

Re: [v2] coccinelle: semantic code search for missing of_node_put

[Markus Elfring]

>> +x =
>> +(of_…
>> +|of_…
>> +)@p1(...);
>
> Did you actually test this?  I doubt that a position metavariable can be
> put on a ) of a disjunction.

Would you ever like to support this possibility?


>> +|return
>> +(x
>> +|of_fwnode_handle(x)
>> +);
>
> The original code is much more readable.

We have got different views around such specification variants.


> The internal representation will be the same.

I imagine that the Coccinelle software might evolve into additional directions.


Regards,
Markus