Re: [4/5] Coccinelle: put_device: Extend when constraints for twoSmPL ellipses

Re: [4/5] Coccinelle: put_device: Extend when constraints for twoSmPL ellipses

[Markus Elfring]

> I did another experiment at that time and found that this modification will
> reduce the false positive rate,

Thanks for such feedback.

Will my update suggestion influence the current (or future) software situation?


> but it may also reduce the recall rate.

Would you like to explain this information a bit more?


> Could we use it to find out as many bugs as possible in the current kernel
> and then modify it?

I hope so.

* Will the false positive rate influence change acceptance considerably?

* Would you like to work with source code analysis approaches based on
  adjusted confidence settings?

Regards,
Markus

Re: [4/5] Coccinelle: put_device: Extend when constraints for two SmPL ellipses

[Markus Elfring]

> Could we use it to find out as many bugs as possible in the current kernel

How do you think about to work with any more source code analysis approaches?


> and then modify it?

I guess that you do not need to wait for a solution so long.

Variables which get reassigned in unwanted ways (before the desired call
of a resource release function in the discussed use case) can be found also
with the help of the semantic patch language (Coccinelle software).

Regards,
Markus

[PATCH] coccinelle: put_device: reduce false positives

[Wen Yang]

Don't complain about a return when this function returns "&pdev->dev".

Reported-by: Julia Lawall <Julia.Lawall@lip6.fr>
Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
Cc: Julia Lawall <Julia.Lawall@lip6.fr>
Cc: Gilles Muller <Gilles.Muller@lip6.fr>
Cc: Nicolas Palix <nicolas.palix@imag.fr>
Cc: Michal Marek <michal.lkml@markovi.net>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
CC: cocci@systeme.lip6.fr
Cc: linux-kernel@vger.kernel.org
---
 scripts/coccinelle/free/put_device.cocci | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/coccinelle/free/put_device.cocci b/scripts/coccinelle/free/put_device.cocci
index 7395697..c9f071b 100644
--- a/scripts/coccinelle/free/put_device.cocci
+++ b/scripts/coccinelle/free/put_device.cocci
@@ -32,6 +32,7 @@ if (id == NULL || ...) { ... return ...; }
 (    id
 |    (T2)dev_get_drvdata(&id->dev)
 |    (T3)platform_get_drvdata(id)
+|    &id->dev
 );
 | return@p2 ...;
 )
-- 
2.9.5

[PATCH 0/5] Coccinelle: put_device: Adjustments for a SmPL script

[Markus Elfring]

Some adjustments were discussed also for this script a while ago.
The software development attention evolved in a special way in the meantime.

See also for further background information:
https://lore.kernel.org/lkml/CAK7LNATjAsiSeZoTZ57zBHse0j5ZYY_12ZQ0gaF_oCziUWheOQ@mail.gmail.com/
https://systeme.lip6.fr/pipermail/cocci/2019-March/005692.html
https://lkml.org/lkml/2019/3/26/395

I would appreciate if corresponding implementation details will get
another look.

Markus Elfring (5):
  Adjust a message construction
  Add a cast to an expression for an assignment
  Merge four SmPL when constraints into one
  Extend when constraints for two SmPL ellipses
  Merge two SmPL when constraints into one

 scripts/coccinelle/free/put_device.cocci | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

--
2.21.0

[PATCH 4/5] Coccinelle: put_device: Extend when constraints for two SmPL ellipses

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 13 May 2019 09:47:17 +0200

A SmPL ellipsis was specified for a search approach so that additional
source code would be tolerated between an assignment to a local variable
and the corresponding null pointer check.

But such code should be restricted.
* The local variable must not be reassigned there.
* It must also not be forwarded to an other assignment target.

Take additional casts for these code exclusion specifications into account
together with optional parentheses.

Link: https://lore.kernel.org/cocci/201902191014156680299@zte.com.cn/
Link: https://systeme.lip6.fr/pipermail/cocci/2019-February/005620.html
Fixes: da9cfb87a44da61f2403c4312916befcb6b6d7e8 ("coccinelle: semantic code search for missing put_device()")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 scripts/coccinelle/free/put_device.cocci | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/scripts/coccinelle/free/put_device.cocci b/scripts/coccinelle/free/put_device.cocci
index aae79c02c1e0..28b0be53fb3f 100644
--- a/scripts/coccinelle/free/put_device.cocci
+++ b/scripts/coccinelle/free/put_device.cocci
@@ -13,13 +13,15 @@ virtual org
 local idexpression id;
 expression x,e,e1;
 position p1,p2;
-type T,T1,T2,T3;
+type T,T1,T2,T3,T4,T5,T6;
 @@

 id = of_find_device_by_node@p1(x)
-... when != e = id
+ ... when != e = (T4)(id)
+     when != id = (T5)(e)
 if (id == NULL || ...) { ... return ...; }
 ... when != put_device(&id->dev)
