Re: [Cocci] git-coccinelle: adjustments for array.cocci?

Re: [Cocci] git-coccinelle: adjustments for array.cocci?

[Markus Elfring]

> 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
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] git-coccinelle: adjustments for array.cocci?

[Markus Elfring]

> --- 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;
>   }

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
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] git-coccinelle: adjustments for array.cocci?

[Markus Elfring]

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

Re: [Cocci] git-coccinelle: adjustments for array.cocci?

[Julia Lawall]

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



On Fri, 15 Nov 2019, Markus Elfring wrote:

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

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

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

Re: [Cocci] git-coccinelle: adjustments for array.cocci?

[Markus Elfring]

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


>> 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
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] git-coccinelle: adjustments for array.cocci?

[Markus Elfring]

> 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
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] git-coccinelle: adjustments for array.cocci?

[Julia Lawall]

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



On Fri, 15 Nov 2019, Markus Elfring wrote:

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

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.

> 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. The COPY_ARRAY thing looks
nice, but doesn't seem to have anything to do with your semantic patch.

julia



> -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
> _______________________________________________
> Cocci mailing list
> Cocci@systeme.lip6.fr
> https://systeme.lip6.fr/mailman/listinfo/cocci
>

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

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

Re: [Cocci] git-coccinelle: adjustments for array.cocci?

[Markus Elfring]

>> + 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/


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


>> 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
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci