Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
> On Wed, Jul 06 2022, Junio C Hamano wrote:
>
>> The variables 'source', 'destination', and 'submodule_gitfile' are
>> all of type "const char **", and an element of such an array is of
>> "type const char *".
>>
>> Noticed while running "make contrib/coccinelle/array.cocci.patch".
>>
>> Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
>> ---
>>
>> * There is this rule in the array.cocci file:
>>
>> @@
>> 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);
>> )
>>
>> but it triggered only for modes[] array among the four parallel
>> arrays that are being moved here.
>>
> This seems to be the same sort case as noted in 7bd97d6dff3 (git: use
> COPY_ARRAY and MOVE_ARRAY in handle_alias(), 2019-09-19).
"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.
> I tried (going aginst the warnings in that commit about the
> non-generality) updating the rules to catch these cases, which yielded
> the below.
> I wonder if we should apply some version of it. I.e. one-off
> fix these, and perhaps have the cocci rule BUG() if we see this sort of
> code again (the form technically could be used, but it seems all our
> uses of it are ones we could convert to the simpler one...).
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.
> diff --git a/add-patch.c b/add-patch.c
> index 509ca04456b..eff338d3901 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -915,10 +915,9 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,
> file_diff->hunk_nr += splittable_into - 1;
> ALLOC_GROW(file_diff->hunk, file_diff->hunk_nr, file_diff->hunk_alloc);
> if (hunk_index + splittable_into < file_diff->hunk_nr)
> - memmove(file_diff->hunk + hunk_index + splittable_into,
> - file_diff->hunk + hunk_index + 1,
> - (file_diff->hunk_nr - hunk_index - splittable_into)
> - * sizeof(*hunk));
> + MOVE_ARRAY(file_diff->hunk + hunk_index + splittable_into,
> + file_diff->hunk + hunk_index + 1,
> + file_diff->hunk_nr - hunk_index - splittable_into);
This does not look all that more readable, unfortunately.
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 83a465ba831..f6187d1264a 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -282,14 +282,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
> remove_entry:
> if (--argc > 0) {
> int n = argc - i;
> - memmove(source + i, source + i + 1,
> - n * sizeof(char *));
> - memmove(destination + i, destination + i + 1,
> - n * sizeof(char *));
> - memmove(modes + i, modes + i + 1,
> - n * sizeof(enum update_mode));
> - memmove(submodule_gitfile + i, submodule_gitfile + i + 1,
> - n * sizeof(char *));
> + MOVE_ARRAY(source + i, source + i + 1, n);
> + MOVE_ARRAY(destination + i, destination + i + 1, n);
> + MOVE_ARRAY(modes + i, modes + i + 1, n);
> + MOVE_ARRAY(submodule_gitfile + i,
> + submodule_gitfile + i + 1, n);
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).
> diff --git a/commit.c b/commit.c
> index 1fb1b2ea90c..c23d3e3678f 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -151,10 +151,9 @@ int register_commit_graft(struct repository *r, struct commit_graft *graft,
> r->parsed_objects->grafts_alloc);
> r->parsed_objects->grafts_nr++;
> if (pos < r->parsed_objects->grafts_nr)
> - memmove(r->parsed_objects->grafts + pos + 1,
> - r->parsed_objects->grafts + pos,
> - (r->parsed_objects->grafts_nr - pos - 1) *
> - sizeof(*r->parsed_objects->grafts));
> + MOVE_ARRAY(r->parsed_objects->grafts + pos + 1,
> + r->parsed_objects->grafts + pos,
> + r->parsed_objects->grafts_nr - pos - 1);
Likewise.
> diff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci
> index 9a4f00cb1bd..988ff9a3fda 100644
> --- a/contrib/coccinelle/array.cocci
> +++ b/contrib/coccinelle/array.cocci
> @@ -73,6 +73,15 @@ expression n;
> + MOVE_ARRAY(dst, src, n);
> )
>
> +@@
> +expression D;
> +expression S;
> +expression N;
> +expression Z;
> +@@
> +- memmove(D, S, (N) * Z);
> ++ MOVE_ARRAY(D, S, N);
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?
