{"thread":{"id":"58115","subject":"[PATCH] builtin/mv.c: use correct type to compute size of an array element","startedAt":"2022-07-07T02:02:24Z","lastAt":"2022-07-18T20:31:05Z","messageCount":18,"participants":["Junio C Hamano","Ævar Arnfjörð Bjarmason","René Scharfe","Jeff King","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"458552","messageId":"xmqq8rp54r4l.fsf@gitster.g","threadId":"58115","inReplyTo":null,"subject":"[PATCH] builtin/mv.c: use correct type to compute size of an array element","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-07T02:02:18Z","receivedAt":"2022-07-07T02:02:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The variables 'source', 'destination', and 'submodule_gitfile' are\nall of type \"const char **\", and an element of such an array is of\n\"type const char *\".\n\nNoticed while running \"make contrib/coccinelle/array.cocci.patch\".\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * There is this rule in the array.cocci file:\n\n        @@\n        type T;\n        T *dst;\n        T *src;\n        expression n;\n        @@\n        (\n        - memmove(dst, src, (n) * sizeof(*dst));\n        + MOVE_ARRAY(dst, src, n);\n        |\n        - memmove(dst, src, (n) * sizeof(*src));\n        + MOVE_ARRAY(dst, src, n);\n        |\n        - memmove(dst, src, (n) * sizeof(T));\n        + MOVE_ARRAY(dst, src, n);\n        )\n\n   but it triggered only for modes[] array among the four parallel\n   arrays that are being moved here.\n\n   There are a few tangents.\n\n   * Should we in general use sizeof(TYPE) in these cases, instead\n     of the size of the zeroth element, e.g.\n\n \t\tmemmove(source + i, source + i + 1,\n\t\t\tn * sizeof(source[i]));\n    \n     It would have been caught by the above Coccinelle rule (we are\n     taking the size of *dst).\n\n   * Shouldn't we have an array of struct with four members, instead\n     of four parallel arrays, e.g.\n\n\t\tstruct {\n\t\t\tconst char *source;\n\t\t\tconst char *destination;\n\t\t\tenum update_mode mode;\n\t\t\tconst char *submodule_gitfile;\n\t\t} *mv_file;\n\n   The latter question is important to answer before we accept\n   Coccinelle-suggested rewrite to use four MOVE_ARRAY() on these\n   four parallel arrays on top of this fix.\n\n builtin/mv.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git c/builtin/mv.c w/builtin/mv.c\nindex 2a38e2af46..d419e83f0f 100644\n--- c/builtin/mv.c\n+++ w/builtin/mv.c\n@@ -377,13 +377,13 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\tif (--argc > 0) {\n \t\t\tint n = argc - i;\n \t\t\tmemmove(source + i, source + i + 1,\n-\t\t\t\tn * sizeof(char *));\n+\t\t\t\tn * sizeof(const char *));\n \t\t\tmemmove(destination + i, destination + i + 1,\n-\t\t\t\tn * sizeof(char *));\n+\t\t\t\tn * sizeof(const char *));\n \t\t\tmemmove(modes + i, modes + i + 1,\n \t\t\t\tn * sizeof(enum update_mode));\n \t\t\tmemmove(submodule_gitfile + i, submodule_gitfile + i + 1,\n-\t\t\t\tn * sizeof(char *));\n+\t\t\t\tn * sizeof(const char *));\n \t\t\ti--;\n \t\t}\n \t}\n"},{"id":"458553","messageId":"xmqqy1x531vp.fsf@gitster.g","threadId":"58115","inReplyTo":"xmqq8rp54r4l.fsf@gitster.g","subject":"[PATCH v2] builtin/mv.c: use the MOVE_ARRAY() macro instead of memmove()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-07T05:52:58Z","receivedAt":"2022-07-07T05:53:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The variables 'source', 'destination', and 'submodule_gitfile' are\nall of type \"const char **\", and an element of such an array is of\n\"type const char *\", but these memmove() calls were written as if\nthese variables are of type \"char **\".\n\nOnce these memmove() calls are fixed to use the correct type to\ncompute the number of bytes to be moved, e.g.\n\n-      memmove(source + i, source + i + 1, n * sizeof(char *));\n+      memmove(source + i, source + i + 1, n * sizeof(const char *));\n\nexisting contrib/coccinelle/array.cocci rules can recognize them as\ncandidates for turning into MOVE_ARRAY().\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/mv.c | 10 ++++------\n 1 file changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex 83a465ba83..d599233057 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -282,14 +282,12 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n remove_entry:\n \t\tif (--argc > 0) {\n \t\t\tint n = argc - i;\n-\t\t\tmemmove(source + i, source + i + 1,\n-\t\t\t\tn * sizeof(char *));\n-\t\t\tmemmove(destination + i, destination + i + 1,\n-\t\t\t\tn * sizeof(char *));\n+\t\t\tMOVE_ARRAY(source + i, source + i + 1, n);\n+\t\t\tMOVE_ARRAY(destination + i, destination + i + 1, n);\n \t\t\tmemmove(modes + i, modes + i + 1,\n \t\t\t\tn * sizeof(enum update_mode));\n-\t\t\tmemmove(submodule_gitfile + i, submodule_gitfile + i + 1,\n-\t\t\t\tn * sizeof(char *));\n+\t\t\tMOVE_ARRAY(submodule_gitfile + i,\n+\t\t\t\t   submodule_gitfile + i + 1, n);\n \t\t\ti--;\n \t\t}\n \t}\n-- \n2.37.0-211-gafcdf5f063\n\n"},{"id":"458562","messageId":"220707.86y1x585wy.gmgdl@evledraar.gmail.com","threadId":"58115","inReplyTo":"xmqq8rp54r4l.fsf@gitster.g","subject":"Re: [PATCH] builtin/mv.c: use correct type to compute size of an array element","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-07T12:11:22Z","receivedAt":"2022-07-07T12:26:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jul 06 2022, Junio C Hamano wrote:\n\n> The variables 'source', 'destination', and 'submodule_gitfile' are\n> all of type \"const char **\", and an element of such an array is of\n> \"type const char *\".\n>\n> Noticed while running \"make contrib/coccinelle/array.cocci.patch\".\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>\n>  * There is this rule in the array.cocci file:\n>\n>         @@\n>         type T;\n>         T *dst;\n>         T *src;\n>         expression n;\n>         @@\n>         (\n>         - memmove(dst, src, (n) * sizeof(*dst));\n>         + MOVE_ARRAY(dst, src, n);\n>         |\n>         - memmove(dst, src, (n) * sizeof(*src));\n>         + MOVE_ARRAY(dst, src, n);\n>         |\n>         - memmove(dst, src, (n) * sizeof(T));\n>         + MOVE_ARRAY(dst, src, n);\n>         )\n>\n>    but it triggered only for modes[] array among the four parallel\n>    arrays that are being moved here.\n>\n>    There are a few tangents.\n>\n>    * Should we in general use sizeof(TYPE) in these cases, instead\n>      of the size of the zeroth element, e.g.\n>\n>  \t\tmemmove(source + i, source + i + 1,\n> \t\t\tn * sizeof(source[i]));\n>     \n>      It would have been caught by the above Coccinelle rule (we are\n>      taking the size of *dst).\n>\n>    * Shouldn't we have an array of struct with four members, instead\n>      of four parallel arrays, e.g.\n>\n> \t\tstruct {\n> \t\t\tconst char *source;\n> \t\t\tconst char *destination;\n> \t\t\tenum update_mode mode;\n> \t\t\tconst char *submodule_gitfile;\n> \t\t} *mv_file;\n>\n>    The latter question is important to answer before we accept\n>    Coccinelle-suggested rewrite to use four MOVE_ARRAY() on these\n>    four parallel arrays on top of this fix.\n\nThis seems to be the same sort case as noted in 7bd97d6dff3 (git: use\nCOPY_ARRAY and MOVE_ARRAY in handle_alias(), 2019-09-19).\n\nI tried (going aginst the warnings in that commit about the\nnon-generality) updating the rules to catch these cases, which yielded\nthe below. I wonder if we should apply some version of it. I.e. one-off\nfix these, and perhaps have the cocci rule BUG() if we see this sort of\ncode again (the form technically could be used, but it seems all our\nuses of it are ones we could convert to the simpler one...).\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 509ca04456b..eff338d3901 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -915,10 +915,9 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,\n \tfile_diff->hunk_nr += splittable_into - 1;\n \tALLOC_GROW(file_diff->hunk, file_diff->hunk_nr, file_diff->hunk_alloc);\n \tif (hunk_index + splittable_into < file_diff->hunk_nr)\n-\t\tmemmove(file_diff->hunk + hunk_index + splittable_into,\n-\t\t\tfile_diff->hunk + hunk_index + 1,\n-\t\t\t(file_diff->hunk_nr - hunk_index - splittable_into)\n-\t\t\t* sizeof(*hunk));\n+\t\tMOVE_ARRAY(file_diff->hunk + hunk_index + splittable_into,\n+\t\t\t   file_diff->hunk + hunk_index + 1,\n+\t\t\t   file_diff->hunk_nr - hunk_index - splittable_into);\n \thunk = file_diff->hunk + hunk_index;\n \thunk->splittable_into = 1;\n \tmemset(hunk + 1, 0, (splittable_into - 1) * sizeof(*hunk));\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex 83a465ba831..f6187d1264a 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -282,14 +282,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n remove_entry:\n \t\tif (--argc > 0) {\n \t\t\tint n = argc - i;\n-\t\t\tmemmove(source + i, source + i + 1,\n-\t\t\t\tn * sizeof(char *));\n-\t\t\tmemmove(destination + i, destination + i + 1,\n-\t\t\t\tn * sizeof(char *));\n-\t\t\tmemmove(modes + i, modes + i + 1,\n-\t\t\t\tn * sizeof(enum update_mode));\n-\t\t\tmemmove(submodule_gitfile + i, submodule_gitfile + i + 1,\n-\t\t\t\tn * sizeof(char *));\n+\t\t\tMOVE_ARRAY(source + i, source + i + 1, n);\n+\t\t\tMOVE_ARRAY(destination + i, destination + i + 1, n);\n+\t\t\tMOVE_ARRAY(modes + i, modes + i + 1, n);\n+\t\t\tMOVE_ARRAY(submodule_gitfile + i,\n+\t\t\t\t   submodule_gitfile + i + 1, n);\n \t\t\ti--;\n \t\t}\n \t}\ndiff --git a/commit.c b/commit.c\nindex 1fb1b2ea90c..c23d3e3678f 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -151,10 +151,9 @@ int register_commit_graft(struct repository *r, struct commit_graft *graft,\n \t\t   r->parsed_objects->grafts_alloc);\n \tr->parsed_objects->grafts_nr++;\n \tif (pos < r->parsed_objects->grafts_nr)\n-\t\tmemmove(r->parsed_objects->grafts + pos + 1,\n-\t\t\tr->parsed_objects->grafts + pos,\n-\t\t\t(r->parsed_objects->grafts_nr - pos - 1) *\n-\t\t\tsizeof(*r->parsed_objects->grafts));\n+\t\tMOVE_ARRAY(r->parsed_objects->grafts + pos + 1,\n+\t\t\t   r->parsed_objects->grafts + pos,\n+\t\t\t   r->parsed_objects->grafts_nr - pos - 1);\n \tr->parsed_objects->grafts[pos] = graft;\n \tunparse_commit(r, &graft->oid);\n \treturn 0;\ndiff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci\nindex 9a4f00cb1bd..988ff9a3fda 100644\n--- a/contrib/coccinelle/array.cocci\n+++ b/contrib/coccinelle/array.cocci\n@@ -73,6 +73,15 @@ expression n;\n + MOVE_ARRAY(dst, src, n);\n )\n \n+@@\n+expression D;\n+expression S;\n+expression N;\n+expression Z;\n+@@\n+- memmove(D, S, (N) * Z);\n++ MOVE_ARRAY(D, S, N);\n+\n @@\n type T;\n T *ptr;\n"},{"id":"458585","messageId":"xmqq1quw23r8.fsf@gitster.g","threadId":"58115","inReplyTo":"220707.86y1x585wy.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] builtin/mv.c: use correct type to compute size of an array element","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-07T18:10:03Z","receivedAt":"2022-07-07T18:10:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> On Wed, Jul 06 2022, Junio C Hamano wrote:\n>\n>> The variables 'source', 'destination', and 'submodule_gitfile' are\n>> all of type \"const char **\", and an element of such an array is of\n>> \"type const char *\".\n>>\n>> Noticed while running \"make contrib/coccinelle/array.cocci.patch\".\n>>\n>> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n>> ---\n>>\n>>  * There is this rule in the array.cocci file:\n>>\n>>         @@\n>>         type T;\n>>         T *dst;\n>>         T *src;\n>>         expression n;\n>>         @@\n>>         (\n>>         - memmove(dst, src, (n) * sizeof(*dst));\n>>         + MOVE_ARRAY(dst, src, n);\n>>         |\n>>         - memmove(dst, src, (n) * sizeof(*src));\n>>         + MOVE_ARRAY(dst, src, n);\n>>         |\n>>         - memmove(dst, src, (n) * sizeof(T));\n>>         + MOVE_ARRAY(dst, src, n);\n>>         )\n>>\n>>    but it triggered only for modes[] array among the four parallel\n>>    arrays that are being moved here.\n>>\n> This seems to be the same sort case as noted in 7bd97d6dff3 (git: use\n> COPY_ARRAY and MOVE_ARRAY in handle_alias(), 2019-09-19).\n\n\"This\" here means \"sizeof(const T) and sizeof(T) are the same and\nour Cocci rules do not trigger when a wrong one is used\", and I\nagree that is exactly the same issue as fixed by 7bd97d6dff3.\n\n> I tried (going aginst the warnings in that commit about the\n> non-generality) updating the rules to catch these cases, which yielded\n> the below.\n\n> I wonder if we should apply some version of it. I.e. one-off\n> fix these, and perhaps have the cocci rule BUG() if we see this sort of\n> code again (the form technically could be used, but it seems all our\n> uses of it are ones we could convert to the simpler one...).\n\nOne off rewrite using an ultra-loose rule, with human vetting the\nresult, may be a good idea---after all, we are encouraging our\ndevelopers to use MOVE_ARRAY() when appropriate, so it does not\nexactly matter how you discovered a memmove() can be rewritten\nsafely to MOVE_ARRAY() as long as the resulting patch is correct.\n\nIt is a different story to add a loose automation that is designed\nto produce incorrect rewrite, and expects that humans always vet the\noutcome.  Given that we run these rules at CI (doesn't it block GGG\nsubmitters?), it is a bad idea.\n\n> diff --git a/add-patch.c b/add-patch.c\n> index 509ca04456b..eff338d3901 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -915,10 +915,9 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,\n>  \tfile_diff->hunk_nr += splittable_into - 1;\n>  \tALLOC_GROW(file_diff->hunk, file_diff->hunk_nr, file_diff->hunk_alloc);\n>  \tif (hunk_index + splittable_into < file_diff->hunk_nr)\n> -\t\tmemmove(file_diff->hunk + hunk_index + splittable_into,\n> -\t\t\tfile_diff->hunk + hunk_index + 1,\n> -\t\t\t(file_diff->hunk_nr - hunk_index - splittable_into)\n> -\t\t\t* sizeof(*hunk));\n> +\t\tMOVE_ARRAY(file_diff->hunk + hunk_index + splittable_into,\n> +\t\t\t   file_diff->hunk + hunk_index + 1,\n> +\t\t\t   file_diff->hunk_nr - hunk_index - splittable_into);\n\nThis does not look all that more readable, unfortunately.\n\n> diff --git a/builtin/mv.c b/builtin/mv.c\n> index 83a465ba831..f6187d1264a 100644\n> --- a/builtin/mv.c\n> +++ b/builtin/mv.c\n> @@ -282,14 +282,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n>  remove_entry:\n>  \t\tif (--argc > 0) {\n>  \t\t\tint n = argc - i;\n> -\t\t\tmemmove(source + i, source + i + 1,\n> -\t\t\t\tn * sizeof(char *));\n> -\t\t\tmemmove(destination + i, destination + i + 1,\n> -\t\t\t\tn * sizeof(char *));\n> -\t\t\tmemmove(modes + i, modes + i + 1,\n> -\t\t\t\tn * sizeof(enum update_mode));\n> -\t\t\tmemmove(submodule_gitfile + i, submodule_gitfile + i + 1,\n> -\t\t\t\tn * sizeof(char *));\n> +\t\t\tMOVE_ARRAY(source + i, source + i + 1, n);\n> +\t\t\tMOVE_ARRAY(destination + i, destination + i + 1, n);\n> +\t\t\tMOVE_ARRAY(modes + i, modes + i + 1, n);\n> +\t\t\tMOVE_ARRAY(submodule_gitfile + i,\n> +\t\t\t\t   submodule_gitfile + i + 1, n);\n\nThis is exactly what I sent.  I guess the change in this hunk is not\nall that different from add-patch.c but somehow it makes it much\neasier to read.  Perhaps that is only because these moves are much\nsimpler (i.e. shift by 1).\n\n> diff --git a/commit.c b/commit.c\n> index 1fb1b2ea90c..c23d3e3678f 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -151,10 +151,9 @@ int register_commit_graft(struct repository *r, struct commit_graft *graft,\n>  \t\t   r->parsed_objects->grafts_alloc);\n>  \tr->parsed_objects->grafts_nr++;\n>  \tif (pos < r->parsed_objects->grafts_nr)\n> -\t\tmemmove(r->parsed_objects->grafts + pos + 1,\n> -\t\t\tr->parsed_objects->grafts + pos,\n> -\t\t\t(r->parsed_objects->grafts_nr - pos - 1) *\n> -\t\t\tsizeof(*r->parsed_objects->grafts));\n> +\t\tMOVE_ARRAY(r->parsed_objects->grafts + pos + 1,\n> +\t\t\t   r->parsed_objects->grafts + pos,\n> +\t\t\t   r->parsed_objects->grafts_nr - pos - 1);\n\nLikewise.\n\n> diff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci\n> index 9a4f00cb1bd..988ff9a3fda 100644\n> --- a/contrib/coccinelle/array.cocci\n> +++ b/contrib/coccinelle/array.cocci\n> @@ -73,6 +73,15 @@ expression n;\n>  + MOVE_ARRAY(dst, src, n);\n>  )\n>  \n> +@@\n> +expression D;\n> +expression S;\n> +expression N;\n> +expression Z;\n> +@@\n> +- memmove(D, S, (N) * Z);\n> ++ MOVE_ARRAY(D, S, N);\n\nThis is definitely unwelcome, as there is nothing that ensures Z has\nthe same value as sizeof(D[0]).\n\nWhile our eyes are on array.cocci, I have a few observations on it.\n\nThis is not meant specifically to you, Ævar, but comments by those\nmore familiar with Coccinelle (and I am sure the bar to pass is\nfairly low, as I am not all that familiar) are very much\nappreciated.\n\n    @@\n    expression dst, src, n, E;\n    @@\n      memcpy(dst, src, n * sizeof(\n    - E[...]\n    + *(E)\n      ))\n\nThis seems to force us prefer sizeof(*(E)) over sizeof(E[i]), when\nit is used to compute the byte size of memcpy() operation.  There is\nno reason to prefer one over the other, but I presume it is there\nonly for convenience for the other rules in this file (I recall\nvaguely reading somewhere that these rules do not \"execute\" from top\nto bottom, so I wonder how effective it is?).\n\n    @@\n    type T;\n    T *ptr;\n    T[] arr;\n    expression E, n;\n    @@\n    (\n      memcpy(ptr, E,\n    - n * sizeof(*(ptr))\n    + n * sizeof(T)\n      )\n    |\n      memcpy(arr, E,\n    - n * sizeof(*(arr))\n    + n * sizeof(T)\n      )\n    |\n      memcpy(E, ptr,\n    - n * sizeof(*(ptr))\n    + n * sizeof(T)\n      )\n    |\n      memcpy(E, arr,\n    - n * sizeof(*(arr))\n    + n * sizeof(T)\n      )\n    )\n\nLikewise, but this one is a lot worse.\n\nTaken alone, sizeof(*(ptr)) is far more preferrable than sizeof(T),\nbecause the code will be more maintainable.\n\n    Side Note.  I know builtin/mv.c had this type mismatch between\n    the variable and sizeof() from the beginning when 11be42a4 (Make\n    git-mv a builtin, 2006-07-26) introduced both the variable\n    declaration \"const char **source\" and memmove() on it, but a\n    code that starts out with \"char **src\" with memmove() that moves\n    part of src[] and uses sizeof(char *) to compute the byte size\n    of the move would become broken the same way when a later\n    developer tightens the declaration to use \"const char **src\"\n    without realizing that they have to update the type used in\n    sizeof().\n\nSo even though I am guessing that this is to allow the later rules\nto worry only about sizeof(T), I am a bit unhappy to see the rule.\nIf an existing code matched this rule and get rewritten to use\nsizeof(T), not sizeof(*(ptr)), but did not match the other rules to\nbe rewritten to use COPY_ARRAY(), the overall effect would be that\nthe automation made the code worse.\n\n    @@\n    type T;\n    T *dst_ptr;\n    T *src_ptr;\n    T[] dst_arr;\n    T[] src_arr;\n    expression n;\n    @@\n    (\n    - memcpy(dst_ptr, src_ptr, (n) * sizeof(T))\n    + COPY_ARRAY(dst_ptr, src_ptr, n)\n    |\n    - memcpy(dst_ptr, src_arr, (n) * sizeof(T))\n    + COPY_ARRAY(dst_ptr, src_arr, n)\n    |\n    - memcpy(dst_arr, src_ptr, (n) * sizeof(T))\n    + COPY_ARRAY(dst_arr, src_ptr, n)\n    |\n    - memcpy(dst_arr, src_arr, (n) * sizeof(T))\n    + COPY_ARRAY(dst_arr, src_arr, n)\n    )\n\nI take it that thanks to the earlier \"meh -- between sizeof(*p) and\nsizeof(p[0]) there is no reason to prefer one over the other\" and\n\"oh, no, we should prefer sizeof(*p) not sizeof(typeof(*p)) but this\none is the other way around\" rules, this one only has to deal with\nsizeof(T).\n\nAm I reading it correctly?\n\n    @@\n    type T;\n    T *dst;\n    T *src;\n    expression n;\n    @@\n    (\n    - memmove(dst, src, (n) * sizeof(*dst));\n    + MOVE_ARRAY(dst, src, n);\n    |\n    - memmove(dst, src, (n) * sizeof(*src));\n    + MOVE_ARRAY(dst, src, n);\n    |\n    - memmove(dst, src, (n) * sizeof(T));\n    + MOVE_ARRAY(dst, src, n);\n    )\n\nWhat I find interesting is that this one seems to be able to do the\nnecessary rewrite without having to do the \"turn everything into\nsizeof(T) first\" trick.  If this approach works well, I'd rather see\nthe COPY_ARRAY() done without the first two preliminary rewrite\nrules.\n\nI wonder if the pattern in the first rule catches sizeof(dst[0])\ninstead of sizeof(*dst), though.\n\n    @@\n    type T;\n    T *ptr;\n    expression n;\n    @@\n    - ptr = xmalloc((n) * sizeof(*ptr));\n    + ALLOC_ARRAY(ptr, n);\n\n    @@\n    type T;\n    T *ptr;\n    expression n;\n    @@\n    - ptr = xmalloc((n) * sizeof(T));\n    + ALLOC_ARRAY(ptr, n);\n\nIs it a no-op rewrite if we replace the above two rules with\nsomething like:\n\n.   @@\n.   type T;\n.   T *ptr;\n.   expression n;\n.   @@\n.   (\n.   - ptr = xmalloc((n) * sizeof(*ptr));\n.   + ALLOC_ARRAY(ptr, n);\n.   |\n.   - ptr = xmalloc((n) * sizeof(T));\n.   + ALLOC_ARRAY(ptr, n);\n.   )\n\nor even following the pattern of the next one ...\n\n.   @@\n.   type T;\n.   T *ptr;\n.   expression n;\n.   @@\n.   - ptr = xmalloc((n) * \\( sizeof(*ptr) \\| sizeof(T) \\))\n.   + ALLOC_ARRAY(ptr, n);\n\n... I have to wonder?  I like the simplicity of this pattern.\n\n    @@\n    type T;\n    T *ptr;\n    expression n != 1;\n    @@\n    - ptr = xcalloc(n, \\( sizeof(*ptr) \\| sizeof(T) \\) )\n    + CALLOC_ARRAY(ptr, n)\n\nAnd this leaves xcalloc(1, ...) alone because it is a way to get a\ncleared block of memory that may not be an array at all.  Shouldn't\nwe have \"n != 1\" for xmalloc rule as well, I wonder, if only for\nconsistency?\n"},{"id":"458590","messageId":"69cae987-e6cd-d4f5-9119-b393efe544a2@web.de","threadId":"58115","inReplyTo":"220707.86y1x585wy.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] builtin/mv.c: use correct type to compute size of an array element","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-07-07T18:27:44Z","receivedAt":"2022-07-07T18:30:03Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 07.07.22 um 14:11 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Wed, Jul 06 2022, Junio C Hamano wrote:\n>\n>> The variables 'source', 'destination', and 'submodule_gitfile' are\n>> all of type \"const char **\", and an element of such an array is of\n>> \"type const char *\".\n>>\n>> Noticed while running \"make contrib/coccinelle/array.cocci.patch\".\n>>\n>> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n>> ---\n>>\n>>  * There is this rule in the array.cocci file:\n>>\n>>         @@\n>>         type T;\n>>         T *dst;\n>>         T *src;\n>>         expression n;\n>>         @@\n>>         (\n>>         - memmove(dst, src, (n) * sizeof(*dst));\n>>         + MOVE_ARRAY(dst, src, n);\n>>         |\n>>         - memmove(dst, src, (n) * sizeof(*src));\n>>         + MOVE_ARRAY(dst, src, n);\n>>         |\n>>         - memmove(dst, src, (n) * sizeof(T));\n>>         + MOVE_ARRAY(dst, src, n);\n>>         )\n>>\n>>    but it triggered only for modes[] array among the four parallel\n>>    arrays that are being moved here.\n>>\n>>    There are a few tangents.\n>>\n>>    * Should we in general use sizeof(TYPE) in these cases, instead\n>>      of the size of the zeroth element, e.g.\n>>\n>>  \t\tmemmove(source + i, source + i + 1,\n>> \t\t\tn * sizeof(source[i]));\n>>\n>>      It would have been caught by the above Coccinelle rule (we are\n>>      taking the size of *dst).\n\nYou mean using normalization rules for memmove like we have for memcpy,\nto cover a wider range of combinations without having explicit rules for\neach one?  Makes sense -- memcpy and memmove take the same arguments, so\nwe should treat them the same.\n\n>>\n>>    * Shouldn't we have an array of struct with four members, instead\n>>      of four parallel arrays, e.g.\n>>\n>> \t\tstruct {\n>> \t\t\tconst char *source;\n>> \t\t\tconst char *destination;\n>> \t\t\tenum update_mode mode;\n>> \t\t\tconst char *submodule_gitfile;\n>> \t\t} *mv_file;\n\nPossibly, but that would require restructuring the code.  Currently the\nfunction internal_prefix_pathspec() is used to build the string arrays\nsource and destination separately.\n\n>>\n>>    The latter question is important to answer before we accept\n>>    Coccinelle-suggested rewrite to use four MOVE_ARRAY() on these\n>>    four parallel arrays on top of this fix.\n>\n> This seems to be the same sort case as noted in 7bd97d6dff3 (git: use\n> COPY_ARRAY and MOVE_ARRAY in handle_alias(), 2019-09-19).\n>\n> I tried (going aginst the warnings in that commit about the\n> non-generality) updating the rules to catch these cases, which yielded\n> the below. I wonder if we should apply some version of it. I.e. one-off\n> fix these, and perhaps have the cocci rule BUG() if we see this sort of\n> code again (the form technically could be used, but it seems all our\n> uses of it are ones we could convert to the simpler one...).\n\nHaving a loose rule and inspecting its results carefully can work, but\nhaving such a rule published and run by CI pipelines etc. risks adding\ndefects and false positives.  I'm not sure it's much of a win to come\nup with a loose Coccinelle rule instead of grepping and hand-editing if\ngreat care needs to be taken in both cases.\n\n>\n> diff --git a/add-patch.c b/add-patch.c\n> index 509ca04456b..eff338d3901 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -915,10 +915,9 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,\n>  \tfile_diff->hunk_nr += splittable_into - 1;\n>  \tALLOC_GROW(file_diff->hunk, file_diff->hunk_nr, file_diff->hunk_alloc);\n>  \tif (hunk_index + splittable_into < file_diff->hunk_nr)\n> -\t\tmemmove(file_diff->hunk + hunk_index + splittable_into,\n> -\t\t\tfile_diff->hunk + hunk_index + 1,\n> -\t\t\t(file_diff->hunk_nr - hunk_index - splittable_into)\n> -\t\t\t* sizeof(*hunk));\n> +\t\tMOVE_ARRAY(file_diff->hunk + hunk_index + splittable_into,\n> +\t\t\t   file_diff->hunk + hunk_index + 1,\n> +\t\t\t   file_diff->hunk_nr - hunk_index - splittable_into);\n\nOK.\n\n>  \thunk = file_diff->hunk + hunk_index;\n>  \thunk->splittable_into = 1;\n>  \tmemset(hunk + 1, 0, (splittable_into - 1) * sizeof(*hunk));\n> diff --git a/builtin/mv.c b/builtin/mv.c\n> index 83a465ba831..f6187d1264a 100644\n> --- a/builtin/mv.c\n> +++ b/builtin/mv.c\n> @@ -282,14 +282,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n>  remove_entry:\n>  \t\tif (--argc > 0) {\n>  \t\t\tint n = argc - i;\n> -\t\t\tmemmove(source + i, source + i + 1,\n> -\t\t\t\tn * sizeof(char *));\n> -\t\t\tmemmove(destination + i, destination + i + 1,\n> -\t\t\t\tn * sizeof(char *));\n> -\t\t\tmemmove(modes + i, modes + i + 1,\n> -\t\t\t\tn * sizeof(enum update_mode));\n> -\t\t\tmemmove(submodule_gitfile + i, submodule_gitfile + i + 1,\n> -\t\t\t\tn * sizeof(char *));\n> +\t\t\tMOVE_ARRAY(source + i, source + i + 1, n);\n> +\t\t\tMOVE_ARRAY(destination + i, destination + i + 1, n);\n> +\t\t\tMOVE_ARRAY(modes + i, modes + i + 1, n);\n> +\t\t\tMOVE_ARRAY(submodule_gitfile + i,\n> +\t\t\t\t   submodule_gitfile + i + 1, n);\n\nA simpler approach could be to mark the entries as invalid somehow and\nskip them in the final loop instead of compacting the array like that.\nWe don't shrink the allocation anyway, so there's no need to move half\nthe entries on average on each such a delete.\n\n>  \t\t\ti--;\n>  \t\t}\n>  \t}\n> diff --git a/commit.c b/commit.c\n> index 1fb1b2ea90c..c23d3e3678f 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -151,10 +151,9 @@ int register_commit_graft(struct repository *r, struct commit_graft *graft,\n>  \t\t   r->parsed_objects->grafts_alloc);\n>  \tr->parsed_objects->grafts_nr++;\n>  \tif (pos < r->parsed_objects->grafts_nr)\n> -\t\tmemmove(r->parsed_objects->grafts + pos + 1,\n> -\t\t\tr->parsed_objects->grafts + pos,\n> -\t\t\t(r->parsed_objects->grafts_nr - pos - 1) *\n> -\t\t\tsizeof(*r->parsed_objects->grafts));\n> +\t\tMOVE_ARRAY(r->parsed_objects->grafts + pos + 1,\n> +\t\t\t   r->parsed_objects->grafts + pos,\n> +\t\t\t   r->parsed_objects->grafts_nr - pos - 1);\n\nIs that -1 correct?  Yes, it's needed because grafts_nr is incremented\nalready above, before making room and actually adding the the new item.\nConfused me for a minute.\n\n>  \tr->parsed_objects->grafts[pos] = graft;\n>  \tunparse_commit(r, &graft->oid);\n>  \treturn 0;\n> diff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci\n> index 9a4f00cb1bd..988ff9a3fda 100644\n> --- a/contrib/coccinelle/array.cocci\n> +++ b/contrib/coccinelle/array.cocci\n> @@ -73,6 +73,15 @@ expression n;\n>  + MOVE_ARRAY(dst, src, n);\n>  )\n>\n> +@@\n> +expression D;\n> +expression S;\n> +expression N;\n> +expression Z;\n> +@@\n> +- memmove(D, S, (N) * Z);\n> ++ MOVE_ARRAY(D, S, N);\n> +\n>  @@\n>  type T;\n>  T *ptr;\n"},{"id":"458592","messageId":"YscpDTgwxUko/aZM@coredump.intra.peff.net","threadId":"58115","inReplyTo":"xmqq8rp54r4l.fsf@gitster.g","subject":"Re: [PATCH] builtin/mv.c: use correct type to compute size of an array element","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-07T18:42:21Z","receivedAt":"2022-07-07T18:42:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 06, 2022 at 07:02:18PM -0700, Junio C Hamano wrote:\n\n>    * Should we in general use sizeof(TYPE) in these cases, instead\n>      of the size of the zeroth element, e.g.\n> \n>  \t\tmemmove(source + i, source + i + 1,\n> \t\t\tn * sizeof(source[i]));\n>     \n>      It would have been caught by the above Coccinelle rule (we are\n>      taking the size of *dst).\n\nI'm not sure I understand this. As you noted in a later email, using\nsizeof(TYPE) is less maintainable if the type of \"source\" changes. But\nlater you mention using \"*source\" instead of \"source[i]\". I don't think\nthere is a particular reason to prefer one over the other from the\ncompiler perspective. I find \"*source\" more idiomatic (but better still\nof course is MOVE_ARRAY, which removes the choice entirely).\n\n>    * Shouldn't we have an array of struct with four members, instead\n>      of four parallel arrays, e.g.\n> \n> \t\tstruct {\n> \t\t\tconst char *source;\n> \t\t\tconst char *destination;\n> \t\t\tenum update_mode mode;\n> \t\t\tconst char *submodule_gitfile;\n> \t\t} *mv_file;\n> \n>    The latter question is important to answer before we accept\n>    Coccinelle-suggested rewrite to use four MOVE_ARRAY() on these\n>    four parallel arrays on top of this fix.\n\nI think that would make the code a lot cleaner. But it looks like\n\"source\" and \"destination\" come from separate calls to\ninternal_prefix_pathspec().  So you'd have to reconcile that. And\nthere's some extra trickiness that sometimes \"destination\" comes from\n\"dest_path\", which _isn't_ always the same size as \"source\".\n\nSo I suspect the code which uses these arrays would be cleaner with a\nstruct, but the setup may get worse. :)\n\n-Peff\n"},{"id":"458596","messageId":"cb866b8c-dcc6-557f-da23-1c1972619a8a@web.de","threadId":"58115","inReplyTo":"xmqq1quw23r8.fsf@gitster.g","subject":"Re: [PATCH] builtin/mv.c: use correct type to compute size of an array element","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-07-07T19:11:28Z","receivedAt":"2022-07-07T19:11:49Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 07.07.22 um 20:10 schrieb Junio C Hamano:\n> While our eyes are on array.cocci, I have a few observations on it.\n>\n> This is not meant specifically to you, Ævar, but comments by those\n> more familiar with Coccinelle (and I am sure the bar to pass is\n> fairly low, as I am not all that familiar) are very much\n> appreciated.\n>\n>     @@\n>     expression dst, src, n, E;\n>     @@\n>       memcpy(dst, src, n * sizeof(\n>     - E[...]\n>     + *(E)\n>       ))\n>\n> This seems to force us prefer sizeof(*(E)) over sizeof(E[i]), when\n> it is used to compute the byte size of memcpy() operation.  There is\n> no reason to prefer one over the other, but I presume it is there\n> only for convenience for the other rules in this file (I recall\n> vaguely reading somewhere that these rules do not \"execute\" from top\n> to bottom, so I wonder how effective it is?).\n\nIt halves the number of syntax variants to deal with.\n\n>\n>     @@\n>     type T;\n>     T *ptr;\n>     T[] arr;\n>     expression E, n;\n>     @@\n>     (\n>       memcpy(ptr, E,\n>     - n * sizeof(*(ptr))\n>     + n * sizeof(T)\n>       )\n>     |\n>       memcpy(arr, E,\n>     - n * sizeof(*(arr))\n>     + n * sizeof(T)\n>       )\n>     |\n>       memcpy(E, ptr,\n>     - n * sizeof(*(ptr))\n>     + n * sizeof(T)\n>       )\n>     |\n>       memcpy(E, arr,\n>     - n * sizeof(*(arr))\n>     + n * sizeof(T)\n>       )\n>     )\n>\n> Likewise, but this one is a lot worse.\n>\n> Taken alone, sizeof(*(ptr)) is far more preferrable than sizeof(T),\n> because the code will be more maintainable.\n>\n>     Side Note.  I know builtin/mv.c had this type mismatch between\n>     the variable and sizeof() from the beginning when 11be42a4 (Make\n>     git-mv a builtin, 2006-07-26) introduced both the variable\n>     declaration \"const char **source\" and memmove() on it, but a\n>     code that starts out with \"char **src\" with memmove() that moves\n>     part of src[] and uses sizeof(char *) to compute the byte size\n>     of the move would become broken the same way when a later\n>     developer tightens the declaration to use \"const char **src\"\n>     without realizing that they have to update the type used in\n>     sizeof().\n>\n> So even though I am guessing that this is to allow the later rules\n> to worry only about sizeof(T), I am a bit unhappy to see the rule.\n> If an existing code matched this rule and get rewritten to use\n> sizeof(T), not sizeof(*(ptr)), but did not match the other rules to\n> be rewritten to use COPY_ARRAY(), the overall effect would be that\n> the automation made the code worse.\n\nTrue.\n\n>\n>     @@\n>     type T;\n>     T *dst_ptr;\n>     T *src_ptr;\n>     T[] dst_arr;\n>     T[] src_arr;\n>     expression n;\n>     @@\n>     (\n>     - memcpy(dst_ptr, src_ptr, (n) * sizeof(T))\n>     + COPY_ARRAY(dst_ptr, src_ptr, n)\n>     |\n>     - memcpy(dst_ptr, src_arr, (n) * sizeof(T))\n>     + COPY_ARRAY(dst_ptr, src_arr, n)\n>     |\n>     - memcpy(dst_arr, src_ptr, (n) * sizeof(T))\n>     + COPY_ARRAY(dst_arr, src_ptr, n)\n>     |\n>     - memcpy(dst_arr, src_arr, (n) * sizeof(T))\n>     + COPY_ARRAY(dst_arr, src_arr, n)\n>     )\n>\n> I take it that thanks to the earlier \"meh -- between sizeof(*p) and\n> sizeof(p[0]) there is no reason to prefer one over the other\" and\n> \"oh, no, we should prefer sizeof(*p) not sizeof(typeof(*p)) but this\n> one is the other way around\" rules, this one only has to deal with\n> sizeof(T).\n>\n> Am I reading it correctly?\n\nYes.  Without the ugly normalization step in the middle could either\nuse twelve cases instead of four here or use inline alternatives,\ne.g.:\n\ntype T;\nT *dst_ptr;\nT *src_ptr;\nT[] dst_arr;\nT[] src_arr;\nexpression n;\n@@\n(\n- memcpy(dst_ptr, src_ptr, (n) * \\( sizeof(*(dst_ptr)) \\| sizeof(*(src_ptr)) \\| sizeof(T) \\) )\n+ COPY_ARRAY(dst_ptr, src_ptr, n)\n|\n- memcpy(dst_ptr, src_arr, (n) * \\( sizeof(*(dst_ptr)) \\| sizeof(*(src_arr)) \\| sizeof(T) \\) )\n+ COPY_ARRAY(dst_ptr, src_arr, n)\n|\n- memcpy(dst_arr, src_ptr, (n) * \\( sizeof(*(dst_arr)) \\| sizeof(*(src_ptr)) \\| sizeof(T) \\) )\n+ COPY_ARRAY(dst_arr, src_ptr, n)\n|\n- memcpy(dst_arr, src_arr, (n) * \\( sizeof(*(dst_arr)) \\| sizeof(*(src_arr)) \\| sizeof(T) \\) )\n+ COPY_ARRAY(dst_arr, src_arr, n)\n)\n\nI seem to remember that rules like this missed some cases, but perhaps\nthat's no longer an issue with the latest Coccinelle version?\n\n>\n>     @@\n>     type T;\n>     T *dst;\n>     T *src;\n>     expression n;\n>     @@\n>     (\n>     - memmove(dst, src, (n) * sizeof(*dst));\n>     + MOVE_ARRAY(dst, src, n);\n>     |\n>     - memmove(dst, src, (n) * sizeof(*src));\n>     + MOVE_ARRAY(dst, src, n);\n>     |\n>     - memmove(dst, src, (n) * sizeof(T));\n>     + MOVE_ARRAY(dst, src, n);\n>     )\n>\n> What I find interesting is that this one seems to be able to do the\n> necessary rewrite without having to do the \"turn everything into\n> sizeof(T) first\" trick.  If this approach works well, I'd rather see\n> the COPY_ARRAY() done without the first two preliminary rewrite\n> rules.\n\nIt doesn't support arrays (T[]).  That doesn't matter in practice\nbecause we don't have such cases (yet?).\n\n>\n> I wonder if the pattern in the first rule catches sizeof(dst[0])\n> instead of sizeof(*dst), though.\n\nIt doesn't.\n\n>\n>     @@\n>     type T;\n>     T *ptr;\n>     expression n;\n>     @@\n>     - ptr = xmalloc((n) * sizeof(*ptr));\n>     + ALLOC_ARRAY(ptr, n);\n>\n>     @@\n>     type T;\n>     T *ptr;\n>     expression n;\n>     @@\n>     - ptr = xmalloc((n) * sizeof(T));\n>     + ALLOC_ARRAY(ptr, n);\n>\n> Is it a no-op rewrite if we replace the above two rules with\n> something like:\n>\n> .   @@\n> .   type T;\n> .   T *ptr;\n> .   expression n;\n> .   @@\n> .   (\n> .   - ptr = xmalloc((n) * sizeof(*ptr));\n> .   + ALLOC_ARRAY(ptr, n);\n> .   |\n> .   - ptr = xmalloc((n) * sizeof(T));\n> .   + ALLOC_ARRAY(ptr, n);\n> .   )\n\nI think so.\n\n>\n> or even following the pattern of the next one ...\n>\n> .   @@\n> .   type T;\n> .   T *ptr;\n> .   expression n;\n> .   @@\n> .   - ptr = xmalloc((n) * \\( sizeof(*ptr) \\| sizeof(T) \\))\n> .   + ALLOC_ARRAY(ptr, n);\n>\n> ... I have to wonder?  I like the simplicity of this pattern.\n\nIn theory this is equivalent.\n\n>\n>     @@\n>     type T;\n>     T *ptr;\n>     expression n != 1;\n>     @@\n>     - ptr = xcalloc(n, \\( sizeof(*ptr) \\| sizeof(T) \\) )\n>     + CALLOC_ARRAY(ptr, n)\n>\n> And this leaves xcalloc(1, ...) alone because it is a way to get a\n> cleared block of memory that may not be an array at all.  Shouldn't\n> we have \"n != 1\" for xmalloc rule as well, I wonder, if only for\n> consistency?\n\nI agree that a single-element array is a bit awkward, so allowing the\nexplicit sizeof in that case is less iffy.  ALLOC/CALLOC macros for\nsingle items might make that automation more palatable..\n\nRené\n"},{"id":"458601","messageId":"xmqqfsjczn42.fsf@gitster.g","threadId":"58115","inReplyTo":"YscpDTgwxUko/aZM@coredump.intra.peff.net","subject":"Re: [PATCH] builtin/mv.c: use correct type to compute size of an array element","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-07T20:25:33Z","receivedAt":"2022-07-07T20:25:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Jul 06, 2022 at 07:02:18PM -0700, Junio C Hamano wrote:\n>\n>>    * Should we in general use sizeof(TYPE) in these cases, instead\n>>      of the size of the zeroth element, e.g.\n>> \n>>  \t\tmemmove(source + i, source + i + 1,\n>> \t\t\tn * sizeof(source[i]));\n>>     \n>>      It would have been caught by the above Coccinelle rule (we are\n>>      taking the size of *dst).\n>\n> I'm not sure I understand this. As you noted in a later email, using\n> sizeof(TYPE) is less maintainable if the type of \"source\" changes.\n\nSorry for a typo or thinko, whichever one you like ;-)\n\n> But\n> later you mention using \"*source\" instead of \"source[i]\". I don't think\n> there is a particular reason to prefer one over the other from the\n> compiler perspective. I find \"*source\" more idiomatic (but better still\n> of course is MOVE_ARRAY, which removes the choice entirely).\n\nYes, I wrote source[i] there only because I found it somewhat\nawkward to write source[0] or *source there, when the moved\n(sub)array is from the index 'i' to the end.  *(source + i) would\nhave matched the intention better but it still is awkward.\n"},{"id":"458681","messageId":"95432eb4-e66a-5c04-9267-f71391fbe277@web.de","threadId":"58115","inReplyTo":"cb866b8c-dcc6-557f-da23-1c1972619a8a@web.de","subject":"Re: [PATCH] builtin/mv.c: use correct type to compute size of an array element","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-07-09T08:16:28Z","receivedAt":"2022-07-09T08:16:48Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 07.07.22 um 21:11 schrieb René Scharfe:\n> Am 07.07.22 um 20:10 schrieb Junio C Hamano:\n>>     @@\n>>     type T;\n>>     T *dst_ptr;\n>>     T *src_ptr;\n>>     T[] dst_arr;\n>>     T[] src_arr;\n>>     expression n;\n>>     @@\n>>     (\n>>     - memcpy(dst_ptr, src_ptr, (n) * sizeof(T))\n>>     + COPY_ARRAY(dst_ptr, src_ptr, n)\n>>     |\n>>     - memcpy(dst_ptr, src_arr, (n) * sizeof(T))\n>>     + COPY_ARRAY(dst_ptr, src_arr, n)\n>>     |\n>>     - memcpy(dst_arr, src_ptr, (n) * sizeof(T))\n>>     + COPY_ARRAY(dst_arr, src_ptr, n)\n>>     |\n>>     - memcpy(dst_arr, src_arr, (n) * sizeof(T))\n>>     + COPY_ARRAY(dst_arr, src_arr, n)\n>>     )\n>>\n>> I take it that thanks to the earlier \"meh -- between sizeof(*p) and\n>> sizeof(p[0]) there is no reason to prefer one over the other\" and\n>> \"oh, no, we should prefer sizeof(*p) not sizeof(typeof(*p)) but this\n>> one is the other way around\" rules, this one only has to deal with\n>> sizeof(T).\n>>\n>> Am I reading it correctly?\n>\n> Yes.  Without the ugly normalization step in the middle could either\n> use twelve cases instead of four here or use inline alternatives,\n> e.g.:\n>\n> type T;\n> T *dst_ptr;\n> T *src_ptr;\n> T[] dst_arr;\n> T[] src_arr;\n> expression n;\n> @@\n> (\n> - memcpy(dst_ptr, src_ptr, (n) * \\( sizeof(*(dst_ptr)) \\| sizeof(*(src_ptr)) \\| sizeof(T) \\) )\n> + COPY_ARRAY(dst_ptr, src_ptr, n)\n> |\n> - memcpy(dst_ptr, src_arr, (n) * \\( sizeof(*(dst_ptr)) \\| sizeof(*(src_arr)) \\| sizeof(T) \\) )\n> + COPY_ARRAY(dst_ptr, src_arr, n)\n> |\n> - memcpy(dst_arr, src_ptr, (n) * \\( sizeof(*(dst_arr)) \\| sizeof(*(src_ptr)) \\| sizeof(T) \\) )\n> + COPY_ARRAY(dst_arr, src_ptr, n)\n> |\n> - memcpy(dst_arr, src_arr, (n) * \\( sizeof(*(dst_arr)) \\| sizeof(*(src_arr)) \\| sizeof(T) \\) )\n> + COPY_ARRAY(dst_arr, src_arr, n)\n> )\n>\n> I seem to remember that rules like this missed some cases, but perhaps\n> that's no longer an issue with the latest Coccinelle version?\n\nNot a problem, it seems; at least Coccinelle 1.1.1 is still able to\nrecreate the conversions from 45ccef87b3 (use COPY_ARRAY, 2016-09-25)\nand 921d49be86 (use COPY_ARRAY for copying arrays, 2019-06-15) with the\npatch below, which removes the normalization rules.  It increases the\nprocessing time for array.cocci from 53s to 66s for me, though.  Worth\nthe increased precision and clarity?\n\nRené\n\n---\n contrib/coccinelle/array.cocci | 82 +++++++++++++++++-----------------\n 1 file changed, 40 insertions(+), 42 deletions(-)\n\ndiff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci\nindex 9a4f00cb1b..aa75937950 100644\n--- a/contrib/coccinelle/array.cocci\n+++ b/contrib/coccinelle/array.cocci\n@@ -1,60 +1,58 @@\n @@\n-expression dst, src, n, E;\n+type T;\n+T *dst_ptr;\n+T *src_ptr;\n+expression n;\n @@\n-  memcpy(dst, src, n * sizeof(\n-- E[...]\n-+ *(E)\n-  ))\n+- memcpy(dst_ptr, src_ptr, (n) * \\( sizeof(T)\n+-                                \\| sizeof(*(dst_ptr))\n+-                                \\| sizeof(*(src_ptr))\n+-                                \\| sizeof(dst_ptr[...])\n+-                                \\| sizeof(src_ptr[...])\n+-                                \\) )\n++ COPY_ARRAY(dst_ptr, src_ptr, n)\n\n @@\n type T;\n-T *ptr;\n-T[] arr;\n-expression E, n;\n+T *dst_ptr;\n+T[] src_arr;\n+expression n;\n @@\n-(\n-  memcpy(ptr, E,\n-- n * sizeof(*(ptr))\n-+ n * sizeof(T)\n-  )\n-|\n-  memcpy(arr, E,\n-- n * sizeof(*(arr))\n-+ n * sizeof(T)\n-  )\n-|\n-  memcpy(E, ptr,\n-- n * sizeof(*(ptr))\n-+ n * sizeof(T)\n-  )\n-|\n-  memcpy(E, arr,\n-- n * sizeof(*(arr))\n-+ n * sizeof(T)\n-  )\n-)\n+- memcpy(dst_ptr, src_arr, (n) * \\( sizeof(T)\n+-                                \\| sizeof(*(dst_ptr))\n+-                                \\| sizeof(*(src_arr))\n+-                                \\| sizeof(dst_ptr[...])\n+-                                \\| sizeof(src_arr[...])\n+-                                \\) )\n++ COPY_ARRAY(dst_ptr, src_arr, n)\n\n @@\n type T;\n-T *dst_ptr;\n+T[] dst_arr;\n T *src_ptr;\n+expression n;\n+@@\n+- memcpy(dst_arr, src_ptr, (n) * \\( sizeof(T)\n+-                                \\| sizeof(*(dst_arr))\n+-                                \\| sizeof(*(src_ptr))\n+-                                \\| sizeof(dst_arr[...])\n+-                                \\| sizeof(src_ptr[...])\n+-                                \\) )\n++ COPY_ARRAY(dst_arr, src_ptr, n)\n+\n+@@\n+type T;\n T[] dst_arr;\n T[] src_arr;\n expression n;\n @@\n-(\n-- memcpy(dst_ptr, src_ptr, (n) * sizeof(T))\n-+ COPY_ARRAY(dst_ptr, src_ptr, n)\n-|\n-- memcpy(dst_ptr, src_arr, (n) * sizeof(T))\n-+ COPY_ARRAY(dst_ptr, src_arr, n)\n-|\n-- memcpy(dst_arr, src_ptr, (n) * sizeof(T))\n-+ COPY_ARRAY(dst_arr, src_ptr, n)\n-|\n-- memcpy(dst_arr, src_arr, (n) * sizeof(T))\n+- memcpy(dst_arr, src_arr, (n) * \\( sizeof(T)\n+-                                \\| sizeof(*(dst_arr))\n+-                                \\| sizeof(*(src_arr))\n+-                                \\| sizeof(dst_arr[...])\n+-                                \\| sizeof(src_arr[...])\n+-                                \\) )\n + COPY_ARRAY(dst_arr, src_arr, n)\n-)\n\n @@\n type T;\n--\n2.37.0\n"},{"id":"458698","messageId":"xmqq4jzpu4xp.fsf_-_@gitster.g","threadId":"58115","inReplyTo":"xmqqy1x531vp.fsf@gitster.g","subject":"[PATCH v3] builtin/mv.c: use the MOVE_ARRAY() macro instead of memmove()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-10T01:33:54Z","receivedAt":"2022-07-10T01:34:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The variables 'source', 'destination', and 'submodule_gitfile' are\nall of type \"const char **\", and an element of such an array is of\n\"type const char *\", but these memmove() calls were written as if\nthese variables are of type \"char **\".\n\nOnce these memmove() calls are fixed to use the correct type to\ncompute the number of bytes to be moved, e.g.\n\n-      memmove(source + i, source + i + 1, n * sizeof(char *));\n+      memmove(source + i, source + i + 1, n * sizeof(const char *));\n\nexisting contrib/coccinelle/array.cocci rules can recognize them as\ncandidates for turning into MOVE_ARRAY().\n\nWhile at it, use CALLOC_ARRAY() instead of xcalloc() to allocate the\nmodes[] array that is involved in the change.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * The first hunk is new; with this additional rewrite from\n   xcalloc() to CALLOC_ARRAY(), the static-analysis job on 'seen'\n   starts passing (cf. https://github.com/git/git/runs/7267146111)\n\n   I think this is now good enough to merge down to 'next' and to\n   'master'.\n\n builtin/mv.c | 16 +++++++---------\n 1 file changed, 7 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex 83a465ba83..859fa5023f 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -148,7 +148,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"index file corrupt\"));\n \n \tsource = internal_prefix_pathspec(prefix, argv, argc, 0);\n-\tmodes = xcalloc(argc, sizeof(enum update_mode));\n+\tCALLOC_ARRAY(modes, argc);\n+\n \t/*\n \t * Keep trailing slash, needed to let\n \t * \"git mv file no-such-dir/\" error out, except in the case\n@@ -282,14 +283,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n remove_entry:\n \t\tif (--argc > 0) {\n \t\t\tint n = argc - i;\n-\t\t\tmemmove(source + i, source + i + 1,\n-\t\t\t\tn * sizeof(char *));\n-\t\t\tmemmove(destination + i, destination + i + 1,\n-\t\t\t\tn * sizeof(char *));\n-\t\t\tmemmove(modes + i, modes + i + 1,\n-\t\t\t\tn * sizeof(enum update_mode));\n-\t\t\tmemmove(submodule_gitfile + i, submodule_gitfile + i + 1,\n-\t\t\t\tn * sizeof(char *));\n+\t\t\tMOVE_ARRAY(source + i, source + i + 1, n);\n+\t\t\tMOVE_ARRAY(destination + i, destination + i + 1, n);\n+\t\t\tMOVE_ARRAY(modes + i, modes + i + 1, n);\n+\t\t\tMOVE_ARRAY(submodule_gitfile + i,\n+\t\t\t\t   submodule_gitfile + i + 1, n);\n \t\t\ti--;\n \t\t}\n \t}\n-- \n2.37.0-238-g7905cdbf0e\n\n"},{"id":"458700","messageId":"xmqqmtdhsf1z.fsf@gitster.g","threadId":"58115","inReplyTo":"95432eb4-e66a-5c04-9267-f71391fbe277@web.de","subject":"Re: [PATCH] builtin/mv.c: use correct type to compute size of an array element","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-10T05:38:16Z","receivedAt":"2022-07-10T05:38:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Not a problem, it seems; at least Coccinelle 1.1.1 is still able to\n> recreate the conversions from 45ccef87b3 (use COPY_ARRAY, 2016-09-25)\n> and 921d49be86 (use COPY_ARRAY for copying arrays, 2019-06-15) with the\n> patch below, which removes the normalization rules.  \n\nThe result certainly is cleaner and also looks much less error\nprone.\n\n> It increases the\n> processing time for array.cocci from 53s to 66s for me, though.  Worth\n> the increased precision and clarity?\n\nI would say so.  For manual tests that humans stare at their\nprogress waiting for their completion, every second may matter, but\na check that makes us wait for more than 30 seconds *and* forces us\nto be extra careful when vetting its validity is worse than a check\nthat takes 10 more seconds with much less risk of broken output.\n\n"},{"id":"458706","messageId":"ded153d4-4aea-d4da-11cb-ec66d181e4c9@web.de","threadId":"58115","inReplyTo":"xmqqmtdhsf1z.fsf@gitster.g","subject":"[PATCH] cocci: avoid normalization rules for memcpy","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-07-10T10:05:51Z","receivedAt":"2022-07-10T10:06:13Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Some of the rules for using COPY_ARRAY instead of memcpy with sizeof are\nintended to reduce the number of sizeof variants to deal with.  They can\nhave unintended side effects if only they match, but not the one for the\nCOPY_ARRAY conversion at the end.\n\nAvoid these side effects by instead using a self-contained rule for each\ncombination of array and pointer for source and destination which lists\nall sizeof variants inline.\n\nThis lets \"make contrib/coccinelle/array.cocci.patch\" take 15% longer on\nmy machine, but gives peace of mind that no incomplete transformation\nwill be generated.\n\nSuggested-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nPatch generated with --patience for easier reading.\n\n contrib/coccinelle/array.cocci | 92 +++++++++++++++++-----------------\n 1 file changed, 45 insertions(+), 47 deletions(-)\n\ndiff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci\nindex 9a4f00cb1b..aa75937950 100644\n--- a/contrib/coccinelle/array.cocci\n+++ b/contrib/coccinelle/array.cocci\n@@ -1,60 +1,58 @@\n-@@\n-expression dst, src, n, E;\n-@@\n-  memcpy(dst, src, n * sizeof(\n-- E[...]\n-+ *(E)\n-  ))\n-\n-@@\n-type T;\n-T *ptr;\n-T[] arr;\n-expression E, n;\n-@@\n-(\n-  memcpy(ptr, E,\n-- n * sizeof(*(ptr))\n-+ n * sizeof(T)\n-  )\n-|\n-  memcpy(arr, E,\n-- n * sizeof(*(arr))\n-+ n * sizeof(T)\n-  )\n-|\n-  memcpy(E, ptr,\n-- n * sizeof(*(ptr))\n-+ n * sizeof(T)\n-  )\n-|\n-  memcpy(E, arr,\n-- n * sizeof(*(arr))\n-+ n * sizeof(T)\n-  )\n-)\n-\n @@\n type T;\n T *dst_ptr;\n T *src_ptr;\n-T[] dst_arr;\n-T[] src_arr;\n expression n;\n @@\n-(\n-- memcpy(dst_ptr, src_ptr, (n) * sizeof(T))\n+- memcpy(dst_ptr, src_ptr, (n) * \\( sizeof(T)\n+-                                \\| sizeof(*(dst_ptr))\n+-                                \\| sizeof(*(src_ptr))\n+-                                \\| sizeof(dst_ptr[...])\n+-                                \\| sizeof(src_ptr[...])\n+-                                \\) )\n + COPY_ARRAY(dst_ptr, src_ptr, n)\n-|\n-- memcpy(dst_ptr, src_arr, (n) * sizeof(T))\n+\n+@@\n+type T;\n+T *dst_ptr;\n+T[] src_arr;\n+expression n;\n+@@\n+- memcpy(dst_ptr, src_arr, (n) * \\( sizeof(T)\n+-                                \\| sizeof(*(dst_ptr))\n+-                                \\| sizeof(*(src_arr))\n+-                                \\| sizeof(dst_ptr[...])\n+-                                \\| sizeof(src_arr[...])\n+-                                \\) )\n + COPY_ARRAY(dst_ptr, src_arr, n)\n-|\n-- memcpy(dst_arr, src_ptr, (n) * sizeof(T))\n+\n+@@\n+type T;\n+T[] dst_arr;\n+T *src_ptr;\n+expression n;\n+@@\n+- memcpy(dst_arr, src_ptr, (n) * \\( sizeof(T)\n+-                                \\| sizeof(*(dst_arr))\n+-                                \\| sizeof(*(src_ptr))\n+-                                \\| sizeof(dst_arr[...])\n+-                                \\| sizeof(src_ptr[...])\n+-                                \\) )\n + COPY_ARRAY(dst_arr, src_ptr, n)\n-|\n-- memcpy(dst_arr, src_arr, (n) * sizeof(T))\n+\n+@@\n+type T;\n+T[] dst_arr;\n+T[] src_arr;\n+expression n;\n+@@\n+- memcpy(dst_arr, src_arr, (n) * \\( sizeof(T)\n+-                                \\| sizeof(*(dst_arr))\n+-                                \\| sizeof(*(src_arr))\n+-                                \\| sizeof(dst_arr[...])\n+-                                \\| sizeof(src_arr[...])\n+-                                \\) )\n + COPY_ARRAY(dst_arr, src_arr, n)\n-)\n\n @@\n type T;\n--\n2.37.0\n"},{"id":"458714","messageId":"220710.86ilo580mb.gmgdl@evledraar.gmail.com","threadId":"58115","inReplyTo":"ded153d4-4aea-d4da-11cb-ec66d181e4c9@web.de","subject":"Re: [PATCH] cocci: avoid normalization rules for memcpy","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-10T14:45:35Z","receivedAt":"2022-07-10T15:10:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Jul 10 2022, René Scharfe wrote:\n\n> Some of the rules for using COPY_ARRAY instead of memcpy with sizeof are\n> intended to reduce the number of sizeof variants to deal with.  They can\n> have unintended side effects if only they match, but not the one for the\n> COPY_ARRAY conversion at the end.\n\nSince ab/cocci-unused is marked for \"next\" it would be really nice to\nhave this based on top so we can add tests for these transformations\n(the topic adds a way to do that).\n\nBut if you don't feel like  doing that this is fine too.\n\n> diff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci\n> index 9a4f00cb1b..aa75937950 100644\n> --- a/contrib/coccinelle/array.cocci\n> +++ b/contrib/coccinelle/array.cocci\n> @@ -1,60 +1,58 @@\n> -@@\n> -expression dst, src, n, E;\n> -@@\n> -  memcpy(dst, src, n * sizeof(\n> -- E[...]\n> -+ *(E)\n> -  ))\n> -\n> -@@\n> -type T;\n> -T *ptr;\n> -T[] arr;\n> -expression E, n;\n> -@@\n> -(\n> -  memcpy(ptr, E,\n> -- n * sizeof(*(ptr))\n> -+ n * sizeof(T)\n> -  )\n> -|\n> -  memcpy(arr, E,\n> -- n * sizeof(*(arr))\n> -+ n * sizeof(T)\n> -  )\n> -|\n> -  memcpy(E, ptr,\n> -- n * sizeof(*(ptr))\n> -+ n * sizeof(T)\n> -  )\n> -|\n> -  memcpy(E, arr,\n> -- n * sizeof(*(arr))\n> -+ n * sizeof(T)\n> -  )\n> -)\n> -\n>  @@\n>  type T;\n>  T *dst_ptr;\n>  T *src_ptr;\n> -T[] dst_arr;\n> -T[] src_arr;\n>  expression n;\n>  @@\n> -(\n> -- memcpy(dst_ptr, src_ptr, (n) * sizeof(T))\n> +- memcpy(dst_ptr, src_ptr, (n) * \\( sizeof(T)\n> +-                                \\| sizeof(*(dst_ptr))\n> +-                                \\| sizeof(*(src_ptr))\n> +-                                \\| sizeof(dst_ptr[...])\n> +-                                \\| sizeof(src_ptr[...])\n> +-                                \\) )\n>  + COPY_ARRAY(dst_ptr, src_ptr, n)\n> -|\n> -- memcpy(dst_ptr, src_arr, (n) * sizeof(T))\n> +\n> +@@\n> +type T;\n> +T *dst_ptr;\n> +T[] src_arr;\n> +expression n;\n> +@@\n> +- memcpy(dst_ptr, src_arr, (n) * \\( sizeof(T)\n> +-                                \\| sizeof(*(dst_ptr))\n> +-                                \\| sizeof(*(src_arr))\n> +-                                \\| sizeof(dst_ptr[...])\n> +-                                \\| sizeof(src_arr[...])\n> +-                                \\) )\n>  + COPY_ARRAY(dst_ptr, src_arr, n)\n> -|\n> -- memcpy(dst_arr, src_ptr, (n) * sizeof(T))\n> +\n> +@@\n> +type T;\n> +T[] dst_arr;\n> +T *src_ptr;\n> +expression n;\n> +@@\n> +- memcpy(dst_arr, src_ptr, (n) * \\( sizeof(T)\n> +-                                \\| sizeof(*(dst_arr))\n> +-                                \\| sizeof(*(src_ptr))\n> +-                                \\| sizeof(dst_arr[...])\n> +-                                \\| sizeof(src_ptr[...])\n> +-                                \\) )\n>  + COPY_ARRAY(dst_arr, src_ptr, n)\n> -|\n> -- memcpy(dst_arr, src_arr, (n) * sizeof(T))\n> +\n> +@@\n> +type T;\n> +T[] dst_arr;\n> +T[] src_arr;\n> +expression n;\n> +@@\n> +- memcpy(dst_arr, src_arr, (n) * \\( sizeof(T)\n> +-                                \\| sizeof(*(dst_arr))\n> +-                                \\| sizeof(*(src_arr))\n> +-                                \\| sizeof(dst_arr[...])\n> +-                                \\| sizeof(src_arr[...])\n> +-                                \\) )\n>  + COPY_ARRAY(dst_arr, src_arr, n)\n> -)\n>\n>  @@\n>  type T;\n\nHrm, this seems like a lot of repetition, it's here in the rules you're\nediting already, but these repeated \"sizeof\" make it a lot more verbose.\n\nIsn't there a way to avoid this by simply wrapping this across lines, I\ndidn't test, but I think you can do this sort of thing in the cocci\ngrammar:\n\n- memcpy(\n- COPY_ARRAY(\n  (\n  dst_arr\n  |\n  dst_ptr\n  )\n  ,\n  (\n  src_arr\n  |\n  src_ptr\n  )\n  ,\n  (n) *\n-  [your big sizeof alternate here]\n  )\n\nI.e. you want to preserve whatever we match in the 1st and 2nd\narguments, but only want to munge part of the 3rd argument. The cocci\ngrammar can \"reach into\" lines like that, it doesn't need to be limited\nto line-based diffs.\n\nBut I didn't try it in this caes, and maybe there's a good reason for\nwhy it can't happen in this case...\n\nI also wonder if that won't be a lot faster, i.e. if you can condense\nthis all into one rule it won't need to match this N times, but maybe\nthe overall complexity of the rules makes it come out to the same thing\nin the end...\n"},{"id":"458720","messageId":"220710.86a69g9b3e.gmgdl@evledraar.gmail.com","threadId":"58115","inReplyTo":"220710.86ilo580mb.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] cocci: avoid normalization rules for memcpy","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-10T16:32:07Z","receivedAt":"2022-07-10T16:38:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Jul 10 2022, Ævar Arnfjörð Bjarmason wrote:\n\n> On Sun, Jul 10 2022, René Scharfe wrote:\n\n> Isn't there a way to avoid this by simply wrapping this across lines, I\n> didn't test, but I think you can do this sort of thing in the cocci\n> grammar:\n>\n> - memcpy(\n> - COPY_ARRAY(\n\nOops, typo!\n\nI just manually typed this out, so I meant \"+ COPY_ARRAY(\" here, in case\nit wasn't clear.\n\nTo elaborate: Here's an example in the cocci repository:\nhttps://github.com/coccinelle/coccinelle/blob/master/tests/pcim.cocci\n\nYou can also use \"...\" elides, but I didn't check if that works for the\nstart of an argument list (but it should, I think), here's an example\nwith it at the end:\nhttps://github.com/coccinelle/coccinelle/blob/master/scripts/coccicheck/cocci/kc.cocci\n\nSo I think rules like:\n\n- memcpy(\n+ COPY_ARRAY(\n  ...,\n- A\n+ B\n  );\n\nAre possible, and something you can use in this case (I have \"5.1 Basic\nTransformations\" from cocci_spatch_grammar.pdf in front of me).\n"},{"id":"458726","messageId":"xmqqr12srcj8.fsf@gitster.g","threadId":"58115","inReplyTo":"220710.86ilo580mb.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] cocci: avoid normalization rules for memcpy","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-10T19:30:19Z","receivedAt":"2022-07-10T19:30:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> Since ab/cocci-unused is marked for \"next\" it would be really nice to\n> have this based on top so we can add tests for these transformations\n> (the topic adds a way to do that).\n\nPlease do not follow this mantra in general.\n\nWhat is already in 'next' cannot be avoided.  What is not yet in\n'next' can yield just fine if needed.\n"},{"id":"458805","messageId":"e29e424f-c214-a912-fba8-107c5e402b8a@web.de","threadId":"58115","inReplyTo":"220710.86ilo580mb.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] cocci: avoid normalization rules for memcpy","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-07-11T17:11:18Z","receivedAt":"2022-07-11T17:11:44Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 10.07.22 um 16:45 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Sun, Jul 10 2022, René Scharfe wrote:\n>\n>> Some of the rules for using COPY_ARRAY instead of memcpy with sizeof are\n>> intended to reduce the number of sizeof variants to deal with.  They can\n>> have unintended side effects if only they match, but not the one for the\n>> COPY_ARRAY conversion at the end.\n>\n> Since ab/cocci-unused is marked for \"next\" it would be really nice to\n> have this based on top so we can add tests for these transformations\n> (the topic adds a way to do that).\n\nTesting semantic patches sounds like a good idea.  We can add tests later,\nonce that feature landed.\n\n>\n> But if you don't feel like  doing that this is fine too.\n>\n>> diff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci\n>> index 9a4f00cb1b..aa75937950 100644\n>> --- a/contrib/coccinelle/array.cocci\n>> +++ b/contrib/coccinelle/array.cocci\n>> @@ -1,60 +1,58 @@\n>> -@@\n>> -expression dst, src, n, E;\n>> -@@\n>> -  memcpy(dst, src, n * sizeof(\n>> -- E[...]\n>> -+ *(E)\n>> -  ))\n>> -\n>> -@@\n>> -type T;\n>> -T *ptr;\n>> -T[] arr;\n>> -expression E, n;\n>> -@@\n>> -(\n>> -  memcpy(ptr, E,\n>> -- n * sizeof(*(ptr))\n>> -+ n * sizeof(T)\n>> -  )\n>> -|\n>> -  memcpy(arr, E,\n>> -- n * sizeof(*(arr))\n>> -+ n * sizeof(T)\n>> -  )\n>> -|\n>> -  memcpy(E, ptr,\n>> -- n * sizeof(*(ptr))\n>> -+ n * sizeof(T)\n>> -  )\n>> -|\n>> -  memcpy(E, arr,\n>> -- n * sizeof(*(arr))\n>> -+ n * sizeof(T)\n>> -  )\n>> -)\n>> -\n>>  @@\n>>  type T;\n>>  T *dst_ptr;\n>>  T *src_ptr;\n>> -T[] dst_arr;\n>> -T[] src_arr;\n>>  expression n;\n>>  @@\n>> -(\n>> -- memcpy(dst_ptr, src_ptr, (n) * sizeof(T))\n>> +- memcpy(dst_ptr, src_ptr, (n) * \\( sizeof(T)\n>> +-                                \\| sizeof(*(dst_ptr))\n>> +-                                \\| sizeof(*(src_ptr))\n>> +-                                \\| sizeof(dst_ptr[...])\n>> +-                                \\| sizeof(src_ptr[...])\n>> +-                                \\) )\n>>  + COPY_ARRAY(dst_ptr, src_ptr, n)\n>> -|\n>> -- memcpy(dst_ptr, src_arr, (n) * sizeof(T))\n>> +\n>> +@@\n>> +type T;\n>> +T *dst_ptr;\n>> +T[] src_arr;\n>> +expression n;\n>> +@@\n>> +- memcpy(dst_ptr, src_arr, (n) * \\( sizeof(T)\n>> +-                                \\| sizeof(*(dst_ptr))\n>> +-                                \\| sizeof(*(src_arr))\n>> +-                                \\| sizeof(dst_ptr[...])\n>> +-                                \\| sizeof(src_arr[...])\n>> +-                                \\) )\n>>  + COPY_ARRAY(dst_ptr, src_arr, n)\n>> -|\n>> -- memcpy(dst_arr, src_ptr, (n) * sizeof(T))\n>> +\n>> +@@\n>> +type T;\n>> +T[] dst_arr;\n>> +T *src_ptr;\n>> +expression n;\n>> +@@\n>> +- memcpy(dst_arr, src_ptr, (n) * \\( sizeof(T)\n>> +-                                \\| sizeof(*(dst_arr))\n>> +-                                \\| sizeof(*(src_ptr))\n>> +-                                \\| sizeof(dst_arr[...])\n>> +-                                \\| sizeof(src_ptr[...])\n>> +-                                \\) )\n>>  + COPY_ARRAY(dst_arr, src_ptr, n)\n>> -|\n>> -- memcpy(dst_arr, src_arr, (n) * sizeof(T))\n>> +\n>> +@@\n>> +type T;\n>> +T[] dst_arr;\n>> +T[] src_arr;\n>> +expression n;\n>> +@@\n>> +- memcpy(dst_arr, src_arr, (n) * \\( sizeof(T)\n>> +-                                \\| sizeof(*(dst_arr))\n>> +-                                \\| sizeof(*(src_arr))\n>> +-                                \\| sizeof(dst_arr[...])\n>> +-                                \\| sizeof(src_arr[...])\n>> +-                                \\) )\n>>  + COPY_ARRAY(dst_arr, src_arr, n)\n>> -)\n>>\n>>  @@\n>>  type T;\n>\n> Hrm, this seems like a lot of repetition, it's here in the rules you're\n> editing already, but these repeated \"sizeof\" make it a lot more verbose.\n>\n> Isn't there a way to avoid this by simply wrapping this across lines, I\n> didn't test, but I think you can do this sort of thing in the cocci\n> grammar:\n>\n> - memcpy(\n> - COPY_ARRAY(\n>   (\n>   dst_arr\n>   |\n>   dst_ptr\n>   )\n>   ,\n>   (\n>   src_arr\n>   |\n>   src_ptr\n>   )\n>   ,\n>   (n) *\n> -  [your big sizeof alternate here]\n>   )\n\nHmm, that would match many more combinations, e.g. this one:\n\nvoid f(int *a, int *b, long n, int c[1]) {\n\tmemcpy(a, b, n * sizeof(*c));\n}\n\nThe elements of a, b and c have the same type, so replacing c with a\n(which a conversion to COPY_ARRAY would do) would produce the same\nobject code.  I can't come up with a plausible scenario like above and\nwhere a type change of c down the line would cause problems, but I\nalso can't convince myself that no such thing can exist.  Tricky.\n\nAnd I can't get it to format the whitespace around the third argument\nof COPY_ARRAY nicely in all cases.\n\nAnd it takes 37% longer on my machine.\n\nBut it sure is more compact. :)\n\n@@\ntype T;\nT *dst_ptr;\nT *src_ptr;\nT[] dst_arr;\nT[] src_arr;\nexpression n;\n@@\n- memcpy\n+ COPY_ARRAY\n  (\n  \\( dst_ptr \\| dst_arr \\) ,\n  \\( src_ptr \\| src_arr \\) ,\n- (n) *  \\( sizeof(T)\n-        \\| sizeof(*(dst_ptr))\n-        \\| sizeof(*(dst_arr))\n-        \\| sizeof(*(src_ptr))\n-        \\| sizeof(*(src_arr))\n-        \\| sizeof(dst_ptr[...])\n-        \\| sizeof(dst_arr[...])\n-        \\| sizeof(src_ptr[...])\n-        \\| sizeof(src_arr[...])\n-        \\)\n+ n\n  )\n\n>\n> I.e. you want to preserve whatever we match in the 1st and 2nd\n> arguments, but only want to munge part of the 3rd argument. The cocci\n> grammar can \"reach into\" lines like that, it doesn't need to be limited\n> to line-based diffs.\n>\n> But I didn't try it in this caes, and maybe there's a good reason for\n> why it can't happen in this case...\n>\n> I also wonder if that won't be a lot faster, i.e. if you can condense\n> this all into one rule it won't need to match this N times, but maybe\n> the overall complexity of the rules makes it come out to the same thing\n> in the end...\n"},{"id":"458815","messageId":"220711.86y1wz5s7u.gmgdl@evledraar.gmail.com","threadId":"58115","inReplyTo":"e29e424f-c214-a912-fba8-107c5e402b8a@web.de","subject":"Re: [PATCH] cocci: avoid normalization rules for memcpy","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-11T20:05:25Z","receivedAt":"2022-07-11T20:07:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jul 11 2022, René Scharfe wrote:\n\n> Am 10.07.22 um 16:45 schrieb Ævar Arnfjörð Bjarmason:\n>>\n>> On Sun, Jul 10 2022, René Scharfe wrote:\n>>\n>>> Some of the rules for using COPY_ARRAY instead of memcpy with sizeof are\n>>> intended to reduce the number of sizeof variants to deal with.  They can\n>>> have unintended side effects if only they match, but not the one for the\n>>> COPY_ARRAY conversion at the end.\n>>\n>> Since ab/cocci-unused is marked for \"next\" it would be really nice to\n>> have this based on top so we can add tests for these transformations\n>> (the topic adds a way to do that).\n>\n> Testing semantic patches sounds like a good idea.  We can add tests later,\n> once that feature landed.\n\nOkey, I might look at this then...\n\n>>\n>> But if you don't feel like  doing that this is fine too.\n>>\n>>> diff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci\n>>> index 9a4f00cb1b..aa75937950 100644\n>>> --- a/contrib/coccinelle/array.cocci\n>>> +++ b/contrib/coccinelle/array.cocci\n>>> @@ -1,60 +1,58 @@\n>>> -@@\n>>> -expression dst, src, n, E;\n>>> -@@\n>>> -  memcpy(dst, src, n * sizeof(\n>>> -- E[...]\n>>> -+ *(E)\n>>> -  ))\n>>> -\n>>> -@@\n>>> -type T;\n>>> -T *ptr;\n>>> -T[] arr;\n>>> -expression E, n;\n>>> -@@\n>>> -(\n>>> -  memcpy(ptr, E,\n>>> -- n * sizeof(*(ptr))\n>>> -+ n * sizeof(T)\n>>> -  )\n>>> -|\n>>> -  memcpy(arr, E,\n>>> -- n * sizeof(*(arr))\n>>> -+ n * sizeof(T)\n>>> -  )\n>>> -|\n>>> -  memcpy(E, ptr,\n>>> -- n * sizeof(*(ptr))\n>>> -+ n * sizeof(T)\n>>> -  )\n>>> -|\n>>> -  memcpy(E, arr,\n>>> -- n * sizeof(*(arr))\n>>> -+ n * sizeof(T)\n>>> -  )\n>>> -)\n>>> -\n>>>  @@\n>>>  type T;\n>>>  T *dst_ptr;\n>>>  T *src_ptr;\n>>> -T[] dst_arr;\n>>> -T[] src_arr;\n>>>  expression n;\n>>>  @@\n>>> -(\n>>> -- memcpy(dst_ptr, src_ptr, (n) * sizeof(T))\n>>> +- memcpy(dst_ptr, src_ptr, (n) * \\( sizeof(T)\n>>> +-                                \\| sizeof(*(dst_ptr))\n>>> +-                                \\| sizeof(*(src_ptr))\n>>> +-                                \\| sizeof(dst_ptr[...])\n>>> +-                                \\| sizeof(src_ptr[...])\n>>> +-                                \\) )\n>>>  + COPY_ARRAY(dst_ptr, src_ptr, n)\n>>> -|\n>>> -- memcpy(dst_ptr, src_arr, (n) * sizeof(T))\n>>> +\n>>> +@@\n>>> +type T;\n>>> +T *dst_ptr;\n>>> +T[] src_arr;\n>>> +expression n;\n>>> +@@\n>>> +- memcpy(dst_ptr, src_arr, (n) * \\( sizeof(T)\n>>> +-                                \\| sizeof(*(dst_ptr))\n>>> +-                                \\| sizeof(*(src_arr))\n>>> +-                                \\| sizeof(dst_ptr[...])\n>>> +-                                \\| sizeof(src_arr[...])\n>>> +-                                \\) )\n>>>  + COPY_ARRAY(dst_ptr, src_arr, n)\n>>> -|\n>>> -- memcpy(dst_arr, src_ptr, (n) * sizeof(T))\n>>> +\n>>> +@@\n>>> +type T;\n>>> +T[] dst_arr;\n>>> +T *src_ptr;\n>>> +expression n;\n>>> +@@\n>>> +- memcpy(dst_arr, src_ptr, (n) * \\( sizeof(T)\n>>> +-                                \\| sizeof(*(dst_arr))\n>>> +-                                \\| sizeof(*(src_ptr))\n>>> +-                                \\| sizeof(dst_arr[...])\n>>> +-                                \\| sizeof(src_ptr[...])\n>>> +-                                \\) )\n>>>  + COPY_ARRAY(dst_arr, src_ptr, n)\n>>> -|\n>>> -- memcpy(dst_arr, src_arr, (n) * sizeof(T))\n>>> +\n>>> +@@\n>>> +type T;\n>>> +T[] dst_arr;\n>>> +T[] src_arr;\n>>> +expression n;\n>>> +@@\n>>> +- memcpy(dst_arr, src_arr, (n) * \\( sizeof(T)\n>>> +-                                \\| sizeof(*(dst_arr))\n>>> +-                                \\| sizeof(*(src_arr))\n>>> +-                                \\| sizeof(dst_arr[...])\n>>> +-                                \\| sizeof(src_arr[...])\n>>> +-                                \\) )\n>>>  + COPY_ARRAY(dst_arr, src_arr, n)\n>>> -)\n>>>\n>>>  @@\n>>>  type T;\n>>\n>> Hrm, this seems like a lot of repetition, it's here in the rules you're\n>> editing already, but these repeated \"sizeof\" make it a lot more verbose.\n>>\n>> Isn't there a way to avoid this by simply wrapping this across lines, I\n>> didn't test, but I think you can do this sort of thing in the cocci\n>> grammar:\n>>\n>> - memcpy(\n>> - COPY_ARRAY(\n>>   (\n>>   dst_arr\n>>   |\n>>   dst_ptr\n>>   )\n>>   ,\n>>   (\n>>   src_arr\n>>   |\n>>   src_ptr\n>>   )\n>>   ,\n>>   (n) *\n>> -  [your big sizeof alternate here]\n>>   )\n>\n> Hmm, that would match many more combinations, e.g. this one:\n>\n> void f(int *a, int *b, long n, int c[1]) {\n> \tmemcpy(a, b, n * sizeof(*c));\n> }\n>\n> The elements of a, b and c have the same type, so replacing c with a\n> (which a conversion to COPY_ARRAY would do) would produce the same\n> object code.  I can't come up with a plausible scenario like above and\n> where a type change of c down the line would cause problems, but I\n> also can't convince myself that no such thing can exist.  Tricky.\n>\n> And I can't get it to format the whitespace around the third argument\n> of COPY_ARRAY nicely in all cases.\n>\n> And it takes 37% longer on my machine.\n>\n> But it sure is more compact. :)\n>\n> @@\n> type T;\n> T *dst_ptr;\n> T *src_ptr;\n> T[] dst_arr;\n> T[] src_arr;\n> expression n;\n> @@\n> - memcpy\n> + COPY_ARRAY\n>   (\n>   \\( dst_ptr \\| dst_arr \\) ,\n>   \\( src_ptr \\| src_arr \\) ,\n> - (n) *  \\( sizeof(T)\n> -        \\| sizeof(*(dst_ptr))\n> -        \\| sizeof(*(dst_arr))\n> -        \\| sizeof(*(src_ptr))\n> -        \\| sizeof(*(src_arr))\n> -        \\| sizeof(dst_ptr[...])\n> -        \\| sizeof(dst_arr[...])\n> -        \\| sizeof(src_ptr[...])\n> -        \\| sizeof(src_arr[...])\n> -        \\)\n> + n\n>   )\n\n..once those land, because cocci debug output is a lot more useful with\ntarget test data :)\n"},{"id":"459328","messageId":"a27913f5-797d-3de2-4c73-5c1b8e654d44@github.com","threadId":"58115","inReplyTo":"xmqq4jzpu4xp.fsf_-_@gitster.g","subject":"Re: [PATCH v3] builtin/mv.c: use the MOVE_ARRAY() macro instead of memmove()","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-07-18T20:30:54Z","receivedAt":"2022-07-18T20:31:05Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/9/2022 9:33 PM, Junio C Hamano wrote:\n> The variables 'source', 'destination', and 'submodule_gitfile' are\n> all of type \"const char **\", and an element of such an array is of\n> \"type const char *\", but these memmove() calls were written as if\n> these variables are of type \"char **\".\n> \n> Once these memmove() calls are fixed to use the correct type to\n> compute the number of bytes to be moved, e.g.\n> \n> -      memmove(source + i, source + i + 1, n * sizeof(char *));\n> +      memmove(source + i, source + i + 1, n * sizeof(const char *));\n> \n> existing contrib/coccinelle/array.cocci rules can recognize them as\n> candidates for turning into MOVE_ARRAY().\n> \n> While at it, use CALLOC_ARRAY() instead of xcalloc() to allocate the\n> modes[] array that is involved in the change.\n\nI'm super late in noticing this, but these changes will fix the\nstatic-analysis build in the CI build, so the sooner this can make\nit to master the better.\n\nThanks!\n-Stolee\n\n"}]}