Re: [PATCH v5] coccinelle: semantic code search for missing put_device()

Re: [PATCH v5] coccinelle: semantic code search for missing put_device()

[Markus Elfring]

> In a function, for a local variable returned by calling
> of_find_device_by_node(),

I suggest to reconsider this information once more.

1. Will an other wording be more appropriate for the storage of
   a function return value?

2. Can the restriction “local” be omitted?

3. Will any macros be involved eventually?


> c, for the rest of the situation, the current function should release the
>    reference by calling put_device,

Can it happen that on other function will perform the desired reference release?


>                                     this code search will report the
>    corresponding error message.

Rewording?
A code search can report an error with a specific confidence.


> v5->v4:

Such version information would be sufficient also without arrows, wouldn't it?


> - add a SPDX identifierfix

Would you like to fix a typo at the end?


> +@script:python depends on report@
> +p1 << search.p1;
> +p2 << search.p2;
> +@@
> +
> +coccilib.report.print_report(p2[0], "ERROR: missing put_device; "
> +			      + "call of_find_device_by_node on line "
> +			      + p1[0].line
> +			      + ", but without a corresponding object release "
> +			      + "within this function.")

I find your interpretation of my reminder for the preferred avoidance
of split string literals interesting somehow.

Can the following source code variant be more appropriate?

+coccilib.report.print_report(p2[0],
+			      "WARNING: missing put_device - of_find_device_by_node() call on line "
+			      + p1[0].line
+			      + ", but without a corresponding object release within this function.")


Will any more advanced error diagnostics be eventually developed?

Regards,
Markus

Re: [PATCH v5] Coccinelle: semantic code search for missing put_device()

[Markus Elfring]

> +@search exists@
> +local idexpression id;
> +expression x,e,e1;
> +position p1,p2;
> +type T,T1,T2,T3;
> +@@
> +
> +id = of_find_device_by_node@p1(x)
> +... when != e = id
> +if (id == NULL || ...) { ... return ...; }
> +... when != put_device(&id->dev)
…
> +    when != if (id) { ... put_device(&id->dev) ... }
…

I would interpret this SmPL code in the way that the if statement
for the pointer check is “optional” in this line.
Is it an extra and redundant SmPL specification when the reference
release function could eventually be found just anywhere within
an implementation?


Will a need evolve to develop a similar source code search approach
for safer resource management with other function combinations?

Regards,
Markus

Re: [PATCH v5] Coccinelle: semantic code search for missing put_device()

[Julia Lawall]

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



On Fri, 15 Feb 2019, Markus Elfring wrote:

> > +@search exists@
> > +local idexpression id;
> > +expression x,e,e1;
> > +position p1,p2;
> > +type T,T1,T2,T3;
> > +@@
> > +
> > +id = of_find_device_by_node@p1(x)
> > +... when != e = id
> > +if (id == NULL || ...) { ... return ...; }
> > +... when != put_device(&id->dev)
> …
> > +    when != if (id) { ... put_device(&id->dev) ... }
> …
>
> I would interpret this SmPL code in the way that the if statement
> for the pointer check is “optional” in this line.
> Is it an extra and redundant SmPL specification when the reference
> release function could eventually be found just anywhere within
> an implementation?

The proposed when code is correct.  It is not redundant, because it checks
for a particular control-flow pattern.

julia

>
>
> Will a need evolve to develop a similar source code search approach
> for safer resource management with other function combinations?
>
> Regards,
> Markus
>

Re: [v5] Coccinelle: semantic code search for missing put_device()

[Markus Elfring]

>>> +id = of_find_device_by_node@p1(x)
…
>>> +if (id == NULL || ...) { ... return ...; }
>>> +... when != put_device(&id->dev)
>> …
>>> +    when != if (id) { ... put_device(&id->dev) ... }
>> …
>>
>> I would interpret this SmPL code in the way that the if statement
>> for the pointer check is “optional” in this line.
>> Is it an extra and redundant SmPL specification when the reference
>> release function could eventually be found just anywhere within
>> an implementation?
>
> The proposed when code is correct.

I agree that this SmPL code can work in the way it was designed.


