[cocci] [PATCH v5] coccinelle: Extend address test from ifaddr semantic patch to test expressions

[cocci] [PATCH v5] coccinelle: Extend address test from ifaddr semantic patch to test expressions

[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. Also, an expression's
address is likely to be non-zero in every test expression, not only in
if statements.

This change aims at detecting an address test in more complex conditions
and not only in if statements.

Signed-off-by: Jérémy Lefaure <jeremy.lefaure@netatmo.com>
---
v4 -> v5: Change semantic patch to match explicit tests to NULL +
improve commit message
v3 -> v4: Improve patch subject
v2 -> v3: Apply Julia's suggestion to have a more generic solution + adapt
commit message and file name to this new solution.
v1 -> v2: Moved disjunction on the condition itself instead of being on the
if statements.

 scripts/coccinelle/misc/{ifaddr.cocci => test_addr.cocci} | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
 rename scripts/coccinelle/misc/{ifaddr.cocci => test_addr.cocci} (94%)

diff --git a/scripts/coccinelle/misc/ifaddr.cocci b/scripts/coccinelle/misc/test_addr.cocci
similarity index 94%
rename from scripts/coccinelle/misc/ifaddr.cocci
rename to scripts/coccinelle/misc/test_addr.cocci
index fc92e8fcbfcb..029db9069c44 100644
--- a/scripts/coccinelle/misc/ifaddr.cocci
+++ b/scripts/coccinelle/misc/test_addr.cocci
@@ -14,12 +14,10 @@ virtual context
 
 @r@
 expression x;
-statement S1,S2;
 position p;
 @@
 
-*if@p (&x)
- S1 else S2
+*\(&x@p == NULL \| &x@p != NULL\)
 
 @script:python depends on org@
 p << r.p;
-- 
2.25.1

Re: [cocci] [PATCH v5] coccinelle: Extend address test from ifaddr semantic patch to test expressions

[Markus Elfring]

> This change aims at detecting an address test in more complex conditions
> and not only in if statements.
>
> Signed-off-by: Jérémy Lefaure <jeremy.lefaure@netatmo.com>
> ---
> v4 -> v5: Change semantic patch to match explicit tests to NULL +
> improve commit message
> v3 -> v4: Improve patch subject
> v2 -> v3: Apply Julia's suggestion to have a more generic solution + adapt
> commit message and file name to this new solution.
> v1 -> v2: Moved disjunction on the condition itself instead of being on the
> if statements.
>
>  scripts/coccinelle/misc/{ifaddr.cocci => test_addr.cocci} | 4 +---
…
> @@ -14,12 +14,10 @@ virtual context
>
>  @r@
>  expression x;
> -statement S1,S2;
>  position p;
>  @@
>
> -*if@p (&x)
> - S1 else S2
> +*\(&x@p == NULL \| &x@p != NULL\)

…


Further code review concerns:
A)
I interpret such SmPL code as an undesirable contradiction.

Changes are generally context-dependent.
Thus context data are accordingly required also for the usage of structure isomorphisms.
https://gitlab.inria.fr/coccinelle/coccinelle/-/blob/02d51a763c8bcce054e065062e43b9b196840159/standard.iso#L421
https://github.com/coccinelle/coccinelle/blob/19ee1697bf152d37a78a20cefe148775bf4b0e0d/standard.iso#L421

I would prefer to extend the SmPL search pattern
so that more case distinctions will be taken better into account.


B) Reminder:
I find the sharing of SmPL asterisk functionality questionable here
for subsequent SmPL rules.


C)
The discussed SmPL script supports only the operation modes “org” and “report” so far.
Thus I find the term “semantic match” more appropriate instead of “semantic patch”
in the message subject.

Regards,
Markus

Re: [cocci] [PATCH v5] coccinelle: Extend address test from ifaddr semantic patch to test expressions

[Markus Elfring]

> This change aims at detecting an address test in more complex conditions
> and not only in if statements.
…
> v4 -> v5: Change semantic patch to match explicit tests to NULL +
> improve commit message
…
>  scripts/coccinelle/misc/{ifaddr.cocci => test_addr.cocci} | 4 +---
…
> @@ -14,12 +14,10 @@ virtual context
>
>  @r@
>  expression x;
> -statement S1,S2;
>  position p;
>  @@
>
> -*if@p (&x)
> - S1 else S2
> +*\(&x@p == NULL \| &x@p != NULL\)

