coccinelle: adjustments for array.cocci?

coccinelle: adjustments for array.cocci?

[Markus Elfring]

Hello,

I would like to comment implementation details from
the commit 177fbab747da4f58cb2a8ce010b3515c86dd67c9 ("coccinelle: use COPY_ARRAY for copying arrays").


Do you find the following code variant (for the semantic patch language) also useful?

 memcpy(
(       ptr, E, n *
-       sizeof(*(ptr))
+       sizeof(T)
|       arr, E, n *
-       sizeof(*(arr))
+       sizeof(T)
|       E, ptr, n *
-       sizeof(*(ptr))
+       sizeof(T)
|       E, arr, n *
-       sizeof(*(arr))
+       sizeof(T)
)
       )


How do you think about the following SmPL code variant?

-memcpy
+COPY_ARRAY
       (
(       dst_ptr
|       dst_arr
)
       ,
(       src_ptr
|       src_arr
)
       ,
-       (n) * sizeof(T)
+       n
       )


Regards,
Markus

coccinelle: adjustments for array.cocci?

[Markus Elfring]

Hello,

I would like to comment implementation details from
the commit 177fbab747da4f58cb2a8ce010b3515c86dd67c9 ("coccinelle: use COPY_ARRAY for copying arrays").


Do you find the following code variant (for the semantic patch language) also useful?

 memcpy(
(       ptr, E, n *
-       sizeof(*(ptr))
+       sizeof(T)
|       arr, E, n *
-       sizeof(*(arr))
+       sizeof(T)
|       E, ptr, n *
-       sizeof(*(ptr))
+       sizeof(T)
|       E, arr, n *
-       sizeof(*(arr))
+       sizeof(T)
)
       )


How do you think about the following SmPL code variant?

-memcpy
+COPY_ARRAY
       (
(       dst_ptr
|       dst_arr
)
       ,
(       src_ptr
|       src_arr
)
       ,
-       (n) * sizeof(T)
+       n
       )


Regards,
Markus

Re: coccinelle: adjustments for array.cocci?

[René Scharfe]

Am 12.11.19 um 16:08 schrieb Markus Elfring:
> Hello,
>
> I would like to comment implementation details from
> the commit 177fbab747da4f58cb2a8ce010b3515c86dd67c9 ("coccinelle: use COPY_ARRAY for copying arrays").
>
>
> Do you find the following code variant (for the semantic patch language) also useful?
>
>  memcpy(
> (       ptr, E, n *
> -       sizeof(*(ptr))
> +       sizeof(T)
> |       arr, E, n *
> -       sizeof(*(arr))
> +       sizeof(T)
> |       E, ptr, n *
> -       sizeof(*(ptr))
> +       sizeof(T)
> |       E, arr, n *
> -       sizeof(*(arr))
> +       sizeof(T)
> )
>        )

