Re: [PATCH] coccinelle: improve array.cocci

From: René Scharfe <l.s.r@web.de>
Date: Nov 19, 2019, 19:15 UTC
Message-ID: <d291ec11-c0f3-2918-193d-49fcbd65a18e@web.de>
In-Reply-To: <0d9cf772-268d-bd00-1cbb-0bbbec9dfc9a@web.de>

Am 18.11.19 um 17:10 schrieb Markus Elfring:

Show 23 quoted lines

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 18 Nov 2019 17:00:37 +0100
>
> This script contained transformation rules for the semantic patch language
> which used similar code.
>
> 1. Delete two SmPL rules which were used to transform source code fragments
>    (pointer expressions) so that the search pattern “sizeof(T)” would work
>    in the third rule.
>    See also the topic “coccinelle: adjustments for array.cocci?”:
>    https://public-inbox.org/git/f28f5fb8-2814-9df5-faf2-7146ed1a1f4d@web.de/
>
> 2. Combine the remaining rules by using six SmPL disjunctions.
>
> 3. Adjust case distinctions and corresponding metavariables so that
>    the desired search for update candidates can be more complete.
>
> 4. Increase the precision for the specification of required changes.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  contrib/coccinelle/array.cocci | 100 ++++++---------------------------
>  1 file changed, 18 insertions(+), 82 deletions(-)

The diff is hard to read, so here's the resulting semantic patch:

-- start --
@@
type T;
T[] src_arr;
expression n, dst_e, src_e;
expression* dst_p_e, src_p_e;
@@
(
(
-memcpy
+COPY_ARRAY
|
-memmove
+MOVE_ARRAY
)
       (
        dst_e,
        src_e
-       , (n) * \( sizeof(T) \| sizeof( \( *(src_p_e) \| src_e[...] \| src_arr \) ) \)
+       , n
       )
|
+ALLOC_ARRAY(
             dst_p_e
-                    = xmalloc((n) * \( sizeof( \( *(src_p_e) \| src_e[...] \| src_arr \) ) \| sizeof(T) \))
+            , n)
)
-- end --

I like that COPY_ARRAY and MOVE_ARRAY are handled in the same rule, as they share the same parameters and do the same -- except that the latter handles overlaps, while the former may be a bit faster.

And I like that it's short.

I don't like that ALLOC_ARRAY is handled in the same rule, as it is quite different from the other two macros.

Coccinelle needs significantly longer to apply the new version. Here are times for master:

Benchmark #1: make contrib/coccinelle/array.cocci.patch
  Time (mean ± σ):     19.314 s ±  0.200 s    [User: 19.065 s, System: 0.224 s]
  Range (min … max):   19.009 s … 19.718 s    10 runs

... and here with the patch applied:

Benchmark #1: make contrib/coccinelle/array.cocci.patch
  Time (mean ± σ):     43.420 s ±  0.490 s    [User: 43.087 s, System: 0.273 s]
  Range (min … max):   42.636 s … 44.359 s    10 runs

The current version checks if source and destination are of the same type, and whether the sizeof operand is either said type or an element of source or destination. The new one does not. So I don't see claim 4 ("Increase the precision") fulfilled, quite the opposite rather. It can produce e.g. a transformation like this:

 void f(int *dst, char *src, size_t n)
 {
-	memcpy(dst, src, n * sizeof(short));
+	COPY_ARRAY(dst, src, n);
 }

The COPY_ARRAY there effectively expands to:

	memcpy(dst, src, n * sizeof(*dst));

... which is quite different -- if short is 2 bytes wide and int 4 bytes then we copy twice as many bytes as before.

I think an automatic transformation should only be generated if it is safe. It's hard to spot a weird case in a generated patch amid ten well-behaving ones.

Show 117 quoted lines

>
> diff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci
> index 46b8d2ee11..bcd6ff4793 100644
> --- a/contrib/coccinelle/array.cocci
> +++ b/contrib/coccinelle/array.cocci
> @@ -1,90 +1,26 @@
> -@@
> -expression dst, src, n, E;
> -@@
> -  memcpy(dst, src, n * sizeof(
> -- E[...]
> -+ *(E)
> -  ))
> -
>  @@
>  type T;
> -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)
> -  )
> -)
> -
> -@@
> -type T;
> -T *dst_ptr;
> -T *src_ptr;
> -T[] dst_arr;
>  T[] src_arr;
> -expression n;
> +expression n, dst_e, src_e;
> +expression* dst_p_e, src_p_e;
>  @@
>  (
> -- memcpy(dst_ptr, src_ptr, (n) * sizeof(T))
> -+ COPY_ARRAY(dst_ptr, src_ptr, n)
> -|
> -- memcpy(dst_ptr, src_arr, (n) * sizeof(T))
> -+ COPY_ARRAY(dst_ptr, src_arr, n)
> -|
> -- memcpy(dst_arr, src_ptr, (n) * sizeof(T))
> -+ COPY_ARRAY(dst_arr, src_ptr, n)
> -|
> -- memcpy(dst_arr, src_arr, (n) * sizeof(T))
> -+ COPY_ARRAY(dst_arr, src_arr, n)
> -)
> -
> -@@
> -type T;
> -T *dst;
> -T *src;
> -expression n;
> -@@
>  (
> -- memmove(dst, src, (n) * sizeof(*dst));
> -+ MOVE_ARRAY(dst, src, n);
> -|
> -- memmove(dst, src, (n) * sizeof(*src));
> -+ MOVE_ARRAY(dst, src, n);
> +-memcpy
> ++COPY_ARRAY
>  |
> -- memmove(dst, src, (n) * sizeof(T));
> -+ MOVE_ARRAY(dst, src, n);
> +-memmove
> ++MOVE_ARRAY
> +)
> +       (
> +        dst_e,
> +        src_e
> +-       , (n) * \( sizeof(T) \| sizeof( \( *(src_p_e) \| src_e[...] \| src_arr \) ) \)
> ++       , n
> +       )
> +|
> ++ALLOC_ARRAY(
> +             dst_p_e
> +-                    = xmalloc((n) * \( sizeof( \( *(src_p_e) \| src_e[...] \| src_arr \) ) \| sizeof(T) \))
> ++            , n)
>  )
> -
> -@@
> -type T;
> -T *ptr;
> -expression n;
> -@@
> -- ptr = xmalloc((n) * sizeof(*ptr));
> -+ ALLOC_ARRAY(ptr, n);
> -
> -@@
> -type T;
> -T *ptr;
> -expression n;
> -@@
> -- ptr = xmalloc((n) * sizeof(T));
> -+ ALLOC_ARRAY(ptr, n);
> --
> 2.24.0
>