> It is not redundant, because it checks for a particular control-flow pattern.

It took another moment until I dared to express a different software
development opinion also on this implementation detail.

Does the first SmPL when specification include the case that a call
of the function “put_device” can occur within a branch of an if statement?

Regards,
Markus

Re: [v5] Coccinelle: semantic code search for missing put_device()

[Markus Elfring]

>>> +id = of_find_device_by_node@p1(x)
…
>>> +if (id == NULL || ...) { ... return ...; }
>>> +... when != put_device(&id->dev)
>> …
>>> +    when != if (id) { ... put_device(&id->dev) ... }
>> …
>>
>> I would interpret this SmPL code in the way that the if statement
>> for the pointer check is “optional” in this line.
>> Is it an extra and redundant SmPL specification when the reference
>> release function could eventually be found just anywhere within
>> an implementation?
>
> The proposed when code is correct.

I agree that this SmPL code can work in the way it was designed.


> It is not redundant, because it checks for a particular control-flow pattern.

It took another moment until I dared to express a different software
development opinion also on this implementation detail.

Does the first SmPL when specification include the case that a call
of the function “put_device” can occur within a branch of an if statement?

Regards,
Markus

Re: [v5] Coccinelle: semantic code search for missing put_device()

[Julia Lawall]

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



On Fri, 15 Feb 2019, Markus Elfring wrote:

> >>> +id = of_find_device_by_node@p1(x)
> …
> >>> +if (id == NULL || ...) { ... return ...; }
> >>> +... when != put_device(&id->dev)
> >> …
> >>> +    when != if (id) { ... put_device(&id->dev) ... }
> >> …
> >>
> >> I would interpret this SmPL code in the way that the if statement
> >> for the pointer check is “optional” in this line.
> >> Is it an extra and redundant SmPL specification when the reference
> >> release function could eventually be found just anywhere within
> >> an implementation?
> >
> > The proposed when code is correct.
>
> I agree that this SmPL code can work in the way it was designed.
>
>
> > It is not redundant, because it checks for a particular control-flow pattern.
>
> It took another moment until I dared to express a different software
> development opinion also on this implementation detail.
>
> Does the first SmPL when specification include the case that a call
> of the function “put_device” can occur within a branch of an if statement?

It does include that, but there is another execution path where the
put device is not present.  But given the test in the if in the when code,
on that execution path id is NULL, an so there is no need to put it.

julia

Re: [v5] Coccinelle: semantic code search for missing put_device()

[Markus Elfring]

>> Does the first SmPL when specification include the case that a call
>> of the function “put_device” can occur within a branch of an if statement?
>
> It does include that,

Thanks for this acknowledgement.

So it seems that you find my interpretation of this bit of SmPL code appropriate.


> but there is another execution path where the put device is not present.

It is tried to find such cases.


> But given the test in the if in the when code,
> on that execution path id is NULL, an so there is no need to put it.

I would like to point out that the function “put_device” belongs also to
the category of functions which tolerate the passing of null pointers.
https://elixir.bootlin.com/linux/v5.0-rc6/source/drivers/base/core.c#L2053
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/core.c?id=cb5b020a8d38f77209d0472a0fea755299a8ec78#n2053

Have we got still different software development opinions about the need
for an extra pointer check in the “second” SmPL when specification?

Regards,
Markus

Re: [v5] coccinelle: semantic code search for missing put_device()

[Markus Elfring]

> Applied to linux-kbuild.

How does this information fit to the published sixth version
of the discussed script for the semantic patch language?
https://lore.kernel.org/cocci/HK0PR02MB36344E2B29CEB195892F6420B2610@HK0PR02MB3634.apcprd02.prod.outlook.com/
https://systeme.lip6.fr/pipermail/cocci/2019-February/005598.html

Are possible changes still in the waiting queue for a seventh variant?
https://lore.kernel.org/cocci/201902191014156680299@zte.com.cn/

Regards,
Markus

Re: [v5] coccinelle: semantic code search for missing put_device()

[Markus Elfring]

> Applied to linux-kbuild.