This reduces duplication in the semantic patch, which is nice.  I think
I tried something like that at the time, but found that it failed to
produce some of the cases in 921d49be86 ("use COPY_ARRAY for copying
arrays", 2019-06-15) for some reason.

> How do you think about the following SmPL code variant?
>
> -memcpy
> +COPY_ARRAY
>        (
> (       dst_ptr
> |       dst_arr
> )
>        ,
> (       src_ptr
> |       src_arr
> )
>        ,
> -       (n) * sizeof(T)
> +       n
>        )

This eliminates duplication in the semantic patch, which is good.  It
messes up the indentation of n in some of the cases in 921d49be86 ("use
COPY_ARRAY for copying arrays", 2019-06-15), though.  Hmm, but that can
be cured by duplicating the comma:

   - , (n) * sizeof(T)
   + , n

René

Re: coccinelle: adjustments for array.cocci?

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> This reduces duplication in the semantic patch, which is nice.  I think
> I tried something like that at the time, but found that it failed to
> produce some of the cases in 921d49be86 ("use COPY_ARRAY for copying
> arrays", 2019-06-15) for some reason.

Thanks for mentioning.

I too recall that seemingly redundant entries were noticed during
the review and at least back then removing the seemingly redundant
ones caused failures in rewriting.

That is why I am hesitant to touch any patch that says "simplify
cocci rule" making it sound as if simplification is a good thing on
its own.  I have no problem with "we change the rule this way, which
eliminates this false positive / negative, that is demonstrated in
the added tests in t/ directory", though.

Thanks.

Re: coccinelle: adjustments for array.cocci?

[Markus Elfring]

> I too recall that seemingly redundant entries were noticed during
> the review and at least back then removing the seemingly redundant
> ones caused failures in rewriting.

I am curious if the redundancy can be reconsidered once more.

Do you refer to open issues around source code reformatting
and pretty-printing together with the Coccinelle software here?

Would you like to achieve any further improvements also in this area?

Regards,
Markus

Re: coccinelle: adjustments for array.cocci?

[Junio C Hamano]

Markus Elfring <Markus.Elfring@web.de> writes:

>> I too recall that seemingly redundant entries were noticed during
>> the review and at least back then removing the seemingly redundant
>> ones caused failures in rewriting.
>
> I am curious if the redundancy can be reconsidered once more.
>
> Do you refer to open issues around source code reformatting
> and pretty-printing together with the Coccinelle software here?

Sorry, I do not follow.  

If you are asking if I am interested in following bleeding edge
Coccinelle development and use this project as a guinea pig to do
so, then the answer is no.  I'd rather see us instead staying on the
trailing edge ;-) to make sure that we use common denominator
features that are known to be available in all widely deployed and
perhaps a bit dated versions that come with popular distros.

And if that means we have to accept inefficient ways to express our
patterns, we are willing to pay for that cost.

So, "the A.cocci file uses a set of inefficient expressions that can
be written more concisely like this, using the bleeding edge version
of the syntax" is not a useful improvement for the purpose of this
project, while "the A.cocci file uses a set of inefficient
expressions that can be written more concisely like this, and all
versions of cocci that is newer than X would understand the
notation.  Even distro D that tends to ship with fairly stale
versions of packages ship version X+n, so this change should be
safe" is very much appreciated.

Thanks.

Re: coccinelle: adjustments for array.cocci?

[Markus Elfring]

>>> I too recall that seemingly redundant entries were noticed during
>>> the review and at least back then removing the seemingly redundant
>>> ones caused failures in rewriting.
>>
>> I am curious if the redundancy can be reconsidered once more.
>>
>> Do you refer to open issues around source code reformatting
>> and pretty-printing together with the Coccinelle software here?
>
> Sorry, I do not follow.
>
> If you are asking if I am interested in following bleeding edge
> Coccinelle development and use this project as a guinea pig to do so,

I did not ask this.

You mentioned “failures”. - I became curious then if corresponding software
development challenges can be clarified a bit more.


> then the answer is no.

Such feedback is reasonable.


> I'd rather see us instead staying on the trailing edge ;-)
> to make sure that we use common denominator features that are known
> to be available in all widely deployed and perhaps a bit dated versions
> that come with popular distros.

I find that I am proposing script adjustments within the basic feature set
for the semantic patch language here.
Further fine-tuning will become possible, won't it?

Regards,
Markus

Re: coccinelle: adjustments for array.cocci?

[René Scharfe]

Am 14.11.19 um 14:15 schrieb Markus Elfring:
> You mentioned “failures”. - I became curious then if corresponding software
> development challenges can be clarified a bit more.

Let's try to restore/repeat the pertinent paragraph, with context and
attribution:

Am 13.11.19 um 03:11 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>> Am 12.11.19 um 16:08 schrieb Markus Elfring:
>>>
>>> Do you find the following code variant (for the semantic patch language) also useful?
>>>
>>>  memcpy(
>>> (       ptr, E, n *
>>> -       sizeof(*(ptr))
>>> +       sizeof(T)
>>> |       arr, E, n *
>>> -       sizeof(*(arr))
>>> +       sizeof(T)
>>> |       E, ptr, n *
>>> -       sizeof(*(ptr))
>>> +       sizeof(T)
>>> |       E, arr, n *
>>> -       sizeof(*(arr))
>>> +       sizeof(T)
>>> )
>>>        )
>
>> This reduces duplication in the semantic patch, which is nice.  I think
>> I tried something like that at the time, but found that it failed to
>> produce some of the cases in 921d49be86 ("use COPY_ARRAY for copying
>> arrays", 2019-06-15) for some reason.
> Thanks for mentioning.
>
> I too recall that seemingly redundant entries were noticed during
> the review and at least back then removing the seemingly redundant
> ones caused failures in rewriting.

You can see for yourself by:

 1. applying the patch at the bottom to implement your suggested change,
 2. running "git show 921d49be86 | patch -p1 -R" to undo 921d49be86,
 3. running "make contrib/coccinelle/array.cocci.patch",
 4. running "patch -p1 <contrib/coccinelle/array.cocci.patch",
 5. running "git diff".

If the new version of array.cocci is equivalent to the current one then
that last step should show no difference.  For me, "git diff --stat"
reports, however:

 contrib/coccinelle/array.cocci | 30 ++++++++++++++----------------
 fast-import.c                  |  2 +-
 packfile.c                     |  4 ++--
 pretty.c                       |  4 ++--
 4 files changed, 19 insertions(+), 21 deletions(-)

The changes in array.cocci are expected of course, but the others
indicate that the new version missed transformations that the current
version generated.

René


-- >8 --
diff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci
index 46b8d2ee11..e7bcbefcc1 100644
--- a/contrib/coccinelle/array.cocci
+++ b/contrib/coccinelle/array.cocci
@@ -12,27 +12,25 @@ T *ptr;
 T[] arr;
 expression E, n;
 @@
+  memcpy(
 (
-  memcpy(ptr, E,
-- n * sizeof(*(ptr))
-+ n * sizeof(T)
-  )
+  ptr, E, n *
+- sizeof(*(ptr))
++ sizeof(T)
 |
-  memcpy(arr, E,
-- n * sizeof(*(arr))
-+ n * sizeof(T)
-  )
+  arr, E, n *
+- sizeof(*(arr))
++ sizeof(T)
 |
-  memcpy(E, ptr,
-- n * sizeof(*(ptr))
-+ n * sizeof(T)
-  )
+  E, ptr, n *
+- sizeof(*(ptr))
++ sizeof(T)
 |
-  memcpy(E, arr,
-- n * sizeof(*(arr))
-+ n * sizeof(T)
-  )
+  E, arr, n *
+- sizeof(*(arr))
++ sizeof(T)
 )
+  )

 @@
 type T;

Re: coccinelle: adjustments for array.cocci?

[Markus Elfring]

> If the new version of array.cocci is equivalent to the current one then
> that last step should show no difference.

I hoped it.


>  contrib/coccinelle/array.cocci | 30 ++++++++++++++----------------
>  fast-import.c                  |  2 +-
>  packfile.c                     |  4 ++--
>  pretty.c                       |  4 ++--
>  4 files changed, 19 insertions(+), 21 deletions(-)
>
> The changes in array.cocci are expected of course, but the others
> indicate that the new version missed transformations that the current
> version generated.

Would we like to submit a bug report for the Coccinelle software?

Which version did you try out for the comparison of generated patches?

Regards,
Markus

Re: coccinelle: adjustments for array.cocci?

[René Scharfe]

Am 14.11.19 um 18:14 schrieb Markus Elfring:
>> If the new version of array.cocci is equivalent to the current one then
>> that last step should show no difference.
>
> I hoped it.
>
>
>>  contrib/coccinelle/array.cocci | 30 ++++++++++++++----------------
>>  fast-import.c                  |  2 +-
>>  packfile.c                     |  4 ++--
>>  pretty.c                       |  4 ++--
>>  4 files changed, 19 insertions(+), 21 deletions(-)
>>
>> The changes in array.cocci are expected of course, but the others
>> indicate that the new version missed transformations that the current
>> version generated.
>
> Would we like to submit a bug report for the Coccinelle software?

Not really, because...

> Which version did you try out for the comparison of generated patches?

... I use the last version of the Debian testing package, 1.0.4.deb-4.
https://tracker.debian.org/pkg/coccinelle says it was removed from
testing recently.  I was actually waiting for a more recent version
like 1.0.8 to be packaged; not sure what's going on there.

Anyway, someone who can reproduce the issue using the latest release
of Coccinelle would be in a better position to file a bug report.

René

Re: coccinelle: adjustments for array.cocci?

[Markus Elfring]

> This eliminates duplication in the semantic patch, which is good.

Thanks that you think in such a direction.


> It messes up the indentation of n in some of the cases in 921d49be86 ("use
> COPY_ARRAY for copying arrays", 2019-06-15), though.  Hmm, but that can
> be cured by duplicating the comma:

I have picked up further improvement possibilities for this SmPL script.
Would you like to integrate any of these changes?


@@
expression dst, src, n, E;
@@
 memcpy(dst, src, sizeof(
+                        *(
                           E
-                           [...]
+                         )
                         ) * n
       )

@@
type T;
T *ptr;
T[] arr;
expression E, n;
@@
 memcpy(
(       ptr, E, sizeof(
-                      *(ptr)
+                      T
                      ) * n
|       arr, E, sizeof(
-                      *(arr)
+                      T
                      ) * n
|       E, ptr, sizeof(
-                      *(ptr)
+                      T
                      ) * n
|       E, arr, sizeof(
-                      *(arr)
+                      T
                      ) * n
)
       )

@@
type T;
T* dst_ptr, src_ptr;
T[] dst_arr, src_arr;
expression n, x;
@@
-memcpy
+COPY_ARRAY
       (
(       dst_ptr
|       dst_arr
)
       ,
(       src_ptr
|       src_arr
)
-      , (n) * \( sizeof(T) \| sizeof(*(x)) \)
+      , n
       )

@@
type T;
T* dst, src, ptr;
expression n;
@@
(
-memmove
+MOVE_ARRAY
        (dst, src
-                , (n) * \( sizeof(* \( dst \| src \) ) \| sizeof(T) \)
+                , n
        )
|
-ptr = xmalloc((n) * \( sizeof(*ptr) \| sizeof(T) \))
+ALLOC_ARRAY(ptr, n)
);


Now I observe that the placement of space characters can be a coding style
concern at four places for adjusted lines by the generated patch.
Would you like to clarify remaining issues for pretty-printing
in such use cases?

Regards,
Markus

Re: coccinelle: adjustments for array.cocci?

[Markus Elfring]

> This reduces duplication in the semantic patch, which is nice.  I think
> I tried something like that at the time, but found that it failed to
> produce some of the cases in 921d49be86 ("use COPY_ARRAY for copying
> arrays", 2019-06-15) for some reason.

I propose to integrate an other solution variant.

* How do you think about to delete questionable transformation rules
  together with increasing the usage of nested disjunctions in this script
  for the semantic patch language?

* Can a single transformation rule become sufficient for the discussed
  change pattern?


@@
type T;
T* dst_ptr, src_ptr, ptr;
T[] dst_arr, src_arr;
expression n, x;
@@
(
-memcpy
+COPY_ARRAY
       (
(       dst_ptr
|       dst_arr
)
       ,
(       src_ptr
|       src_arr
)
-      , (n) * \( sizeof(T) \| sizeof( \( *(x) \| x[...] \) ) \)
+      , n
       )
|
-memmove
+MOVE_ARRAY
        (dst_ptr,
         src_ptr
-               , (n) * \( sizeof(* \( dst_ptr \| src_ptr \) ) \| sizeof(T) \)
+               , n
        )
|
-ptr = xmalloc((n) * \( sizeof(*ptr) \| sizeof(T) \))
+ALLOC_ARRAY(ptr, n)
)


Would you like to clarify remaining challenges for pretty-printing
in such use cases?

Regards,
Markus

Re: coccinelle: adjustments for array.cocci?

[René Scharfe]

Am 15.11.19 um 21:37 schrieb Markus Elfring:
>> This eliminates duplication in the semantic patch, which is good.
>
> Thanks that you think in such a direction.
>
>
>> It messes up the indentation of n in some of the cases in 921d49be86 ("use
>> COPY_ARRAY for copying arrays", 2019-06-15), though.  Hmm, but that can
>> be cured by duplicating the comma:
>
> I have picked up further improvement possibilities for this SmPL script.
> Would you like to integrate any of these changes?

Not sure, could you please elaborate on the benefits of each proposed
change?

> @@
> expression dst, src, n, E;
> @@
>  memcpy(dst, src, sizeof(
> +                        *(
>                            E
> -                           [...]
> +                         )
>                          ) * n
>        )

That's longer and looks more complicated to me than what we currently have:

  @@
  expression dst, src, n, E;
  @@
    memcpy(dst, src, n * sizeof(
  - E[...]
  + *(E)
    ))

Avoiding to duplicate E doesn't seem to be worth it.  I can see that
indenting the sizeof parameter and parentheses could improve readability,
though.

> @@
> type T;
> T *ptr;
> T[] arr;
> expression E, n;
> @@
>  memcpy(
> (       ptr, E, sizeof(
> -                      *(ptr)
> +                      T
>                       ) * n
> |       arr, E, sizeof(
> -                      *(arr)
> +                      T
>                       ) * n
> |       E, ptr, sizeof(
> -                      *(ptr)
> +                      T
>                       ) * n
> |       E, arr, sizeof(
> -                      *(arr)
> +                      T
>                       ) * n
> )
>        )

This still fails to regenerate two of the changes from 921d49be86 (use
COPY_ARRAY for copying arrays, 2019-06-15), at least with for me (and
Coccinelle 1.0.4).

> @@
> type T;
> T* dst_ptr, src_ptr;
> T[] dst_arr, src_arr;
> expression n, x;
> @@
> -memcpy
> +COPY_ARRAY
>        (
> (       dst_ptr
> |       dst_arr
> )
>        ,
> (       src_ptr
> |       src_arr
> )
> -      , (n) * \( sizeof(T) \| sizeof(*(x)) \)
> +      , n
>        )

That x could be anything -- it's not tied to the element size of source
or destination.  Such a transformation might change the meaning of the
code, as COPY_ARRAY will use the element size of the destination behind
the scenes.  So that doesn't look safe to me.

> @@
> type T;
> T* dst, src, ptr;
> expression n;
> @@
> (
> -memmove
> +MOVE_ARRAY
>         (dst, src
> -                , (n) * \( sizeof(* \( dst \| src \) ) \| sizeof(T) \)
> +                , n
>         )
> |
> -ptr = xmalloc((n) * \( sizeof(*ptr) \| sizeof(T) \))
> +ALLOC_ARRAY(ptr, n)
> );

memmove/MOVE_ARRAY and xmalloc/ALLOC_ARRAY are quite different; why
would we want to jam transformations for them into the same rule like
this?  The only overlap seems to be n.  Handling memmove/MOVE_ARRAY and
memcpy/COPY_ARRAY together would make more sense, as they take the same
kinds of parameters.

I didn't know that disjunctions can be specified inline using \(, \|
and \), though.  Rules can be much more compact that way.  Mixing
languages like that can also be quite confusing.  Syntax highlighting
could help; https://github.com/ahf/cocci-syntax at least doesn't
show those any different from regular code, though.

> Now I observe that the placement of space characters can be a coding style
> concern at four places for adjusted lines by the generated patch.
> Would you like to clarify remaining issues for pretty-printing
> in such use cases?

Ideally, generated code should adhere to Documentation/CodingGuidelines,
so that it can be accepted without requiring hand-editing.

René

Re: coccinelle: adjustments for array.cocci?

[René Scharfe]

Am 16.11.19 um 17:33 schrieb Markus Elfring:
>> This reduces duplication in the semantic patch, which is nice.  I think
>> I tried something like that at the time, but found that it failed to
>> produce some of the cases in 921d49be86 ("use COPY_ARRAY for copying
>> arrays", 2019-06-15) for some reason.
>
> I propose to integrate an other solution variant.
>
> * How do you think about to delete questionable transformation rules
>   together with increasing the usage of nested disjunctions in this script
>   for the semantic patch language?

Which transformation rules are questionable and why?  Removing broken
or ineffective rules would be very welcome.

Specifying disjunctions inline can make rules shorter, but harder to
understand due to mixing languages.  Perhaps this is a matter of
getting used to it, and syntax highlighting might help a bit.

> * Can a single transformation rule become sufficient for the discussed
>   change pattern?
>
>
> @@
> type T;
> T* dst_ptr, src_ptr, ptr;
> T[] dst_arr, src_arr;
> expression n, x;
> @@
> (
> -memcpy
> +COPY_ARRAY
>        (
> (       dst_ptr
> |       dst_arr
> )
>        ,
> (       src_ptr
> |       src_arr
> )
> -      , (n) * \( sizeof(T) \| sizeof( \( *(x) \| x[...] \) ) \)
> +      , n
>        )
> |
> -memmove
> +MOVE_ARRAY
>         (dst_ptr,
>          src_ptr
> -               , (n) * \( sizeof(* \( dst_ptr \| src_ptr \) ) \| sizeof(T) \)
> +               , n
>         )
> |
> -ptr = xmalloc((n) * \( sizeof(*ptr) \| sizeof(T) \))
> +ALLOC_ARRAY(ptr, n)
> )

memmove/MOVE_ARRAY take the same kind of parameters as
memcpy/COPY_ARRAY, so handling them in the same rule makes sense.
The former could take advantage of the transformations for arrays
that the latter has.

Mixing in the unrelated xmalloc/ALLOC_ARRAY transformation does
not make sense to me, though.

Matching sizeof of anything (with the x) can produce inaccurate
transformations, as mentioned in the other reply I just sent.

> Would you like to clarify remaining challenges for pretty-printing
> in such use cases?

Not sure what you mean here.  Did my other reply answer it?  If it
didn't then please state what's unclear to you.

René

Re: coccinelle: adjustments for array.cocci?

[Markus Elfring]

>> @@
>> expression dst, src, n, E;
>> @@
>>  memcpy(dst, src, sizeof(
>> +                        *(
>>                            E
>> -                           [...]
>> +                         )
>>                          ) * n
>>        )
>
> That's longer and looks more complicated to me

I point another possibility out to express a change specification
by the means of the semantic patch language.
How would you think about such SmPL code if the indentation
will be reduced?


> than what we currently have:
>   @@
>   expression dst, src, n, E;
>   @@
>     memcpy(dst, src, n * sizeof(
>   - E[...]
>   + *(E)
>     ))
>
> Avoiding to duplicate E doesn't seem to be worth it.

I show other development preferences occasionally.


> I can see that indenting the sizeof parameter and parentheses could
> improve readability, though.

Thanks that you can follow such coding style aspects.


>> @@
>> type T;
>> T *ptr;
>> T[] arr;
>> expression E, n;
>> @@
>>  memcpy(
>> (       ptr, E, sizeof(
>> -                      *(ptr)
>> +                      T
>>                       ) * n
>> |       arr, E, sizeof(
>> -                      *(arr)
>> +                      T
>>                       ) * n
>> |       E, ptr, sizeof(
>> -                      *(ptr)
>> +                      T
>>                       ) * n
>> |       E, arr, sizeof(
>> -                      *(arr)
>> +                      T
>>                       ) * n
>> )
>>        )
>
> This still fails to regenerate two of the changes from 921d49be86
> (use COPY_ARRAY for copying arrays, 2019-06-15), at least with for me
> (and Coccinelle 1.0.4).

Would you become keen to find the reasons out for unexpected data processing
results (also by the software combination “Coccinelle 1.0.8-00004-g842075f7”)
at this place?

But this transformation rule can probably be omitted if the usage
of SmPL disjunctions will be increased in a subsequent rule, can't it?


>> @@
>> type T;
>> T* dst_ptr, src_ptr;
>> T[] dst_arr, src_arr;
>> expression n, x;
>> @@
>> -memcpy
>> +COPY_ARRAY
>>        (
>> (       dst_ptr
>> |       dst_arr
>> )
>>        ,
>> (       src_ptr
>> |       src_arr
>> )
>> -      , (n) * \( sizeof(T) \| sizeof(*(x)) \)
>> +      , n
>>        )
>
> That x could be anything -- it's not tied to the element size of source
> or destination.  Such a transformation might change the meaning of the
> code, as COPY_ARRAY will use the element size of the destination behind
> the scenes.  So that doesn't look safe to me.

Would you like to use the SmPL code “*( \( src_ptr \| src_arr \) )” instead?


>> @@
>> type T;
>> T* dst, src, ptr;
>> expression n;
>> @@
>> (
>> -memmove
>> +MOVE_ARRAY
>>         (dst, src
>> -                , (n) * \( sizeof(* \( dst \| src \) ) \| sizeof(T) \)
>> +                , n
>>         )
>> |
>> -ptr = xmalloc((n) * \( sizeof(*ptr) \| sizeof(T) \))
>> +ALLOC_ARRAY(ptr, n)
>> );
>
> memmove/MOVE_ARRAY and xmalloc/ALLOC_ARRAY are quite different;

These functions provide another programming interface.


> why would we want to jam transformations for them into the same rule
> like this?

Possible nicer run time characteristics by the Coccinelle software.


> The only overlap seems to be n.

These case distinctions can share also the metavariable “T” for the
desired source code deletion.


> Handling memmove/MOVE_ARRAY and memcpy/COPY_ARRAY together would make
> more sense, as they take the same kinds of parameters.

Would you like to adjust the SmPL code in such a design direction?


> I didn't know that disjunctions can be specified inline using \(, \|
> and \), though.  Rules can be much more compact that way.

I hope that more corresponding software improvements can be achieved.


> Mixing languages like that can also be quite confusing.

I agree to this development concern.


>> Now I observe that the placement of space characters can be a coding style
>> concern at four places for adjusted lines by the generated patch.
>> Would you like to clarify remaining issues for pretty-printing
>> in such use cases?
>
> Ideally, generated code should adhere to Documentation/CodingGuidelines,
> so that it can be accepted without requiring hand-editing.

But how does the software situation look like if the original source code
would contain coding style issues?

It seems to be possible to specify SmPL code in a way so that even questionable
code layout would be preserved by an automatic transformation.

Regards,
Markus

Re: coccinelle: adjustments for array.cocci?

[Markus Elfring]

> Which transformation rules are questionable and why?

It was chosen to transform source code fragments (pointer expressions)
by two SmPL rules so that the search pattern “sizeof(T)” would work
in the third rule.


> Removing broken or ineffective rules would be very welcome.

I suggest to reconsider programming opportunities also by the means of
the semantic patch language.


> Specifying disjunctions inline can make rules shorter, but harder to
> understand due to mixing languages.  Perhaps this is a matter of
> getting used to it, and syntax highlighting might help a bit.

I agree to this view.


> Mixing in the unrelated xmalloc/ALLOC_ARRAY transformation does
> not make sense to me, though.

I propose to increase the sharing (or reuse) of involved metavariables.


> Matching sizeof of anything (with the x) can produce inaccurate
> transformations, as mentioned in the other reply I just sent.

Would you like to apply any further SmPL code fine-tuning?

Regards,
Markus

Re: coccinelle: adjustments for array.cocci?

[René Scharfe]

Am 17.11.19 um 08:56 schrieb Markus Elfring:
>>> @@
>>> expression dst, src, n, E;
>>> @@
>>>  memcpy(dst, src, sizeof(
>>> +                        *(
>>>                            E
>>> -                           [...]
>>> +                         )
>>>                          ) * n
>>>        )
>>
>> That's longer and looks more complicated to me
>
> I point another possibility out to express a change specification
> by the means of the semantic patch language.
> How would you think about such SmPL code if the indentation
> will be reduced?

Whitespace is not what makes the above example more complicated than the
equivalent rule below; separating the pieces of simple expressions does.

>> than what we currently have:
>>   @@
>>   expression dst, src, n, E;
>>   @@
>>     memcpy(dst, src, n * sizeof(
>>   - E[...]
>>   + *(E)
>>     ))

>>> @@
>>> type T;
>>> T *ptr;
>>> T[] arr;
>>> expression E, n;
>>> @@
>>>  memcpy(
>>> (       ptr, E, sizeof(
>>> -                      *(ptr)
>>> +                      T
>>>                       ) * n
>>> |       arr, E, sizeof(
>>> -                      *(arr)
>>> +                      T
>>>                       ) * n
>>> |       E, ptr, sizeof(
>>> -                      *(ptr)
>>> +                      T
>>>                       ) * n
>>> |       E, arr, sizeof(
>>> -                      *(arr)
>>> +                      T
>>>                       ) * n
>>> )
>>>        )
>>
>> This still fails to regenerate two of the changes from 921d49be86
>> (use COPY_ARRAY for copying arrays, 2019-06-15), at least with for me
>> (and Coccinelle 1.0.4).
>
> Would you become keen to find the reasons out for unexpected data processing
> results (also by the software combination “Coccinelle 1.0.8-00004-g842075f7”)
> at this place?

It looks like a bug in Coccinelle to me and I'd like to see it fixed if
that's confirmed, of course.  And I'd like to see Debian pick up a newer
version, preferably containing that fix.  But at least until then our
semantic patches need to work around it.

> But this transformation rule can probably be omitted if the usage
> of SmPL disjunctions will be increased in a subsequent rule, can't it?

Perhaps, but I don't see how.  Do you?

>>> @@
>>> type T;
>>> T* dst_ptr, src_ptr;
>>> T[] dst_arr, src_arr;
>>> expression n, x;
>>> @@
>>> -memcpy
>>> +COPY_ARRAY
>>>        (
>>> (       dst_ptr
>>> |       dst_arr
>>> )
>>>        ,
>>> (       src_ptr
>>> |       src_arr
>>> )
>>> -      , (n) * \( sizeof(T) \| sizeof(*(x)) \)
>>> +      , n
>>>        )
>>
>> That x could be anything -- it's not tied to the element size of source
>> or destination.  Such a transformation might change the meaning of the
>> code, as COPY_ARRAY will use the element size of the destination behind
>> the scenes.  So that doesn't look safe to me.
>
> Would you like to use the SmPL code “*( \( src_ptr \| src_arr \) )” instead?

That leaves out dst_ptr and dst_arr.

And what would it mean to match e.g. this ?

	memcpy(dst_ptr, src_ptr, n * sizeof(*src_arr))

At least the element size would be the same, but I'd rather shy away from
transforming weird cases like this automatically.

>>> @@
>>> type T;
>>> T* dst, src, ptr;
>>> expression n;
>>> @@
>>> (
>>> -memmove
>>> +MOVE_ARRAY
>>>         (dst, src
>>> -                , (n) * \( sizeof(* \( dst \| src \) ) \| sizeof(T) \)
>>> +                , n
>>>         )
>>> |
>>> -ptr = xmalloc((n) * \( sizeof(*ptr) \| sizeof(T) \))
>>> +ALLOC_ARRAY(ptr, n)
>>> );
>>
>> memmove/MOVE_ARRAY and xmalloc/ALLOC_ARRAY are quite different;
>
> These functions provide another programming interface.

Huh, which one specifically?  Here are the signatures of the functions
and macros, for reference:

  void *memmove(void *dest, const void *src, size_t n);
  void *memcpy(void *dest, const void *src, size_t n);

  COPY_ARRAY(dst, src, n)
  MOVE_ARRAY(dst, src, n)

>> why would we want to jam transformations for them into the same rule
>> like this?
>
> Possible nicer run time characteristics by the Coccinelle software.

How much faster is it exactly?

Speedups are good, but I think readability of rules is more important
than coccicheck duration.

>> Handling memmove/MOVE_ARRAY and memcpy/COPY_ARRAY together would make
>> more sense, as they take the same kinds of parameters.
>
> Would you like to adjust the SmPL code in such a design direction?

I can't find any examples in our code base that would be transformed by
a generalized rule.  That reduces my own motivation to tinker with the
existing rules to close to zero.

>>> Now I observe that the placement of space characters can be a coding style
>>> concern at four places for adjusted lines by the generated patch.
>>> Would you like to clarify remaining issues for pretty-printing
>>> in such use cases?
>>
>> Ideally, generated code should adhere to Documentation/CodingGuidelines,
>> so that it can be accepted without requiring hand-editing.
>
> But how does the software situation look like if the original source code
> would contain coding style issues?

The same: Generated code should not add coding style issues.  We can
still use results that need to be polished, but that's a manual step
which reduces the benefits of automation.

> It seems to be possible to specify SmPL code in a way so that even questionable
> code layout would be preserved by an automatic transformation.

That may be acceptable.

René

Re: coccinelle: adjustments for array.cocci?

[René Scharfe]

Am 17.11.19 um 09:19 schrieb Markus Elfring:
>> Which transformation rules are questionable and why?
>
> It was chosen to transform source code fragments (pointer expressions)
> by two SmPL rules so that the search pattern “sizeof(T)” would work
> in the third rule.

Ah, right, it would be nice to get rid of those normalization rules,
especially the second one.  I don't see how, though, without either
causing a combinatorial explosion or loosening up the matching too much.

>> Matching sizeof of anything (with the x) can produce inaccurate
>> transformations, as mentioned in the other reply I just sent.
>
> Would you like to apply any further SmPL code fine-tuning?

I guess that's a question for Junio, and his reply in
https://public-inbox.org/git/xmqqa790cyp1.fsf@gitster-ct.c.googlers.com/
seems relevant.

René

Re: coccinelle: adjustments for array.cocci?

[Markus Elfring]

> Whitespace is not what makes the above example more complicated than the
> equivalent rule below;

A different code layout might help in a better understanding for such
change specifications.


> separating the pieces of simple expressions does.

Will there occasionally be a need to change only the required source code parts?


>>> than what we currently have:
>>>   @@
>>>   expression dst, src, n, E;
>>>   @@
>>>     memcpy(dst, src, n * sizeof(
>>>   - E[...]
>>>   + *(E)
>>>     ))

Are any circumstances to consider where only the essential implementation details
should be touched by an automatic software transformation?


>>>> @@
>>>> type T;
>>>> T *ptr;
>>>> T[] arr;
>>>> expression E, n;
>>>> @@
>>>>  memcpy(
>>>> (       ptr, E, sizeof(
>>>> -                      *(ptr)
>>>> +                      T
>>>>                       ) * n
>>>> |       arr, E, sizeof(
>>>> -                      *(arr)
>>>> +                      T
>>>>                       ) * n
>>>> |       E, ptr, sizeof(
>>>> -                      *(ptr)
>>>> +                      T
>>>>                       ) * n
>>>> |       E, arr, sizeof(
>>>> -                      *(arr)
>>>> +                      T
>>>>                       ) * n
>>>> )
>>>>        )
>>>
>>> This still fails to regenerate two of the changes from 921d49be86
>>> (use COPY_ARRAY for copying arrays, 2019-06-15), at least with for me
>>> (and Coccinelle 1.0.4).
>>
>> Would you become keen to find the reasons out for unexpected data processing
>> results (also by the software combination “Coccinelle 1.0.8-00004-g842075f7”)
>> at this place?
>
> It looks like a bug in Coccinelle to me

We might stumble also on just another (temporary) software limitation.


> and I'd like to see it fixed

Would you like to support corresponding development anyhow?


> if that's confirmed, of course.

I am curious if further feedback will evolve for affected software areas.


> And I'd like to see Debian pick up a newer version, preferably containing that fix.

I assume that you can wait a long time for progress in the software
distribution direction.


> But at least until then our semantic patches need to work around it.

Would another concrete fix for the currently discussed SmPL script
be better than a “workaround”?


>> But this transformation rule can probably be omitted if the usage
>> of SmPL disjunctions will be increased in a subsequent rule, can't it?
>
> Perhaps, but I don't see how.  Do you?

Obviously, yes (in principle according to my proposal from yesterday).
https://public-inbox.org/git/05ab1110-2115-7886-f890-9983caabc52c@web.de/


>> Would you like to use the SmPL code “*( \( src_ptr \| src_arr \) )” instead?
>
> That leaves out dst_ptr and dst_arr.

How many items should finally be filtered in the discussed SmPL disjunction?


> And what would it mean to match e.g. this ?
>
> 	memcpy(dst_ptr, src_ptr, n * sizeof(*src_arr))

The Coccinelle software takes care for commutativity by isomorphisms.
https://github.com/coccinelle/coccinelle/blob/19ee1697bf152d37a78a20cefe148775bf4b0e0d/standard.iso#L241


> At least the element size would be the same, but I'd rather shy away from
> transforming weird cases like this automatically.

Do you mean to specify additional restrictions by SmPL code?


>   void *memmove(void *dest, const void *src, size_t n);
>   void *memcpy(void *dest, const void *src, size_t n);
>
>   COPY_ARRAY(dst, src, n)
>   MOVE_ARRAY(dst, src, n)

Can the replacement of these functions by macro calls be combined further
by improved SmPL code?


>> Possible nicer run time characteristics by the Coccinelle software.
>
> How much faster is it exactly?

The answer will depend on efforts which you would like to invest
in corresponding (representative) measurements.


> Speedups are good, but I think readability of rules is more important
> than coccicheck duration.

I hope that a more pleasing balance can be found for the involved
usability factors.


>> But how does the software situation look like if the original source code
>> would contain coding style issues?
>
> The same: Generated code should not add coding style issues.

Such an expectation is generally nice. - But target conflicts can occur there.


> We can still use results that need to be polished, but that's a manual step
> which reduces the benefits of automation.

I am curious how the software development practice will evolve further.

Regards,
Markus

Re: coccinelle: adjustments for array.cocci?

[Markus Elfring]

>> It was chosen to transform source code fragments (pointer expressions)
>> by two SmPL rules so that the search pattern “sizeof(T)” would work
>> in the third rule.
>
> Ah, right, it would be nice to get rid of those normalization rules,
> especially the second one.

Thanks for such positive feedback.


> I don't see how,

Where is your view too limited at the moment?


> though, without either causing a combinatorial explosion

Growing combinations can become more interesting, can't they?


> or loosening up the matching too much.

I hope that we can achieve another reasonably safe transformation
approach together.

Regards,
Markus

Re: coccinelle: adjustments for array.cocci?

[René Scharfe]

Am 17.11.19 um 19:19 schrieb Markus Elfring:
>> Whitespace is not what makes the above example more complicated than the
>> equivalent rule below;
>
> A different code layout might help in a better understanding for such
> change specifications.
>
>
>> separating the pieces of simple expressions does.
>
> Will there occasionally be a need to change only the required source code parts?

Changing parts that don't need to be changed does not make sense to me.
Why do you ask and how does it relate to the example at hand?

>>>> than what we currently have:
>>>>   @@
>>>>   expression dst, src, n, E;
>>>>   @@
>>>>     memcpy(dst, src, n * sizeof(
>>>>   - E[...]
>>>>   + *(E)
>>>>     ))
>
> Are any circumstances to consider where only the essential implementation details
> should be touched by an automatic software transformation?

I don't understand this question.

>> It looks like a bug in Coccinelle to me
>
> We might stumble also on just another (temporary) software limitation.
>
>
>> and I'd like to see it fixed
>
> Would you like to support corresponding development anyhow?

I don't see me learning OCaml in the near future.  Or are you looking
for donations? :)

>> But at least until then our semantic patches need to work around it.
>
> Would another concrete fix for the currently discussed SmPL script
> be better than a “workaround”?

These are different things.  Fixes (repairs) are always welcome.  But
they should not rely on SmPL constructs that only work properly using
unreleased versions of Coccinelle.

>>> Would you like to use the SmPL code “*( \( src_ptr \| src_arr \) )” instead?
>>
>> That leaves out dst_ptr and dst_arr.
>
> How many items should finally be filtered in the discussed SmPL disjunction?

Let's see: dst and src can be pointers or array references, which makes
four combinations.  sizeof could either operate the shared type or on an
element of dst or an element of src.  An element can be accessed either
using dereference (*) or subscript ([]).  That makes five possible
variations for the sizeof, right?  So twenty combinations in total.

>> And what would it mean to match e.g. this ?
>>
>> 	memcpy(dst_ptr, src_ptr, n * sizeof(*src_arr))
>
> The Coccinelle software takes care for commutativity by isomorphisms.
> https://github.com/coccinelle/coccinelle/blob/19ee1697bf152d37a78a20cefe148775bf4b0e0d/standard.iso#L241

OK, but I had a different concern (more below).

>> At least the element size would be the same, but I'd rather shy away from
>> transforming weird cases like this automatically.
>
> Do you mean to specify additional restrictions by SmPL code?

Let's take this silly C fragment as an example:

	char *src = strdup("foo");
	size_t src_len = strlen(src);
	char *dst = malloc(src_len);
	char unrelated[17];
	memcpy(dst, src, src_len * sizeof(*unrelated));

My point is that taking the size of something that is neither source nor
destination is weird enough that it should be left alone by semantic
patches.  Matching should be precise enough to avoid false
transformations.

>>   void *memmove(void *dest, const void *src, size_t n);
>>   void *memcpy(void *dest, const void *src, size_t n);
>>
>>   COPY_ARRAY(dst, src, n)
>>   MOVE_ARRAY(dst, src, n)
>
> Can the replacement of these functions by macro calls be combined further
> by improved SmPL code?

Very likely.

>>> Possible nicer run time characteristics by the Coccinelle software.
>>
>> How much faster is it exactly?
>
> The answer will depend on efforts which you would like to invest
> in corresponding (representative) measurements.

Is that some kind of quantum effect? ;-)

When I try to convince people to apply a patch that is intended to
speed up something, I often use https://github.com/sharkdp/hyperfine
these days.

René

Re: coccinelle: adjustments for array.cocci?

[René Scharfe]

Am 17.11.19 um 19:36 schrieb Markus Elfring:
>>> It was chosen to transform source code fragments (pointer expressions)
>>> by two SmPL rules so that the search pattern “sizeof(T)” would work
>>> in the third rule.
>>
>> Ah, right, it would be nice to get rid of those normalization rules,
>> especially the second one.
>
> Thanks for such positive feedback.
>
>
>> I don't see how,
>
> Where is your view too limited at the moment?

I don't know.  Or perhaps it's rather too wide and I worry about
irrelevant details?

>> though, without either causing a combinatorial explosion
>
> Growing combinations can become more interesting, can't they?

Perhaps, but not if they blow up the size of the semantic patch or
the runtime of Coccinelle.

René

Re: coccinelle: adjustments for array.cocci?

[Markus Elfring]

>> Will there occasionally be a need to change only the required source code parts?
>
> Changing parts that don't need to be changed does not make sense to me.
> Why do you ask and how does it relate to the example at hand?

How does such feedback fit to the discussed SmPL change specification?

- E[...]
+ *(E)


>> Would you like to support corresponding development anyhow?
>
> I don't see me learning OCaml in the near future.

This can be fine.


> Or are you looking for donations?

Other (software) projects can benefit also from additional resources,
can't they?

Regards,
Markus

Re: coccinelle: adjustments for array.cocci?

[René Scharfe]

Am 19.11.19 um 21:21 schrieb Markus Elfring:
>>> Will there occasionally be a need to change only the required source code parts?
>>
>> Changing parts that don't need to be changed does not make sense to me.
>> Why do you ask and how does it relate to the example at hand?
>
> How does such feedback fit to the discussed SmPL change specification?
>
> - E[...]
> + *(E)

The cited fragment is from a rule that normalizes references to array
elements which are fed to sizeof.  It reduces the number of combinations
to consider in the rules following it, but it's not in itself a change
we'd want to apply.

The next helper rule turns sizeof operating on array elements to sizeof
on specific types.  That one is much uglier, as it removes the
information from whence the inferred type came.

In practice none of these helpers transformed any code that wasn't
matched by the final rule for using COPY_ARRAY.  It would be nice to get
rid of them nevertheless, to rule out such side-effects.  I just don't
see a practical way to make do without them, though.

René