+    when != id = (T6)(e)
     when != platform_device_put(id)
     when != of_dev_put(id)
     when != if (id) { ... put_device(&id->dev) ... }
--
2.21.0

Re: [PATCH 4/5] Coccinelle: put_device: Extend when constraints for two SmPL ellipses

[Julia Lawall]

On Mon, 13 May 2019, Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 13 May 2019 09:47:17 +0200
>
> A SmPL ellipsis was specified for a search approach so that additional
> source code would be tolerated between an assignment to a local variable
> and the corresponding null pointer check.
>
> But such code should be restricted.
> * The local variable must not be reassigned there.
> * It must also not be forwarded to an other assignment target.
>
> Take additional casts for these code exclusion specifications into account
> together with optional parentheses.

NACK.  You don't need so many type metavariables.  Type metavariables in
the same ... can be the same.

julia

>
> Link: https://lore.kernel.org/cocci/201902191014156680299@zte.com.cn/
> Link: https://systeme.lip6.fr/pipermail/cocci/2019-February/005620.html
> Fixes: da9cfb87a44da61f2403c4312916befcb6b6d7e8 ("coccinelle: semantic code search for missing put_device()")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  scripts/coccinelle/free/put_device.cocci | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/coccinelle/free/put_device.cocci b/scripts/coccinelle/free/put_device.cocci
> index aae79c02c1e0..28b0be53fb3f 100644
> --- a/scripts/coccinelle/free/put_device.cocci
> +++ b/scripts/coccinelle/free/put_device.cocci
> @@ -13,13 +13,15 @@ virtual org
>  local idexpression id;
>  expression x,e,e1;
>  position p1,p2;
> -type T,T1,T2,T3;
> +type T,T1,T2,T3,T4,T5,T6;
>  @@
>
>  id = of_find_device_by_node@p1(x)
> -... when != e = id
> + ... when != e = (T4)(id)
> +     when != id = (T5)(e)
>  if (id == NULL || ...) { ... return ...; }
>  ... when != put_device(&id->dev)
> +    when != id = (T6)(e)
>      when != platform_device_put(id)
>      when != of_dev_put(id)
>      when != if (id) { ... put_device(&id->dev) ... }
> --
> 2.21.0
>
>

Re: [4/5] Coccinelle: put_device: Extend when constraints for two SmPL ellipses

[Markus Elfring]

>> Take additional casts for these code exclusion specifications into account
>> together with optional parentheses.
>
> NACK.

I find this rejection surprising.


> You don't need so many type metavariables.

I got an other software development opinion for this aspect.

Yesterday we started to clarify consequences from the isomorphism specification
“drop_cast” (for SmPL code).
https://github.com/coccinelle/coccinelle/blob/32d3b89ad909316464344a5f61a8092d8d702321/standard.iso#L52

Information like the following influenced my design decision to add three
metavariables here.

elfring@Sonne:~/Projekte/Linux/next-patched> spatch --parse-cocci scripts/coccinelle/free/put_device.cocci
…
warning: iso drop_cast does not match the code below on line -1
T (T )id

pure metavariable T is matched against the following nonpure code:
T
…


> Type metavariables in the same ... can be the same.

I would find it also occasionally nice when multiple SmPL ellipses
can refer to identical type casts.

* The under-documented “type purity” hinders this at the moment.

* But I got the impression that it can be safer to distinguish these
  code variants better.

Regards,
Markus

Re: [4/5] Coccinelle: put_device: Extend when constraints for two SmPL ellipses

[Markus Elfring]

>> A SmPL ellipsis was specified for a search approach so that additional
>> source code would be tolerated between an assignment to a local variable
>> and the corresponding null pointer check.
>>
>> But such code should be restricted.
>> * The local variable must not be reassigned there.
>> * It must also not be forwarded to an other assignment target.
>>
>> Take additional casts for these code exclusion specifications into account
>> together with optional parentheses.
>
> NACK.

Can you agree to any information which I presented in the commit message?


> You don't need so many type metavariables.

It seems that the Coccinelle software can cope also with my SmPL code addition.
You might feel uncomfortable with the suggested changes for a while.


> Type metavariables in the same ... can be the same.

Such information is good to know for the proper usage of specifications
after a SmPL ellipsis.

* Can it become required to identify involved source code placeholders
  by extra metavariables?

* Would you like to clarify the probability any more how often the shown
  type casts will be identical?

Regards,
Markus

Re: [4/5] Coccinelle: put_device: Extend when constraints for two SmPL ellipses

[Julia Lawall]

On Tue, 14 May 2019, Markus Elfring wrote:

