Re: coccinelle: adjustments for array.cocci?

Markus Elfring [off-list ref] writes:

quoted

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.

quoted

quoted

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

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 [off-list ref] writes:

quoted

Am 12.11.19 um 16:08 schrieb Markus Elfring:

quoted

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)
)
       )

quoted

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;

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

Am 14.11.19 um 18:14 schrieb Markus Elfring:

quoted

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

I hoped it.

quoted

 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é

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

Hello,

I repeated the discussed source code transformation approach together
with the software combination “Coccinelle 1.0.8-00004-g842075f7” (OCaml 4.09).
https://github.com/coccinelle/coccinelle/commits/master

1. Yesterday I checked the source files out for the software “Git”
   according to the commit “The first batch post 2.24 cycle”.
   https://github.com/git/git/commit/d9f6f3b6195a0ca35642561e530798ad1469bd41

2. I restored a previous development status by the following command.

   git show 921d49be86 | patch -p1 -R

   See also:
   https://public-inbox.org/git/53346d52-e096-c651-f70a-ce6ca4d82ff9@web.de/

3. I stored a generated patch based on the currently released SmPL script.
   https://github.com/git/git/blob/177fbab747da4f58cb2a8ce010b3515c86dd67c9/contrib/coccinelle/array.cocci

4. I applied the following patch then.

diff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci
index 46b8d2ee11..89df184bbd 100644
--- a/contrib/coccinelle/array.cocci
+++ b/contrib/coccinelle/array.cocci

@@ -12,27 +12,21 @@ T *ptr;
 T[] arr;
 expression E, n;
 @@
-(
-  memcpy(ptr, E,
-- n * sizeof(*(ptr))
-+ n * sizeof(T)
-  )
-|
-  memcpy(arr, E,
-- n * sizeof(*(arr))
-+ n * sizeof(T)
-  )
-|
-  memcpy(E, ptr,
-- n * sizeof(*(ptr))
-+ n * sizeof(T)
-  )
-|
-  memcpy(E, arr,
-- n * sizeof(*(arr))
-+ n * sizeof(T)
-  )
+ 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)
 )
+       )

 @@
 type T;

   I suggested in this way to move a bit of SmPL code.

5. I stored another generated patch based on the adjusted SmPL script.

6. I performed a corresponding file comparison.

--- array-released.diff	2019-11-14 21:29:11.020576916 +0100
+++ array-reduced1.diff	2019-11-14 21:45:58.931956527 +0100

@@ -6,24 +6,10 @@
  	r->entry_count = t->entry_count;
  	r->delta_depth = t->delta_depth;
 -	memcpy(r->entries,t->entries,t->entry_count*sizeof(t->entries[0]));
-+	COPY_ARRAY(r->entries, t->entries, t->entry_count);
++	memcpy(r->entries,t->entries,t->entry_count*sizeof(*(t->entries)));
  	release_tree_content(t);
  	return r;
  }
-diff -u -p a/pretty.c b/pretty.c
---- a/pretty.c
-+++ b/pretty.c
-@@ -106,8 +106,8 @@ static void setup_commit_formats(void)
- 	commit_formats_len = ARRAY_SIZE(builtin_formats);
- 	builtin_formats_len = commit_formats_len;
- 	ALLOC_GROW(commit_formats, commit_formats_len, commit_formats_alloc);
--	memcpy(commit_formats, builtin_formats,
--	       sizeof(*builtin_formats)*ARRAY_SIZE(builtin_formats));
-+	COPY_ARRAY(commit_formats, builtin_formats,
-+		   ARRAY_SIZE(builtin_formats));
-
- 	git_config(git_pretty_formats_config, NULL);
- }
 diff -u -p a/packfile.c b/packfile.c
 --- a/packfile.c
 +++ b/packfile.c

@@ -36,17 +22,6 @@
  		} else {
  			ALLOC_GROW(poi_stack, poi_stack_nr+1, poi_stack_alloc);
  		}