A questionable development version was integrated for this SmPL script.

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/scripts/coccinelle/free/put_device.cocci?idÚ9cfb87a44da61f2403c4312916befcb6b6d7e8

https://lore.kernel.org/cocci/201902191014156680299@zte.com.cn/


Now I am curious on how such a source code analysis approach will be
improved further.
Which changes will get the desired acceptance?

https://lore.kernel.org/patchwork/patch/1053979/
https://lore.kernel.org/lkml/1553321671-27749-1-git-send-email-wen.yang99@zte.com.cn/
https://systeme.lip6.fr/pipermail/cocci/2019-March/005679.html

Regards,
Markus

Re: [v5] coccinelle: semantic code search for missing put_device()

[Masahiro Yamada]

On Sun, Mar 24, 2019 at 1:19 AM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> > Applied to linux-kbuild.
>
> A questionable development version was integrated for this SmPL script.


Sorry for my bad job.

I usually use LKML patchwork to find patches.

When I searched "Wen Yang", v6 did not show up for some reasons.
https://lore.kernel.org/patchwork/project/lkml/list/?series=&submitter"638&state=*&q=&archive=&delegate
So, I just thought v5 was the latest one
and I was completely missing the context.


Apology.

Masahiro



> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/scripts/coccinelle/free/put_device.cocci?idÚ9cfb87a44da61f2403c4312916befcb6b6d7e8
>
> https://lore.kernel.org/cocci/201902191014156680299@zte.com.cn/
>
>
> Now I am curious on how such a source code analysis approach will be
> improved further.
> Which changes will get the desired acceptance?
>
> https://lore.kernel.org/patchwork/patch/1053979/
> https://lore.kernel.org/lkml/1553321671-27749-1-git-send-email-wen.yang99@zte.com.cn/
> https://systeme.lip6.fr/pipermail/cocci/2019-March/005679.html
>
> Regards,
> Markus
--
Best Regards
Masahiro Yamada

Re: [v5] coccinelle: semantic code search for missing put_device()

[Julia Lawall]

On Tue, 26 Mar 2019, Masahiro Yamada wrote:

> On Sun, Mar 24, 2019 at 1:19 AM Markus Elfring <Markus.Elfring@web.de> wrote:
> >
> > > Applied to linux-kbuild.
> >
> > A questionable development version was integrated for this SmPL script.
>
>
> Sorry for my bad job.
>
> I usually use LKML patchwork to find patches.
>
> When I searched "Wen Yang", v6 did not show up for some reasons.
> https://lore.kernel.org/patchwork/project/lkml/list/?series=&submitter"638&state=*&q=&archive=&delegate>
> So, I just thought v5 was the latest one
> and I was completely missing the context.

I think it is a minor detail that will have no impact in practice.

julia


>
>
> Apology.
>
> Masahiro
>
>
>
> > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/scripts/coccinelle/free/put_device.cocci?idÚ9cfb87a44da61f2403c4312916befcb6b6d7e8
> >
> > https://lore.kernel.org/cocci/201902191014156680299@zte.com.cn/
> >
> >
> > Now I am curious on how such a source code analysis approach will be
> > improved further.
> > Which changes will get the desired acceptance?
> >
> > https://lore.kernel.org/patchwork/patch/1053979/
> > https://lore.kernel.org/lkml/1553321671-27749-1-git-send-email-wen.yang99@zte.com.cn/
> > https://systeme.lip6.fr/pipermail/cocci/2019-March/005679.html
> >
> > Regards,
> > Markus
> --
> Best Regards
> Masahiro Yamada
>

Re: [v5] coccinelle: semantic code search for missing put_device()

[Markus Elfring]

>> So, I just thought v5 was the latest one
>> and I was completely missing the context.
>
> I think it is a minor detail

Additional implementation details were discussed also for this script
of the semantic patch language.


> that will have no impact in practice.

It will take another while to understand the relevance for the circumstances
of corresponding software adjustments better, won't it?

Will mentioned change possibilities be repeated with other update steps?

Regards,
Markus

Re: [v5] coccinelle: semantic code search for missing put_device()

[Markus Elfring]

> When I searched "Wen Yang", v6 did not show up for some reasons.
> https://lore.kernel.org/patchwork/project/lkml/list/?series=&submitter"638&state=*&q=&archive=&delegate
I find such a situation also interesting somehow.
I assume that there was another temporary technical difficulty involved
with the Linux mailing list (and related information systems).


> So, I just thought v5 was the latest one
> and I was completely missing the context.

Did any messages for the sixth (and seventh) script version reach you?

https://lore.kernel.org/cocci/201902191014156680299@zte.com.cn/

https://lore.kernel.org/lkml/1553321671-27749-1-git-send-email-wen.yang99@zte.com.cn/

Regards,
Markus

Re: [v5] coccinelle: semantic code search for missing put_device()

[Masahiro Yamada]

On Tue, Mar 26, 2019 at 6:06 PM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> > When I searched "Wen Yang", v6 did not show up for some reasons.
> > https://lore.kernel.org/patchwork/project/lkml/list/?series=&submitter"638&state=*&q=&archive=&delegate>
> I find such a situation also interesting somehow.
> I assume that there was another temporary technical difficulty involved
> with the Linux mailing list (and related information systems).


I think v6 fell into a crack somehow.


Check the LKML archive:
https://lkml.org/lkml/2019/2/16


I see your reply:

Re: [v6] coccinelle: semantic code search for missing put_device()Markus Elfring


But, I cannot find the v6 patch itself.



>
> > So, I just thought v5 was the latest one
> > and I was completely missing the context.
>
> Did any messages for the sixth (and seventh) script version reach you?
>
> https://lore.kernel.org/cocci/201902191014156680299@zte.com.cn/


I use both my corporate email address and Gmail,
and play it by ear.


I see it in my corporate mail box now.
It reached me probably because I subscribe to cocci@systeme.lip6.fr
with my socionext email address.

However, this time I only checked my Gmail box,
which subscribes to linux-kernel ML, but not cocci ML.

I just skimmed over the thread, then
I missed the fact you were giving some comments about v6.



> https://lore.kernel.org/lkml/1553321671-27749-1-git-send-email-wen.yang99@zte.com.cn/


Yes. I see this one.

I will apply it to fixes branch
if nobody sees a problem in it.


>
> Regards,
> Markus



-- 
Best Regards
Masahiro Yamada

Re: [v5] coccinelle: semantic code search for missing put_device()

[Julia Lawall]

On Tue, 26 Mar 2019, Masahiro Yamada wrote:

> On Tue, Mar 26, 2019 at 6:06 PM Markus Elfring <Markus.Elfring@web.de> wrote:
> >
> > > When I searched "Wen Yang", v6 did not show up for some reasons.
> > > https://lore.kernel.org/patchwork/project/lkml/list/?series=&submitter"638&state=*&q=&archive=&delegate> >
> > I find such a situation also interesting somehow.
> > I assume that there was another temporary technical difficulty involved
> > with the Linux mailing list (and related information systems).
>
>
> I think v6 fell into a crack somehow.
>
>
> Check the LKML archive:
> https://lkml.org/lkml/2019/2/16
>
>
> I see your reply:
>
> Re: [v6] coccinelle: semantic code search for missing put_device()Markus Elfring
>
>
> But, I cannot find the v6 patch itself.
>
>
>
> >
> > > So, I just thought v5 was the latest one
> > > and I was completely missing the context.
> >
> > Did any messages for the sixth (and seventh) script version reach you?
> >
> > https://lore.kernel.org/cocci/201902191014156680299@zte.com.cn/
>
>
> I use both my corporate email address and Gmail,
> and play it by ear.
>
>
> I see it in my corporate mail box now.
> It reached me probably because I subscribe to cocci@systeme.lip6.fr
> with my socionext email address.
>
> However, this time I only checked my Gmail box,
> which subscribes to linux-kernel ML, but not cocci ML.
>
> I just skimmed over the thread, then
> I missed the fact you were giving some comments about v6.
>
>
>
> > https://lore.kernel.org/lkml/1553321671-27749-1-git-send-email-wen.yang99@zte.com.cn/
>
>
> Yes. I see this one.
>
> I will apply it to fixes branch
> if nobody sees a problem in it.

