[cocci] [PATCH v2] coccinelle: ifaddr: Find address test in more complex conditions

[cocci] [PATCH v2] coccinelle: ifaddr: Find address test in more complex conditions

[Jérémy LEFAURE]

The test of an expression's address does not necessarily represent the
whole condition, it may only be a part of it.
This change aims at detecting an address test in more complex conditions.

Signed-off-by: Jérémy Lefaure <jeremy.lefaure@netatmo.com>
---
v1 -> v2: Moved disjunction on the condition itself instead of being on the if statements

 scripts/coccinelle/misc/ifaddr.cocci | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/coccinelle/misc/ifaddr.cocci b/scripts/coccinelle/misc/ifaddr.cocci
index fc92e8fcbfcb..14dbf3131a3f 100644
--- a/scripts/coccinelle/misc/ifaddr.cocci
+++ b/scripts/coccinelle/misc/ifaddr.cocci
@@ -18,7 +18,7 @@ statement S1,S2;
 position p;
 @@
 
-*if@p (&x)
+*if@p ( \(&x || ... \| &x && ... \) )
  S1 else S2
 
 @script:python depends on org@
-- 
2.25.1

Re: [cocci] [PATCH v2] coccinelle: ifaddr: Find address test in more complex conditions

[Julia Lawall]

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



On Tue, 7 Jun 2022, Jérémy LEFAURE wrote:

> The test of an expression's address does not necessarily represent the
> whole condition, it may only be a part of it.
> This change aims at detecting an address test in more complex conditions.
>
> Signed-off-by: Jérémy Lefaure <jeremy.lefaure@netatmo.com>
> ---
> v1 -> v2: Moved disjunction on the condition itself instead of being on the if statements

Thanks for the change.  It seems to cover all the cases.

julia

>
>  scripts/coccinelle/misc/ifaddr.cocci | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/coccinelle/misc/ifaddr.cocci b/scripts/coccinelle/misc/ifaddr.cocci
> index fc92e8fcbfcb..14dbf3131a3f 100644
> --- a/scripts/coccinelle/misc/ifaddr.cocci
> +++ b/scripts/coccinelle/misc/ifaddr.cocci
> @@ -18,7 +18,7 @@ statement S1,S2;
>  position p;
>  @@
>
> -*if@p (&x)
> +*if@p ( \(&x || ... \| &x && ... \) )
>   S1 else S2
>
>  @script:python depends on org@
> --
> 2.25.1
>

Re: [cocci] [PATCH v2] coccinelle: ifaddr: Find address test in more complex conditions

[Markus Elfring]

> The test of an expression's address does not necessarily represent the
> whole condition, it may only be a part of it.
> This change aims at detecting an address test in more complex conditions.
>
> Signed-off-by: Jérémy Lefaure <jeremy.lefaure@netatmo.com>
> ---
> v1 -> v2: Moved disjunction on the condition itself instead of being on the if statements
>
>  scripts/coccinelle/misc/ifaddr.cocci | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/coccinelle/misc/ifaddr.cocci b/scripts/coccinelle/misc/ifaddr.cocci
> index fc92e8fcbfcb..14dbf3131a3f 100644
> --- a/scripts/coccinelle/misc/ifaddr.cocci
> +++ b/scripts/coccinelle/misc/ifaddr.cocci
> @@ -18,7 +18,7 @@ statement S1,S2;
>  position p;
>  @@
>
> -*if@p (&x)
> +*if@p ( \(&x || ... \| &x && ... \) )
>   S1 else S2
>
>  @script:python depends on org@


Would you ever dare to adjust the specification another bit for such
a source code search approach according to the difference in the selection
of the operators “||” (disjunction) and “&&” (conjunction)?

Regards,
Markus

Re: [cocci] [PATCH v2] coccinelle: ifaddr: Find address test in more complex conditions

[Julia Lawall]

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



On Tue, 7 Jun 2022, Markus Elfring wrote:

>
> > The test of an expression's address does not necessarily represent the
> > whole condition, it may only be a part of it.
> > This change aims at detecting an address test in more complex conditions.
> >
> > Signed-off-by: Jérémy Lefaure <jeremy.lefaure@netatmo.com>
> > ---
> > v1 -> v2: Moved disjunction on the condition itself instead of being on the if statements
> >
> >  scripts/coccinelle/misc/ifaddr.cocci | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/scripts/coccinelle/misc/ifaddr.cocci b/scripts/coccinelle/misc/ifaddr.cocci
> > index fc92e8fcbfcb..14dbf3131a3f 100644
> > --- a/scripts/coccinelle/misc/ifaddr.cocci
> > +++ b/scripts/coccinelle/misc/ifaddr.cocci
> > @@ -18,7 +18,7 @@ statement S1,S2;
> >  position p;
> >  @@
> >
> > -*if@p (&x)
> > +*if@p ( \(&x || ... \| &x && ... \) )
> >   S1 else S2
> >
> >  @script:python depends on org@
>
>
> Would you ever dare to adjust the specification another bit for such
> a source code search approach according to the difference in the selection
> of the operators “||” (disjunction) and “&&” (conjunction)?