> >> A SmPL ellipsis was specified for a search approach so that additional
> >> source code would be tolerated between an assignment to a local variable
> >> and the corresponding null pointer check.
> >>
> >> But such code should be restricted.
> >> * The local variable must not be reassigned there.
> >> * It must also not be forwarded to an other assignment target.
> >>
> >> Take additional casts for these code exclusion specifications into account
> >> together with optional parentheses.
> >
> > NACK.
>
> Can you agree to any information which I presented in the commit message?
>
>
> > You don't need so many type metavariables.
>
> It seems that the Coccinelle software can cope also with my SmPL code addition.
> You might feel uncomfortable with the suggested changes for a while.

It's ugly.  Much more ugly than msg =

>
>
> > Type metavariables in the same ... can be the same.
>
> Such information is good to know for the proper usage of specifications
> after a SmPL ellipsis.
>
> * Can it become required to identify involved source code placeholders
>   by extra metavariables?

I don't understand the question.

> * Would you like to clarify the probability any more how often the shown
>   type casts will be identical?

No idea about this one either.

Basically, if you have T && T, the two T's have to be the same, and T is
not pure.  If you have T || T, then only one will be matched and T remains
pure.  If you have T on two separate ...s then you are in the && case.  If
you have T in two branches of a disjunction or in two whens on the same
... you are in the || case.  Just as you can use the variable e1 over and
over on the same when, you can use the same T.

julia

Re: [4/5] Coccinelle: put_device: Extend when constraints for two SmPL ellipses

[Markus Elfring]

>> Can you agree to any information which I presented in the commit message?

Do you find this description inappropriate?


>>> You don't need so many type metavariables.
>>
>> It seems that the Coccinelle software can cope also with my SmPL code addition.
>> You might feel uncomfortable with the suggested changes for a while.
>
> It's ugly.  Much more ugly than msg =

The clarification of this change reluctance might become more interesting.
I got convinced that there is a need for further software updates.


>> * Can it become required to identify involved source code placeholders
>>   by extra metavariables?
>
> I don't understand the question.

Wen Yang was planning a corresponding modification since 2019-02-19.
https://lore.kernel.org/cocci/201902191014156680299@zte.com.cn/
https://systeme.lip6.fr/pipermail/cocci/2019-February/005620.html

I got into the development mood to contribute another concrete update suggestion
for an open issue in affected scripts for the semantic patch language.
Do you recognise the need for the extension of exclusion specifications here?


>> * Would you like to clarify the probability any more how often the shown
>>   type casts will be identical?
>
> No idea about this one either.

I am curious if helpful ideas will be added also by other contributors.


> Basically, if you have T && T, the two T's have to be the same,

Such an expectation might be logical.


> and T is not pure.

This information triggers various communication difficulties.


> If you have T on two separate ...s then you are in the && case.

I agree to this view also according the application of two ellipses
in the SmPL rule “search”.


> If you have T in two branches of a disjunction or in two whens on the same
> ... you are in the || case.

It is clear that disjunctions will check code alternatives here.
The clarification of consequences around the interpretation of “type purity”
might distract from the final solution.


How many (optional) type casts would we like to handle by the discussed
source code search approach?

Regards,
Markus

Re: [4/5] Coccinelle: put_device: Extend when constraints for two SmPL ellipses

[Markus Elfring]

>> Can you agree to any information which I presented in the commit message?

Do you find this description inappropriate?


>>> You don't need so many type metavariables.
>>
>> It seems that the Coccinelle software can cope also with my SmPL code addition.
>> You might feel uncomfortable with the suggested changes for a while.
>
> It's ugly.  Much more ugly than msg =

The clarification of this change reluctance might become more interesting.
I got convinced that there is a need for further software updates.


>> * Can it become required to identify involved source code placeholders
>>   by extra metavariables?
>
> I don't understand the question.

Wen Yang was planning a corresponding modification since 2019-02-19.
https://lore.kernel.org/cocci/201902191014156680299@zte.com.cn/
https://systeme.lip6.fr/pipermail/cocci/2019-February/005620.html

I got into the development mood to contribute another concrete update suggestion
for an open issue in affected scripts for the semantic patch language.
Do you recognise the need for the extension of exclusion specifications here?


>> * Would you like to clarify the probability any more how often the shown
>>   type casts will be identical?
>
> No idea about this one either.

I am curious if helpful ideas will be added also by other contributors.


> Basically, if you have T && T, the two T's have to be the same,

Such an expectation might be logical.


> and T is not pure.

This information triggers various communication difficulties.


> If you have T on two separate ...s then you are in the && case.

I agree to this view also according the application of two ellipses
in the SmPL rule “search”.


> If you have T in two branches of a disjunction or in two whens on the same
> ... you are in the || case.

It is clear that disjunctions will check code alternatives here.
The clarification of consequences around the interpretation of “type purity”
might distract from the final solution.


How many (optional) type casts would we like to handle by the discussed
source code search approach?

Regards,
Markus