-@@ -1698,8 +1698,8 @@ void *unpack_entry(struct repository *r,
- 		    && delta_stack == small_delta_stack) {
- 			delta_stack_alloc = alloc_nr(delta_stack_nr);
- 			ALLOC_ARRAY(delta_stack, delta_stack_alloc);
--			memcpy(delta_stack, small_delta_stack,
--			       sizeof(*delta_stack)*delta_stack_nr);
-+			COPY_ARRAY(delta_stack, small_delta_stack,
-+				   delta_stack_nr);
- 		} else {
- 			ALLOC_GROW(delta_stack, delta_stack_nr+1, delta_stack_alloc);
- 		}
 diff -u -p a/compat/regex/regexec.c b/compat/regex/regexec.c
 --- a/compat/regex/regexec.c
 +++ b/compat/regex/regexec.c

How do you think about the differences from this test result?

Regards,
Markus

Can another variant for a transformation rule help to clarify unexpected
software behaviour around data processing with the semantic patch language?

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


elfring@Sonne:~/Projekte/git/lokal> spatch contrib/coccinelle/array-test3.cocci fast-import.c
…

Regards,
Markus

It took a while to become more aware of software development challenges
for the safe data processing with the semantic patch language also
at such a source code place.
https://github.com/git/git/blob/3edfcc65fdfc708c1c8f1d314885eecf9beb9b67/fast-import.c#L640

I got the impression that the Coccinelle software is occasionally able
to determine from the search specification “sizeof(T)” the corresponding
data type for code like “*(t->entries)”.
But it seems that there are circumstances to consider where the desired
data type was not automatically determined.
Thus the data processing  can become safer by explicitly expressing
the case distinction for the handling of expressions.

Adjusted transformation rule:
@@
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
       )


Regards,
Markus

On Fri, 15 Nov 2019, Markus Elfring wrote:

quoted

--- array-released.diff	2019-11-14 21:29:11.020576916 +0100
+++ array-reduced1.diff	2019-11-14 21:45:58.931956527 +0100

@@ -6,24 +6,10 @@
  	r->entry_count = t->entry_count;
  	r->delta_depth = t->delta_depth;
 -	memcpy(r->entries,t->entries,t->entry_count*sizeof(t->entries[0]));
-+	COPY_ARRAY(r->entries, t->entries, t->entry_count);
++	memcpy(r->entries,t->entries,t->entry_count*sizeof(*(t->entries)));
  	release_tree_content(t);
  	return r;
  }

It took a while to become more aware of software development challenges
for the safe data processing with the semantic patch language also
at such a source code place.
https://github.com/git/git/blob/3edfcc65fdfc708c1c8f1d314885eecf9beb9b67/fast-import.c#L640

I got the impression that the Coccinelle software is occasionally able
to determine from the search specification “sizeof(T)” the corresponding
data type for code like “*(t->entries)”.

It can determine the type of t->entries if it has access to the definition
of the type of t.  This type may be in a header file.  If you want
Coccinelle to be able to find this information you can use the option
--all-includes or --recursive-includes.  It will be more efficient with
the option --include-headers-for-types.

julia

But it seems that there are circumstances to consider where the desired
data type was not automatically determined.
Thus the data processing  can become safer by explicitly expressing
the case distinction for the handling of expressions.

Adjusted transformation rule:
@@
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
       )


Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

quoted

https://github.com/git/git/blob/3edfcc65fdfc708c1c8f1d314885eecf9beb9b67/fast-import.c#L640

I got the impression that the Coccinelle software is occasionally able
to determine from the search specification “sizeof(T)” the corresponding
data type for code like “*(t->entries)”.

It can determine the type of t->entries if it has access to the definition
of the type of t.

Should this type determination always work here because the data structure
“tree_content” for the parameter “t” of the function “grow_tree_content”
is defined in the same source file?
https://github.com/git/git/blob/3edfcc65fdfc708c1c8f1d314885eecf9beb9b67/fast-import.c#L85

                   This type may be in a header file.  If you want
Coccinelle to be able to find this information you can use the option
--all-includes or --recursive-includes.  It will be more efficient with
the option --include-headers-for-types.