I don't really understand the above, but it made me realize that actually
the following is sufficient:

@@
expression x;
@@

*&x || ...

This forces &x to appear as a test expression.  There can be 0 or more
occurrences of ||, so eg &x && y is also matched.

julia

Re: [cocci] [PATCH v2] coccinelle: ifaddr: Find address test in more complex conditions

[Jérémy LEFAURE]

> I don't really understand the above, but it made me realize that actually
> the following is sufficient:
>
> @@
> expression x;
> @@
>
> *&x || ...
>
> This forces &x to appear as a test expression. There can be 0 or more
> occurrences of ||, so eg &x && y is also matched.

That's indeed better because it allows to catch issues in tests that
are not in if statements (such as loops). Is it ok to rename this file
to test_addr.cocci in the next version of the patch to give it a more
appropriated name?

It also gives a better line indication for conditions split on several
lines.

I also noticed that with this change the semantic patch catches tests of
addresses in if statements when likely/unlikely is used, for example
if (likely(&a)). This was not the case with my previous patch nor with
the current semantic patch version on master even though the isomorphism
unlikely(E) <=> likely(E) => E is in the standard.iso file. I don't get
this behavior change, do you have any idea why it didn't work?


Thank you!
Jérémy

________________________________________
From: Julia Lawall <julia.lawall@inria.fr>
Sent: Tuesday, June 7, 2022 21:39
To: Markus Elfring
Cc: Jérémy LEFAURE; cocci@inria.fr; kernel-janitors@vger.kernel.org; Julia Lawall; Nicolas Palix
Subject: Re: [PATCH v2] coccinelle: ifaddr: Find address test in more complex conditions



On Tue, 7 Jun 2022, Markus Elfring wrote:

>
> > The test of an expression's address does not necessarily represent the
> > whole condition, it may only be a part of it.
> > This change aims at detecting an address test in more complex conditions.
> >
> > Signed-off-by: Jérémy Lefaure <jeremy.lefaure@netatmo.com>
> > ---
> > v1 -> v2: Moved disjunction on the condition itself instead of being on the if statements
> >
> >  scripts/coccinelle/misc/ifaddr.cocci | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/scripts/coccinelle/misc/ifaddr.cocci b/scripts/coccinelle/misc/ifaddr.cocci
> > index fc92e8fcbfcb..14dbf3131a3f 100644
> > --- a/scripts/coccinelle/misc/ifaddr.cocci
> > +++ b/scripts/coccinelle/misc/ifaddr.cocci
> > @@ -18,7 +18,7 @@ statement S1,S2;
> >  position p;
> >  @@
> >
> > -*if@p (&x)
> > +*if@p ( \(&x || ... \| &x && ... \) )
> >   S1 else S2
> >
> >  @script:python depends on org@
>
>
> Would you ever dare to adjust the specification another bit for such
> a source code search approach according to the difference in the selection
> of the operators “||” (disjunction) and “&&” (conjunction)?

I don't really understand the above, but it made me realize that actually
the following is sufficient:

@@
expression x;
@@

*&x || ...

This forces &x to appear as a test expression.  There can be 0 or more
occurrences of ||, so eg &x && y is also matched.

julia

Re: [cocci] [PATCH v2] coccinelle: ifaddr: Find address test in more complex conditions

[Julia Lawall]

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



On Wed, 8 Jun 2022, Jérémy LEFAURE wrote:

> > I don't really understand the above, but it made me realize that actually
> > the following is sufficient:
> >
> > @@
> > expression x;
> > @@
> >
> > *&x || ...
> >
> > This forces &x to appear as a test expression. There can be 0 or more
> > occurrences of ||, so eg &x && y is also matched.
>
> That's indeed better because it allows to catch issues in tests that
> are not in if statements (such as loops). Is it ok to rename this file
> to test_addr.cocci in the next version of the patch to give it a more
> appropriated name?

That seems like a good idea, thanks.

>
> It also gives a better line indication for conditions split on several
> lines.
>
> I also noticed that with this change the semantic patch catches tests of
> addresses in if statements when likely/unlikely is used, for example
> if (likely(&a)). This was not the case with my previous patch nor with
> the current semantic patch version on master even though the isomorphism
> unlikely(E) <=> likely(E) => E is in the standard.iso file. I don't get
> this behavior change, do you have any idea why it didn't work?

