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

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: [PATCH v2] coccinelle: semantic code search for missingof_node_put

[Julia Lawall]

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



On Thu, 4 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.  Could you send me
again the entire semantic patch so I can check on this?

I think that what you want is:

when != v4l2_async_notifier_add_fwnode_subdev(...,<+...x...+>,...)

ie x occurring somewhere within some argument.

julia

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 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