…


Can another SmPL code example help with the clarification of proposed
implementation details?

@display@
expression x;
binary operator bo = { ==, != };
@@
* &x bo NULL


Regards,
Markus

Re: [PATCH v5] coccinelle: Extend address test from ifaddr semantic patch to test expressions

[Julia Lawall]

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



On Fri, 1 Jul 2022, Markus Elfring wrote:

> > This change aims at detecting an address test in more complex conditions
> > and not only in if statements.
> …
> > v4 -> v5: Change semantic patch to match explicit tests to NULL +
> > improve commit message
> …
> >  scripts/coccinelle/misc/{ifaddr.cocci => test_addr.cocci} | 4 +---
> …
> > @@ -14,12 +14,10 @@ virtual context
> >
> >  @r@
> >  expression x;
> > -statement S1,S2;
> >  position p;
> >  @@
> >
> > -*if@p (&x)
> > - S1 else S2
> > +*\(&x@p == NULL \| &x@p != NULL\)
>
> …
>
>
> Can another SmPL code example help with the clarification of proposed
> implementation details?
>
> @display@
> expression x;
> binary operator bo = { ==, != };
> @@
> * &x bo NULL

Markus, did you actually try this?  Please try things before suggesting
them.  The whole point of the proposed change is to trigger some
isomorphisms.  I really doubt that any isomorphisms will be triggered with
this suggestion.

julia

Re: [cocci] [PATCH v5] coccinelle: Extend address test from ifaddr semantic patch to test expressions

[Julia Lawall]

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



On Fri, 1 Jul 2022, Markus Elfring wrote:

> > This change aims at detecting an address test in more complex conditions
> > and not only in if statements.
> …
> > v4 -> v5: Change semantic patch to match explicit tests to NULL +
> > improve commit message
> …
> >  scripts/coccinelle/misc/{ifaddr.cocci => test_addr.cocci} | 4 +---
> …
> > @@ -14,12 +14,10 @@ virtual context
> >
> >  @r@
> >  expression x;
> > -statement S1,S2;
> >  position p;
> >  @@
> >
> > -*if@p (&x)
> > - S1 else S2
> > +*\(&x@p == NULL \| &x@p != NULL\)
>
> …
>
>
> Can another SmPL code example help with the clarification of proposed
> implementation details?
>
> @display@
> expression x;
> binary operator bo = { ==, != };
> @@
> * &x bo NULL

Markus, did you actually try this?  Please try things before suggesting
them.  The whole point of the proposed change is to trigger some
isomorphisms.  I really doubt that any isomorphisms will be triggered with
this suggestion.

julia

Re: [cocci] [v5] coccinelle: Extend address test from ifaddr semantic patch to test expressions

[Markus Elfring]

>> Can another SmPL code example help with the clarification of proposed
>> implementation details?
>>
>> @display@
>> expression x;
>> binary operator bo = { ==, != };
>> @@
>> * &x bo NULL
> Markus, did you actually try this?


A command part like “spatch --parse-cocci ….cocci” will show a bit of information
as usual.


> Please try things before suggesting them.


I suggest to take another look also at related SmPL code variations.


> The whole point of the proposed change is to trigger some isomorphisms.
> I really doubt that any isomorphisms will be triggered with
> this suggestion.


Will such a desire influence corresponding descriptions any further?


Would you dare to follow the presented design direction any further
so that even the explicit source code filter for equal/unequal binary operators
could be omitted (according to the combination with the address-of operator)?


Will any related software limitations be adjusted?

Regards,
Markus

Re: [cocci] [PATCH v5] coccinelle: Extend address test from ifaddr semantic patch to test expressions

[Markus Elfring]

> The whole point of the proposed change is to trigger some isomorphisms.
> I really doubt that any isomorphisms will be triggered with this suggestion.


Such expectations are interesting for further clarification, aren't they?

Will any extensions become relevant also for the handling of isomorphisms
by the Coccinelle software?

Would you like to extend the following SmPL code variant anyhow
so that more case distinctions will be taken better into account?

