[cocci] [PATCH v3] coccinelle: Extend address test from ifaddr to every test expression

[cocci] [PATCH v3] coccinelle: Extend address test from ifaddr to every test expression

[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
for every test expression.

Signed-off-by: Jérémy Lefaure <jeremy.lefaure@netatmo.com>
---
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..2d0ec86d1701 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 || ...
 
 @script:python depends on org@
 p << r.p;
-- 
2.25.1

Re: [cocci] [PATCH v3] coccinelle: Extend address test from ifaddr to every test expression

[Markus Elfring]

> 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
> for every test expression.
>
> Signed-off-by: Jérémy Lefaure <jeremy.lefaure@netatmo.com>
> ---
> 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..2d0ec86d1701 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 || ...
>
>  @script:python depends on org@
>  p << r.p;

A)
I would like to point out once more that no questionable places were marked
by the software “Coccinelle 1.1.1” (OCaml 4.14.0) in the following source code.
https://lore.kernel.org/cocci/da86bc36-36af-7bd7-6bee-861e160ba6a4@web.de/
https://sympa.inria.fr/sympa/arc/cocci/2022-06/msg00017.html


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


Thus I propose to add another case distinction by the means of
a SmPL disjunction.


*\( &x || ... \| &x \)


Will any more adjustments become helpful for the handling of optional code
according to SmPL ellipses?



B)
I imagine that generated diff output should be wrapped by a source block
for the operation mode “org”.
https://orgmode.org/quickstart.html#blocks


Regards,
Markus

Re: [cocci] [PATCH v3] coccinelle: Extend address test from ifaddr to every test expression

[Julia Lawall]

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



On Fri, 10 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. 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
> > for every test expression.
> >
> > Signed-off-by: Jérémy Lefaure <jeremy.lefaure@netatmo.com>
> > ---
> > 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..2d0ec86d1701 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 || ...
> >
> >  @script:python depends on org@
> >  p << r.p;
>
> A)
> I would like to point out once more that no questionable places were marked
> by the software “Coccinelle 1.1.1” (OCaml 4.14.0) in the following source code.
> https://lore.kernel.org/cocci/da86bc36-36af-7bd7-6bee-861e160ba6a4@web.de/
> https://sympa.inria.fr/sympa/arc/cocci/2022-06/msg00017.html
>
>
> void check(void)
> {
> int s = 1;
> bool t = (&s);
> }
>
>
> Thus I propose to add another case distinction by the means of
> a SmPL disjunction.
>
>
> *\( &x || ... \| &x \)
>
>
> Will any more adjustments become helpful for the handling of optional code
> according to SmPL ellipses?

Did you actually try this?  Because it will match every occurrence of &x.

I don't think that your example represents a real problem.

julia

>
>
>
> B)
> I imagine that generated diff output should be wrapped by a source block
> for the operation mode “org”.
> https://orgmode.org/quickstart.html#blocks
>
>
> Regards,
> Markus
>

Re: [cocci] coccinelle: Extend address test from ifaddr to every test expression

[Markus Elfring]

> void check(void)
> {
> int s = 1;
> bool t = (&s);
> }
>
>
> Thus I propose to add another case distinction by the means of
> a SmPL disjunction.
>
>
> *\( &x || ... \| &x \)
>
>
> Will any more adjustments become helpful for the handling of optional code
> according to SmPL ellipses?
…
> Did you actually try this?


My tweak demonstrates that it is possible to extend built-in software functionality.


> Because it will match every occurrence of &x.


I admit that a really complete source code search approach would be
more challenging.


> I don't think that your example represents a real problem.


I found a tiny test case which indicates some implementation details
for further considerations.
I am curious if development interests will grow accordingly.

Regards,
Markus

Re: [cocci] [PATCH v3] coccinelle: Extend address test from ifaddr to every test expression

[Jérémy LEFAURE]

> A)
> I would like to point out once more that no questionable places were marked
> by the software “Coccinelle 1.1.1” (OCaml 4.14.0) in the following source code.
> https://lore.kernel.org/cocci/da86bc36-36af-7bd7-6bee-861e160ba6a4@web.de/
> https://sympa.inria.fr/sympa/arc/cocci/2022-06/msg00017.html
>
>
> void check(void)
> {
> int s = 1;
> bool t = (&s);
> }
>
>
> Thus I propose to add another case distinction by the means of
> a SmPL disjunction.
>
>
> *\( &x || ... \| &x \)
>

Unfortunately this creates false positives as it matches address-of
operator outside of test expressions such as in this code:

void foo(int *a);

void call_foo(int a)
{
  foo(&a);
}


> B)
> I imagine that generated diff output should be wrapped by a source block
> for the operation mode “org”.
> https://orgmode.org/quickstart.html#blocks

This patch does not change the "org" mode output, it only changes the
matching part. Anyway, I am not sure that coccilib supports blocks, I
did not find any reference to block in the code [1]. Do you have an
example were it is used?

[1]: https://gitlab.inria.fr/coccinelle/coccinelle/-/blob/master/python/coccilib/org.py


Thank you,
Jérémy

Re: [cocci] [PATCH v3] coccinelle: Extend address test from ifaddr to every test expression

[Julia Lawall]

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



On Mon, 13 Jun 2022, Jérémy LEFAURE wrote:

> > A)
> > I would like to point out once more that no questionable places were marked
> > by the software “Coccinelle 1.1.1” (OCaml 4.14.0) in the following source code.
> > https://lore.kernel.org/cocci/da86bc36-36af-7bd7-6bee-861e160ba6a4@web.de/
> > https://sympa.inria.fr/sympa/arc/cocci/2022-06/msg00017.html
> >
> >
> > void check(void)
> > {
> > int s = 1;
> > bool t = (&s);
> > }
> >
> >
> > Thus I propose to add another case distinction by the means of
> > a SmPL disjunction.
> >
> >
> > *\( &x || ... \| &x \)
> >
>
> Unfortunately this creates false positives as it matches address-of
> operator outside of test expressions such as in this code:
>
> void foo(int *a);
>
> void call_foo(int a)
> {
>   foo(&a);
> }
>
>
> > B)
> > I imagine that generated diff output should be wrapped by a source block
> > for the operation mode “org”.
> > https://orgmode.org/quickstart.html#blocks
>
> This patch does not change the "org" mode output, it only changes the
> matching part. Anyway, I am not sure that coccilib supports blocks, I
> did not find any reference to block in the code [1]. Do you have an
> example were it is used?
>
> [1]: https://gitlab.inria.fr/coccinelle/coccinelle/-/blob/master/python/coccilib/org.py

No, I don't think that blocks are supported and the org mode code should
be fine as it is.  In any case, it's not relevant to this patch.

julia

Re: [cocci] [PATCH v3] coccinelle: Extend address test from ifaddr to every test expression

[Markus Elfring]

>> A)
>> I would like to point out once more that no questionable places were marked
>> by the software “Coccinelle 1.1.1” (OCaml 4.14.0) in the following source code.
>> https://lore.kernel.org/cocci/da86bc36-36af-7bd7-6bee-861e160ba6a4@web.de/
>> https://sympa.inria.fr/sympa/arc/cocci/2022-06/msg00017.html
>>
>>
>> void check(void)
>> {
>> int s = 1;
>> bool t = (&s);
>> }
>>
>>
>> Thus I propose to add another case distinction by the means of
>> a SmPL disjunction.
>>
>>
>> *\( &x || ... \| &x \)
>>
> Unfortunately this creates false positives as it matches address-of
> operator outside of test expressions such as in this code:
>
> void foo(int *a);
>
> void call_foo(int a)
> {
>   foo(&a);
> }


How do you think about to enhance desired case distinctions any further?


>> B)
>> I imagine that generated diff output should be wrapped by a source block
>> for the operation mode “org”.
>> https://orgmode.org/quickstart.html#blocks
> This patch does not change the "org" mode output,


I suggest to increase the precision for such a wording.



> it only changes the matching part.


The line is changed while it contains a SmPL asterisk in the first text column.

The subsequent SmPL script rules depend on this implementation detail
(while rules are available in other SmPL scripts which express a strict separation).


> Anyway, I am not sure that coccilib supports blocks,


Another bit of evolution might be waiting on corresponding development interests.


> I did not find any reference to block in the code [1].


This analysis result is probably appropriate so far.

Regards,
Markus

Re: [cocci] [PATCH v3] coccinelle: Extend address test from ifaddr to every test expression

[Markus Elfring]

> 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
> for every test expression.
>
> Signed-off-by: Jérémy Lefaure <jeremy.lefaure@netatmo.com>
> ---
> 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 +---


It can be determined that the proposed approach for the detection of
questionable source code places is incomplete (also because of the current
development status for the Coccinelle software).
Thus I suggest to reconsider the usage of the word “every” for the patch message
once more.
Will further adjustments become helpful for desired source code analyses
and corresponding transformations?