This one is fine independent of anything else.

thanks,
julia

Re: [v5] Coccinelle: semantic code search for missing put_device()

[Julia Lawall]

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



On Sat, 16 Feb 2019, wen.yang99@zte.com.cn wrote:

> >>> Does the first SmPL when specification include the case that a call
> >>> of the function “put_device” can occur within a branch of an if statement?
> >>
> >> It does include that,
> >
> >Thanks for this acknowledgement.
> >
> >So it seems that you find my interpretation of this bit of SmPL code appropriate.
> >
> >> but there is another execution path where the put device is not present.
> >
> >It is tried to find such cases.
> >
> >> But given the test in the if in the when code,
> >> on that execution path id is NULL, an so there is no need to put it.
> >
> >I would like to point out that the function “put_device” belongs also to
> >the category of functions which tolerate the passing of null pointers.
> >https://elixir.bootlin.com/linux/v5.0-rc6/source/drivers/base/core.c#L2053
> >https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/core.c?id=cb5b020a8d38f77209d0472a0fea755299a8ec78#n2053
> >
> >Have we got still different software development opinions about the need
> >for an extra pointer check in the “second” SmPL when specification?
>
> Thanks to Julia and Markus.
> We will modify the the if in the when code like this:
>
> @@ -22,7 +22,7 @@ if (id == NULL || ...) { ... return ...; }
>  ... when != put_device(&id->dev)
>      when != platform_device_put(id)
>      when != of_dev_put(id)
> -    when != if (id) { ... put_device(&id->dev) ... }
> +    when != if (...) { ... put_device(&id->dev) ... }

This looks ok.  You can check the old version and the new version and see
if there is any differencein the set of reports.

julia

Re: [v5] Coccinelle: semantic code search for missing put_device()

[Markus Elfring]

>> We will modify the the if in the when code like this:
>>
>> @@ -22,7 +22,7 @@ if (id == NULL || ...) { ... return ...; }
>>  ... when != put_device(&id->dev)
…
>> -    when != if (id) { ... put_device(&id->dev) ... }
>> +    when != if (...) { ... put_device(&id->dev) ... }
>
> This looks ok.

I have got another different software development opinion also for
such SmPL code in the adjusted line.
I find this specific source code search variant irrelevant
because the shown reference release function should be found
by the first SmPL when specification already.
Would you like to determine generally if the desired function call
is present at all?

Thus I do not see a need (or requirement) for a duplicate search attempt.

Regards,
Markus

Re: [v5] Coccinelle: semantic code search for missing put_device()

[Julia Lawall]

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



On Sat, 16 Feb 2019, Markus Elfring wrote:

> >> We will modify the the if in the when code like this:
> >>
> >> @@ -22,7 +22,7 @@ if (id == NULL || ...) { ... return ...; }
> >>  ... when != put_device(&id->dev)
> …
> >> -    when != if (id) { ... put_device(&id->dev) ... }
> >> +    when != if (...) { ... put_device(&id->dev) ... }
> >
> > This looks ok.
>
> I have got another different software development opinion also for
> such SmPL code in the adjusted line.
> I find this specific source code search variant irrelevant
> because the shown reference release function should be found
> by the first SmPL when specification already.
> Would you like to determine generally if the desired function call
> is present at all?
>
> Thus I do not see a need (or requirement) for a duplicate search attempt.

Why don't you actually try it and see what the difference is rather than
repeatedly giving false information?

julia

Re: [v5] Coccinelle: semantic code search for missing put_device()

[Markus Elfring]

>> Thus I do not see a need (or requirement) for a duplicate search attempt.
>
> Why don't you actually try it and see what the difference is rather than
> repeatedly giving false information?

I suggest to clarify this software development disagreement by the following
SmPL code.


 ... when != put_stuff(my_pointer)
     when != if (...) { ... put_stuff(my_pointer) ... }


Is this a specification for a bit of duplicate code?


Or:
Do you insist to find such a function call only within a branch of an if statement?

Regards,
Markus