@display@
statement is, es;
expression x;
binary operator bo;
@@
(
*if (&x)
    is
 else
    es
|
* &x bo NULL
|
* NULL bo &x
|
* !(&x)
)


Regards,
Markus

Re: [PATCH v5] coccinelle: Extend address test from ifaddr semantic patch to test expressions

[Julia Lawall]

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

It seems that your resoning is "I wish Coccinelle would work this way, so
I will suggest an "improvement" to a proposed semantic patch that actually
currently has inferior behavior as compared to the original one (but not
mention that at all in the message), and hope that the Coccinelle
developers will think that adding the functionality that you hope for is a
very important priority".  If you want a feature to be added, ask for the
feature.  But please stop confusing the discussion about semantic patches
that are proposed by other people.

julia

On Sat, 2 Jul 2022, Markus Elfring wrote:

>
> > The whole point of the proposed change is to trigger some isomorphisms.
> > I really doubt that any isomorphisms will be triggered with this suggestion.
>
>
> Such expectations are interesting for further clarification, aren't they?
>
> Will any extensions become relevant also for the handling of isomorphisms
> by the Coccinelle software?
>
> Would you like to extend the following SmPL code variant anyhow
> so that more case distinctions will be taken better into account?
>
> @display@
> statement is, es;
> expression x;
> binary operator bo;
> @@
> (
> *if (&x)
>     is
>  else
>     es
> |
> * &x bo NULL
> |
> * NULL bo &x
> |
> * !(&x)
> )
>
>
> Regards,
> Markus
>

Re: [cocci] [PATCH v5] coccinelle: Extend address test from ifaddr semantic patch to test expressions

[Julia Lawall]

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

It seems that your resoning is "I wish Coccinelle would work this way, so
I will suggest an "improvement" to a proposed semantic patch that actually
currently has inferior behavior as compared to the original one (but not
mention that at all in the message), and hope that the Coccinelle
developers will think that adding the functionality that you hope for is a
very important priority".  If you want a feature to be added, ask for the
feature.  But please stop confusing the discussion about semantic patches
that are proposed by other people.

julia

On Sat, 2 Jul 2022, Markus Elfring wrote:

>
> > The whole point of the proposed change is to trigger some isomorphisms.
> > I really doubt that any isomorphisms will be triggered with this suggestion.
>
>
> Such expectations are interesting for further clarification, aren't they?
>
> Will any extensions become relevant also for the handling of isomorphisms
> by the Coccinelle software?
>
> Would you like to extend the following SmPL code variant anyhow
> so that more case distinctions will be taken better into account?
>
> @display@
> statement is, es;
> expression x;
> binary operator bo;
> @@
> (
> *if (&x)
>     is
>  else
>     es
> |
> * &x bo NULL
> |
> * NULL bo &x
> |
> * !(&x)
> )
>
>
> Regards,
> Markus
>

Re: [cocci] [v5] coccinelle: Extend address test from ifaddr semantic patch to test expressions

[Markus Elfring]

> It seems that your resoning is "I wish Coccinelle would work this way,
> so I will suggest an "improvement" to a proposed semantic patch

I pointed further change possibilities out several times.


Examples:
* https://github.com/coccinelle/coccinelle/issues
* https://gitlab.inria.fr/coccinelle/coccinelle/-/issues



> that actually currently has inferior behavior as compared to the original one
> (but not mention that at all in the message),


I find this view interesting.
I hope that we would like to make the discussed SmPL script considerably better.



> and hope that the Coccinelle developers will think that adding the functionality
> that you hope for is a very important priority".


The importance will vary as usual.


> If you want a feature to be added, ask for the feature.


Would you get into the mood to extend isomorphisms (and corresponding documentation)?


> But please stop confusing the discussion about semantic patches
> that are proposed by other people.

Are any additional SmPL disjunctions and metavariable types like “binary operator”
relevant for the current SmPL code review?

Regards,
Markus

Re: [cocci] [PATCH v5] coccinelle: Extend address test from ifaddr semantic patch to test expressions

[Markus Elfring]

> If you want a feature to be added, ask for the feature.
> But please stop confusing the discussion about semantic patches
> that are proposed by other people.