Regards,
Markus

Re: [cocci] [PATCH v3] coccinelle: Extend address test from ifaddr to every test expression

[Jérémy LEFAURE]

> It can be determined that the proposed approach for the detection of
> questionable source code places is incomplete (also because of the current
> development status for the Coccinelle software).
> Thus I suggest to reconsider the usage of the word “every” for the patch message
> once more.

I'm not against changing the patch's subject. Would you find it better
if it was "coccinelle: Extend address test from ifaddr to test
expressions"?

And what about you Julia?

> Will further adjustments become helpful for desired source code analyses
> and corresponding transformations?

Do you mean adjustments in this semantic patch or in the coccinelle software?


Thanks,
Jérémy

Re: [cocci] coccinelle: Extend address test from ifaddr to every test expression

[Markus Elfring]

>> Thus I suggest to reconsider the usage of the word “every” for the patch message
>> once more.
> I'm not against changing the patch's subject.


I propose to reconsider also the patch description accordingly.



> Would you find it better if it was "coccinelle: Extend address test from ifaddr
> to test expressions"?


Another wording variant?

* address test ⇒ address check

* ifaddr ⇒ script “ifaddr”



>> Will further adjustments become helpful for desired source code analyses
>> and corresponding transformations?
> Do you mean adjustments in this semantic patch or in the coccinelle software?


Both.

Are we looking for further collateral evolution?

Regards,
Markus

Re: [cocci] [PATCH v3] coccinelle: Extend address test from ifaddr to every test expression

[Julia Lawall]

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



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

> > It can be determined that the proposed approach for the detection of
> > questionable source code places is incomplete (also because of the current
> > development status for the Coccinelle software).
> > Thus I suggest to reconsider the usage of the word “every” for the patch message
> > once more.
>
> I'm not against changing the patch's subject. Would you find it better
> if it was "coccinelle: Extend address test from ifaddr to test
> expressions"?
>
> And what about you Julia?

The subject you propose is fine.

julia

>
> > Will further adjustments become helpful for desired source code analyses
> > and corresponding transformations?
>
> Do you mean adjustments in this semantic patch or in the coccinelle software?
>
>
> Thanks,
> Jérémy

Re: [cocci] coccinelle: Extend address test from ifaddr to every test expression

[Jérémy LEFAURE]

>>> Thus I suggest to reconsider the usage of the word “every” for the patch message
>>> once more.
>> I'm not against changing the patch's subject.
>
>
> I propose to reconsider also the patch description accordingly.

For me the patch description is fine. Do you have a part of the
description in mind that is not ok?

>> Would you find it better if it was "coccinelle: Extend address test from ifaddr
>> to test expressions"?
>
>
> Another wording variant?
>
> * address test ⇒ address check
>
> * ifaddr ⇒ script “ifaddr”

I'll keep "address check" but I can improve the "ifaddr" part: instead
of "script" as you suggested I'll use "semantic patch".


>>> Will further adjustments become helpful for desired source code analyses
>>> and corresponding transformations?
>> Do you mean adjustments in this semantic patch or in the coccinelle software?
>
>
> Both.
>
> Are we looking for further collateral evolution?

I can't speak for the coccinelle part but for this semantic patch I'm
not planning other changes for the moment (except the improvements
suggested during the review of the patch).

Actually I don't know if the example you presented (bool t = (&s);) is
really a text expression of if it is a cast to bool. But even if it is a
separate case, it may be possible to detect that with a semantic patch.
But I'm not sure that it is in the scope of this patch nor the rule
modified by this patch.


Thank you,
Jérémy

Re: [cocci] coccinelle: Extend address test from ifaddr to every test expression

[Markus Elfring]

> For me the patch description is fine. Do you have a part of the
> description in mind that is not ok?


Yes.

I indicated by my review that explanation consequences should be taken
better into account also for the key word “every”, didn't I?



>> Are we looking for further collateral evolution?
> I can't speak for the coccinelle part but for this semantic patch I'm
> not planning other changes for the moment (except the improvements
> suggested during the review of the patch).


Would other contributors like to influence affected software components
any more?



> Actually I don't know if the example you presented (bool t = (&s);) is
> really a text expression of if it is a cast to bool. But even if it is a
> separate case, it may be possible to detect that with a semantic patch.
> But I'm not sure that it is in the scope of this patch nor the rule
> modified by this patch.


I am curious if understanding variations for such implementation details
will trigger corresponding software improvements.

Regards,
Markus