Re: [PATCH] builtin/mv.c: use correct type to compute size of an array element

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

"This" here means "sizeof(const T) and sizeof(T) are the same and our Cocci rules do not trigger when a wrong one is used", and I agree that is exactly the same issue as fixed by 7bd97d6dff3.

One off rewrite using an ultra-loose rule, with human vetting the result, may be a good idea---after all, we are encouraging our developers to use MOVE_ARRAY() when appropriate, so it does not exactly matter how you discovered a memmove() can be rewritten safely to MOVE_ARRAY() as long as the resulting patch is correct.

It is a different story to add a loose automation that is designed to produce incorrect rewrite, and expects that humans always vet the outcome. Given that we run these rules at CI (doesn't it block GGG submitters?), it is a bad idea.

This does not look all that more readable, unfortunately.

This is exactly what I sent. I guess the change in this hunk is not all that different from add-patch.c but somehow it makes it much easier to read. Perhaps that is only because these moves are much simpler (i.e. shift by 1).

Likewise.

This is definitely unwelcome, as there is nothing that ensures Z has the same value as sizeof(D[0]).

While our eyes are on array.cocci, I have a few observations on it.

This is not meant specifically to you, Ævar, but comments by those more familiar with Coccinelle (and I am sure the bar to pass is fairly low, as I am not all that familiar) are very much appreciated.

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

This seems to force us prefer sizeof(*(E)) over sizeof(E[i]), when it is used to compute the byte size of memcpy() operation. There is no reason to prefer one over the other, but I presume it is there only for convenience for the other rules in this file (I recall vaguely reading somewhere that these rules do not "execute" from top to bottom, so I wonder how effective it is?).

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

Likewise, but this one is a lot worse.

Taken alone, sizeof(*(ptr)) is far more preferrable than sizeof(T), because the code will be more maintainable.

    Side Note.  I know builtin/mv.c had this type mismatch between
    the variable and sizeof() from the beginning when 11be42a4 (Make
    git-mv a builtin, 2006-07-26) introduced both the variable
    declaration "const char **source" and memmove() on it, but a
    code that starts out with "char **src" with memmove() that moves
    part of src[] and uses sizeof(char *) to compute the byte size
    of the move would become broken the same way when a later
    developer tightens the declaration to use "const char **src"
    without realizing that they have to update the type used in
    sizeof().

So even though I am guessing that this is to allow the later rules to worry only about sizeof(T), I am a bit unhappy to see the rule. If an existing code matched this rule and get rewritten to use sizeof(T), not sizeof(*(ptr)), but did not match the other rules to be rewritten to use COPY_ARRAY(), the overall effect would be that the automation made the code worse.

    @@
    type T;
    T *dst_ptr;
    T *src_ptr;
    T[] dst_arr;
    T[] src_arr;
    expression n;
    @@
    (
    - 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)
    )

I take it that thanks to the earlier "meh -- between sizeof(*p) and sizeof(p[0]) there is no reason to prefer one over the other" and "oh, no, we should prefer sizeof(*p) not sizeof(typeof(*p)) but this one is the other way around" rules, this one only has to deal with sizeof(T).

Am I reading it correctly?

    @@
    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);
    |
    - memmove(dst, src, (n) * sizeof(T));
    + MOVE_ARRAY(dst, src, n);
    )

What I find interesting is that this one seems to be able to do the necessary rewrite without having to do the "turn everything into sizeof(T) first" trick. If this approach works well, I'd rather see the COPY_ARRAY() done without the first two preliminary rewrite rules.

I wonder if the pattern in the first rule catches sizeof(dst[0]) instead of sizeof(*dst), though.

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

Is it a no-op rewrite if we replace the above two rules with something like:

. @@ . type T; . T *ptr; . expression n; . @@ . ( . - ptr = xmalloc((n) * sizeof(*ptr)); . + ALLOC_ARRAY(ptr, n); . | . - ptr = xmalloc((n) * sizeof(T)); . + ALLOC_ARRAY(ptr, n); . )

or even following the pattern of the next one ...

. @@ . type T; . T *ptr; . expression n; . @@ . - ptr = xmalloc((n) * \( sizeof(*ptr) \| sizeof(T) \)) . + ALLOC_ARRAY(ptr, n);

... I have to wonder?  I like the simplicity of this pattern.

    @@
    type T;
    T *ptr;
    expression n != 1;
    @@
    - ptr = xcalloc(n, \( sizeof(*ptr) \| sizeof(T) \) )
    + CALLOC_ARRAY(ptr, n)

And this leaves xcalloc(1, ...) alone because it is a way to get a cleared block of memory that may not be an array at all. Shouldn't we have "n != 1" for xmalloc rule as well, I wonder, if only for consistency?