Would you like to achieve that all basic coccicheck operation modes will become
properly supported for a SmPL code variant like the following?


@display@
type t;
statement is, es;
identifier i;
expression x, ix, ex, z;
binary operator bo;
@@
(
*if (&(x))
    is
 else
    es
|
* &(x) bo NULL
|
* NULL bo &(x)
|
* !(&(x))
|
* (&(x))
  ? (ix)
  : (ex)
|
 t i =
* (&(x))
 ;
|
 (z =
* &(x)
 )
)


Regards,
Markus

Re: [cocci] [PATCH v5] coccinelle: Extend address test from ifaddr semantic patch to test expressions

[Markus Elfring]

> ---
> v4 -> v5: Change semantic patch to match explicit tests to NULL +
> improve commit message


I find it sufficient to specify the version list without arrows.
Would you like to avoid the repetition of version numbers in the change log?

Regards,
Markus

Re: [cocci] [PATCH v5] coccinelle: Extend address test from ifaddr semantic patch to test expressions

[Julia Lawall]

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



On Fri, 1 Jul 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. Also, an expression's
> address is likely to be non-zero in every test expression, not only in
> if statements.
>
> This change aims at detecting an address test in more complex conditions
> and not only in if statements.
>
> Signed-off-by: Jérémy Lefaure <jeremy.lefaure@netatmo.com>
> ---
> v4 -> v5: Change semantic patch to match explicit tests to NULL +
> improve commit message
> v3 -> v4: Improve patch subject
> v2 -> v3: Apply Julia's suggestion to have a more generic solution + adapt
> commit message and file name to this new solution.
> v1 -> v2: Moved disjunction on the condition itself instead of being on the
> if statements.

Applied, thanks.

julia

>
>  scripts/coccinelle/misc/{ifaddr.cocci => test_addr.cocci} | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>  rename scripts/coccinelle/misc/{ifaddr.cocci => test_addr.cocci} (94%)
>
> diff --git a/scripts/coccinelle/misc/ifaddr.cocci b/scripts/coccinelle/misc/test_addr.cocci
> similarity index 94%
> rename from scripts/coccinelle/misc/ifaddr.cocci
> rename to scripts/coccinelle/misc/test_addr.cocci
> index fc92e8fcbfcb..029db9069c44 100644
> --- a/scripts/coccinelle/misc/ifaddr.cocci
> +++ b/scripts/coccinelle/misc/test_addr.cocci
> @@ -14,12 +14,10 @@ virtual context
>
>  @r@
>  expression x;
> -statement S1,S2;
>  position p;
>  @@
>
> -*if@p (&x)
> - S1 else S2
> +*\(&x@p == NULL \| &x@p != NULL\)
>
>  @script:python depends on org@
>  p << r.p;
> --
> 2.25.1
>

Re: [cocci] [v5] coccinelle: Extend address test from ifaddr semantic patch to test expressions

[Markus Elfring]

>> This change aims at detecting an address test in more complex conditions
>> and not only in if statements.
…
>> v4 -> v5: Change semantic patch to match explicit tests to NULL +
>> improve commit message
…
> Applied, thanks.


Another intermediate software development result was achieved.


Would you eventually like to care for remaining change possibilities
according to the discussed SmPL code variants?

Regards,
Markus

Re: [cocci] [PATCH v5] coccinelle: Extend address test from ifaddr semantic patch to test expressions

[Markus Elfring]

>> This change aims at detecting an address test in more complex conditions
>> and not only in if statements.
…
>> v4 -> v5: Change semantic patch to match explicit tests to NULL +
>> improve commit message
…
>> Applied, thanks.


How do you think about to support more case distinctions for the discussed
source code search pattern (including also the detection and special handling
for pointer data types)?


@display disable decl_init@
type t;
statement is, es;
identifier i;
expression x, ix, ex, z;
binary operator bo;
@@
(
*if (&x)
    is
 else
    es
|
* &x bo NULL
|
* NULL bo &x
|
* !(&x)
|
* (&x)
  ? ix
  : ex
|
(
 t* i = &x;
|
 t* i;
 ... when != i = z
 i = &x;
|
 t i =
* (&x)
 ;
)
|
 (z =
* (&x)
 )
)



Regards,
Markus