Isomorphisms apply to the semantic patch, not to the code.  You see the
arrow only going to the right that reaches E.  If it were going to the
left, then Coccinellewould take every expression and try to wrap likely
and unlikely around it, which would obviously be undesirable.

julia

Re: [cocci] coccinelle: ifaddr: Find address test in more complex conditions

[Markus Elfring]

> Would you ever dare to adjust the specification another bit for such
> a source code search approach according to the difference in the selection
> of the operators “||” (disjunction) and “&&” (conjunction)?
> I don't really understand the above,


I proposed to take additional software design possibilities into account.


Is there a need to reconsider also the usage of the SmPL asterisk before
data processing for the operation modes “org” and “report”?


> but it made me realize that actually the following is sufficient:


I find this response nice.



> @@
> expression x;
> @@
>
> *&x || ...
>
> This forces &x to appear as a test expression.


Will such an use case become known better?



> There can be 0 or more occurrences of ||,


How do you call the transformation which supports the omission of
a trailing binary operator (according to the discussed combination
with a SmPL ellipsis)?



> so eg &x && y is also matched.


Will this conclusion influence any adjustments for the software documentation?

Regards,
Markus

Re: [cocci] coccinelle: ifaddr: Find address test in more complex conditions

[Markus Elfring]

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

>> I don't really understand the above, but it made me realize that actually
>> the following is sufficient:
>>
>> @@
>> expression x;
>> @@
>>
>> *&x || ...
>>
>> This forces &x to appear as a test expression.  There can be 0 or more
>> occurrences of ||, so eg &x && y is also matched.


How does this promising feedback fit to the following information from
the software “Coccinelle 1.1.1” (OCaml 4.14.0)?

Markus_Elfring@Sonne:/home/altes_Heim2/elfring/Projekte/Coccinelle/Probe> spatch --parse-cocci show_address_determination_in_checks-20220609.cocci
…
@display@
expression x;
@@


(
*&*x *|| *...
|
*&*x *!= *NULL *|| *...
|
*&*x *|| *...
|
*NULL *!= *&*x *|| *...
)


No grep query


Would another transformation be needed for the omission of trailing binary operators
(according to the shown combinations with SmPL ellipses)?


Regards,
Markus

[-- Attachment #2: Type: text/html, Size: 1924 bytes --]

Re: [cocci] coccinelle: ifaddr: Find address test in more complex conditions

[Julia Lawall]

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



On Thu, 9 Jun 2022, Markus Elfring wrote:

>
> I don't really understand the above, but it made me realize that actually
> the following is sufficient:
>
> @@
> expression x;
> @@
>
> *&x || ...
>
> This forces &x to appear as a test expression.  There can be 0 or more
> occurrences of ||, so eg &x && y is also matched.
>
>
> How does this promising feedback fit to the following information from
> the software “Coccinelle 1.1.1” (OCaml 4.14.0)?
>
> Markus_Elfring@Sonne:/home/altes_Heim2/elfring/Projekte/Coccinelle/Probe> spatch --parse-cocci show_address_determination_in_checks-20220609.cocci
> …
> @display@
> expression x;
> @@
>
>
> (
> *&*x *|| *...
> |
> *&*x *!= *NULL *|| *...
> |
> *&*x *|| *...
> |
> *NULL *!= *&*x *|| *...
> )
>
>
> No grep query
>
>
> Would another transformation be needed for the omission of trailing binary operators
> (according to the shown combinations with SmPL ellipses)?

I don't know what you mean.  How about trying an example?

julia

Re: [cocci] coccinelle: ifaddr: Find address test in more complex conditions

[Markus Elfring]

>> Markus_Elfring@Sonne:/home/altes_Heim2/elfring/Projekte/Coccinelle/Probe> spatch --parse-cocci show_address_determination_in_checks-20220609.cocci
>> …
>> @display@
>> expression x;
>> @@
>>
>>
>> (
>> *&*x *|| *...
>> |
>> *&*x *!= *NULL *|| *...
>> |
>> *&*x *|| *...
>> |
>> *NULL *!= *&*x *|| *...
>> )
…
>> Would another transformation be needed for the omission of trailing binary operators
>> (according to the shown combinations with SmPL ellipses)?
> I don't know what you mean.


The results from the application of some isomorphism are shown by the mentioned command.



> How about trying an example?


You might interpret the following test result as an expected one according to
your understanding of implementation details.

Markus_Elfring@Sonne:/home/altes_Heim2/elfring/Projekte/Coccinelle/Probe> spatch show_address_determination_in_checks-20220609.cocci initialisation_test2-20220609.c
…
@@ -1,8 +1,6 @@
 void check(void)
 {
 int s = 1;
-bool t = &s ? 0 : 1;
 bool u;
-u = &t ? 1 : 0;
 }



I observed that no questionable places were marked in the following source code.


void check(void)
{
int s = 1;
bool t = (&s);
}


Will this test case trigger further development considerations?

Regards,
Markus

Re: [cocci] coccinelle: ifaddr: Find address test in more complex conditions

[Julia Lawall]

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



On Thu, 9 Jun 2022, Markus Elfring wrote:

> >> Markus_Elfring@Sonne:/home/altes_Heim2/elfring/Projekte/Coccinelle/Probe> spatch --parse-cocci show_address_determination_in_checks-20220609.cocci
> >> …
> >> @display@
> >> expression x;
> >> @@
> >>
> >>
> >> (
> >> *&*x *|| *...
> >> |
> >> *&*x *!= *NULL *|| *...
> >> |
> >> *&*x *|| *...
> >> |
> >> *NULL *!= *&*x *|| *...
> >> )
> …
> >> Would another transformation be needed for the omission of trailing binary operators
> >> (according to the shown combinations with SmPL ellipses)?
> > I don't know what you mean.
>
>
> The results from the application of some isomorphism are shown by the mentioned command.
>
>
>
> > How about trying an example?
>
>
> You might interpret the following test result as an expected one according to
> your understanding of implementation details.
>
> Markus_Elfring@Sonne:/home/altes_Heim2/elfring/Projekte/Coccinelle/Probe> spatch show_address_determination_in_checks-20220609.cocci initialisation_test2-20220609.c
> …
> @@ -1,8 +1,6 @@
>  void check(void)
>  {
>  int s = 1;
> -bool t = &s ? 0 : 1;
>  bool u;
> -u = &t ? 1 : 0;
>  }
>
>
>
> I observed that no questionable places were marked in the following source code.
>
>
> void check(void)
> {
> int s = 1;
> bool t = (&s);
> }
>
>
> Will this test case trigger further development considerations?

It seems to be doing what it should.

julia

Re: [cocci] coccinelle: ifaddr: Find address test in more complex conditions

[Markus Elfring]

>>> How about trying an example?
>> You might interpret the following test result as an expected one according to
>> your understanding of implementation details.
>>
>> Markus_Elfring@Sonne:/home/altes_Heim2/elfring/Projekte/Coccinelle/Probe> spatch show_address_determination_in_checks-20220609.cocci initialisation_test2-20220609.c
>> …
>> @@ -1,8 +1,6 @@
>>  void check(void)
>>  {
>>  int s = 1;
>> -bool t = &s ? 0 : 1;
>>  bool u;
>> -u = &t ? 1 : 0;
>>  }


Can it be that special transformations (which are not represented as a parsing result
from the application of named isomorphisms) were applied here?



>> I observed that no questionable places were marked in the following source code.
>>
>>
>> void check(void)
>> {
>> int s = 1;
>> bool t = (&s);
>> }
>>
>>
>> Will this test case trigger further development considerations?
> It seems to be doing what it should.

I would expect that the usage of the address-of operator within extra parentheses
should also be marked for reconsideration of such source code.

Regards,
Markus

Re: [cocci] coccinelle: ifaddr: Find address test in more complex conditions

[Julia Lawall]

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



On Thu, 9 Jun 2022, Markus Elfring wrote:

> >>> How about trying an example?
> >> You might interpret the following test result as an expected one according to
> >> your understanding of implementation details.
> >>
> >> Markus_Elfring@Sonne:/home/altes_Heim2/elfring/Projekte/Coccinelle/Probe> spatch show_address_determination_in_checks-20220609.cocci initialisation_test2-20220609.c
> >> …
> >> @@ -1,8 +1,6 @@
> >>  void check(void)
> >>  {
> >>  int s = 1;
> >> -bool t = &s ? 0 : 1;
> >>  bool u;
> >> -u = &t ? 1 : 0;
> >>  }
>
>
> Can it be that special transformations (which are not represented as a parsing result
> from the application of named isomorphisms) were applied here?
>
>
>
> >> I observed that no questionable places were marked in the following source code.
> >>
> >>
> >> void check(void)
> >> {
> >> int s = 1;
> >> bool t = (&s);
> >> }
> >>
> >>
> >> Will this test case trigger further development considerations?
> > It seems to be doing what it should.
>
> I would expect that the usage of the address-of operator within extra parentheses
> should also be marked for reconsideration of such source code.

It's not a test expression.  The bool type is not relevant.

julia

Re: [cocci] coccinelle: ifaddr: Find address test in more complex conditions

[Markus Elfring]

>> I would expect that the usage of the address-of operator within extra parentheses
>> should also be marked for reconsideration of such source code.
> It's not a test expression.  The bool type is not relevant.


It seems that we stumble on opposite views for the involved expression “complexity”.

Regards,
Markus