Such information can be more helpful in other situations than the mentioned
test case.

quoted

But it seems that there are circumstances to consider where the desired
data type was not automatically determined.

Would you like to take the presented differences from the discussed
before/after comparison better into account?

Regards,
Markus

It can determine the type of t->entries if it has access to the definition
of the type of t.

I would like to point another implementation detail out.

Another known function was also an update candidate.
https://github.com/git/git/blob/9a1180fc304ad9831641e5788e9c8d3dfc10ccdd/pretty.c#L90

elfring@Sonne:~/Projekte/git/lokal> spatch contrib/coccinelle/array.cocci pretty.c
…

@@ -106,8 +106,8 @@ static void setup_commit_formats(void)
        commit_formats_len = ARRAY_SIZE(builtin_formats);
        builtin_formats_len = commit_formats_len;
        ALLOC_GROW(commit_formats, commit_formats_len, commit_formats_alloc);
-       memcpy(commit_formats, builtin_formats,
-              sizeof(*builtin_formats)*ARRAY_SIZE(builtin_formats));
+       COPY_ARRAY(commit_formats, builtin_formats,
+                  ARRAY_SIZE(builtin_formats));

        git_config(git_pretty_formats_config, NULL);
 }

This patch generation can work based on the following SmPL code combination.

“…
expression n, x;
…
-      , (n) * \( sizeof(T) \| sizeof(*(x)) \)
…”

The asterisk should refer to a pointer expression within a sizeof operator.
I got informed that the semantic patch language would support such a restriction.

Thus I have tried out to specify the corresponding metavariables in this way.

“…
expression n;
expression* x;
…”

But the shown diff hunk is not regenerated by this SmPL script variant.
How should an array like “builtin_formats” (which is even defined in the same function)
be treated by the Coccinelle software in such use cases?

Regards,
Markus

On Fri, 15 Nov 2019, Markus Elfring wrote:

This seems quite unreadable, in contrast to the original code.

 @@
 type T;

   I suggested in this way to move a bit of SmPL code.

5. I stored another generated patch based on the adjusted SmPL script.

No idea what it means to store a patch.

I have no idea what is being compared here. The COPY_ARRAY thing looks
nice, but doesn't seem to have anything to do with your semantic patch.

julia

quoted

+ 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 seems quite unreadable, in contrast to the original code.

The code formatting can vary for improved applications of SmPL disjunctions.

See also related update suggestions:
* https://public-inbox.org/git/05ab1110-2115-7886-f890-9983caabc52c@web.de/
* https://public-inbox.org/git/75b9417b-14a7-c9c6-25eb-f6e05f340376@web.de/

quoted

5. I stored another generated patch based on the adjusted SmPL script.

No idea what it means to store a patch.

I put the output from the program “spatch” into a text file like “array-reduced1.diff”
in a selected directory.

quoted

6. I performed a corresponding file comparison.

--- array-released.diff	2019-11-14 21:29:11.020576916 +0100
+++ array-reduced1.diff	2019-11-14 21:45:58.931956527 +0100

@@ -6,24 +6,10 @@
  	r->entry_count = t->entry_count;
  	r->delta_depth = t->delta_depth;
 -	memcpy(r->entries,t->entries,t->entry_count*sizeof(t->entries[0]));
-+	COPY_ARRAY(r->entries, t->entries, t->entry_count);
++	memcpy(r->entries,t->entries,t->entry_count*sizeof(*(t->entries)));
  	release_tree_content(t);
  	return r;
  }

I have no idea what is being compared here.

I suggest to take another look at the described steps then.

The COPY_ARRAY thing looks nice, but doesn't seem to have anything to do
with your semantic patch.

I find your interpretation of the presented software situation questionable.

* I got the impression in the meantime that my suggestion for a refactoring
  of a specific SmPL disjunction influenced transformation results for
  a subsequent SmPL rule in unexpected ways.

* Other software adjustments and solution variants can trigger further
  development considerations, can't they?

Regards,
Markus