{"thread":{"id":"58347","subject":"[PATCH] promisor-remote: fix xcalloc() argument order","startedAt":"2022-08-22T21:34:21Z","lastAt":"2022-08-25T13:59:16Z","messageCount":14,"participants":["SZEDER Gábor","Junio C Hamano","Jeff King","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"461814","messageId":"20220822213408.662482-1-szeder.dev@gmail.com","threadId":"58347","inReplyTo":null,"subject":"[PATCH] promisor-remote: fix xcalloc() argument order","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-08-22T21:34:08Z","receivedAt":"2022-08-22T21:34:21Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Pass the number of elements first and their size second, as expected\nby xcalloc().\n\nPatch generated with 'contrib/coccinelle/xcalloc.cocci' and Coccinelle\nv1.0.7 or later (previous Coccinelle versions don't notice this).\n\nSigned-off-by: SZEDER Gábor \n---\n promisor-remote.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 5b33f88bca..68f46f5ec7 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -146,7 +146,7 @@ static void promisor_remote_init(struct repository *r)\n \tif (r->promisor_remote_config)\n \t\treturn;\n \tconfig = r->promisor_remote_config =\n-\t\txcalloc(sizeof(*r->promisor_remote_config), 1);\n+\t\txcalloc(1, sizeof(*r->promisor_remote_config));\n \tconfig->promisors_tail = &config->promisors;\n \n \trepo_config(r, promisor_remote_config, config);\n-- \n2.37.2.880.g3b8fb2ce68\n\n"},{"id":"461817","messageId":"xmqqh7249b8d.fsf@gitster.g","threadId":"58347","inReplyTo":"20220822213408.662482-1-szeder.dev@gmail.com","subject":"Re: [PATCH] promisor-remote: fix xcalloc() argument order","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-22T22:14:58Z","receivedAt":"2022-08-22T22:15:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> Pass the number of elements first and their size second, as expected\n> by xcalloc().\n>\n> Patch generated with 'contrib/coccinelle/xcalloc.cocci' and Coccinelle\n> v1.0.7 or later (previous Coccinelle versions don't notice this).\n\nOne thing is that Coccinelle is way too slow on our codebase,\ncompared to the usual compilation, to run every time we make\nchanges. Combined with the fact that our codebase is mostly clean,\nthe cycles feel huge waste of time only to find something small like\nwhat this patch fixes.\n\nThat sadly discourages us from doing \"make coccicheck\" more often as\nwe should.\n\nI _think_ Googlers have 1.1.1 on their linux boxes, so even if our\nGitHub Actions CI is fixed to Ubuntu 18.04 and does not run more\nrecent Coccinelle, we theoretically should have been able to catch\nit before it hit the public list, if \"1.0.7 or later\" was the\ncondition. FWIW, \"make coccicheck\" with what I happen to have\nnotices it.\n\n$ spatch version\nspatch version 1.1.1 compiled with OCaml version 4.13.1\nFlags passed to the configure script: --prefix=/usr --sysconfdir=/etc --libdir=/usr/lib --enable-ocaml --enable-python --enable-opt\nOCaml scripting support: yes\nPython scripting support: yes\nSyntax of regular expressions: PCRE\n\nAnyway, the patch is correct.\n\nThanks, will queue.\n\n> Signed-off-by: SZEDER Gábor \n> ---\n> promisor-remote.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/promisor-remote.c b/promisor-remote.c\n> index 5b33f88bca..68f46f5ec7 100644\n> --- a/promisor-remote.c\n> +++ b/promisor-remote.c\n> @@ -146,7 +146,7 @@ static void promisor_remote_init(struct repository *r)\n> \tif (r->promisor_remote_config)\n> \t\treturn;\n> \tconfig = r->promisor_remote_config =\n> -\t\txcalloc(sizeof(*r->promisor_remote_config), 1);\n> +\t\txcalloc(1, sizeof(*r->promisor_remote_config));\n> \tconfig->promisors_tail = &config->promisors;\n> \n> \trepo_config(r, promisor_remote_config, config);\n"},{"id":"461823","messageId":"xmqq5yijahpm.fsf@gitster.g","threadId":"58347","inReplyTo":"xmqqh7249b8d.fsf@gitster.g","subject":"Re: [PATCH] promisor-remote: fix xcalloc() argument order","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-23T01:09:41Z","receivedAt":"2022-08-23T01:09:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> ... FWIW, \"make coccicheck\" with what I happen to have\n> notices it.\n\nOops, that was a serious typo. \"notices\" -> \"fails to notice\".\n\n> $ spatch version\n> spatch version 1.1.1 compiled with OCaml version 4.13.1\n> Flags passed to the configure script: --prefix=/usr --sysconfdir=/etc --libdir=/usr/lib --enable-ocaml --enable-python --enable-opt\n> OCaml scripting support: yes\n> Python scripting support: yes\n> Syntax of regular expressions: PCRE\n>\n> Anyway, the patch is correct.\n>\n> Thanks, will queue.\n>\n>> Signed-off-by: SZEDER Gábor \n>> ---\n>> promisor-remote.c | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/promisor-remote.c b/promisor-remote.c\n>> index 5b33f88bca..68f46f5ec7 100644\n>> --- a/promisor-remote.c\n>> +++ b/promisor-remote.c\n>> @@ -146,7 +146,7 @@ static void promisor_remote_init(struct repository *r)\n>> \tif (r->promisor_remote_config)\n>> \t\treturn;\n>> \tconfig = r->promisor_remote_config =\n>> -\t\txcalloc(sizeof(*r->promisor_remote_config), 1);\n>> +\t\txcalloc(1, sizeof(*r->promisor_remote_config));\n>> \tconfig->promisors_tail = &config->promisors;\n>> \n>> \trepo_config(r, promisor_remote_config, config);\n"},{"id":"461835","messageId":"20220823070417.GA1735@szeder.dev","threadId":"58347","inReplyTo":"xmqq5yijahpm.fsf@gitster.g","subject":"Re: [PATCH] promisor-remote: fix xcalloc() argument order","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-08-23T07:04:17Z","receivedAt":"2022-08-23T07:04:24Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Aug 22, 2022 at 06:09:41PM -0700, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > ... FWIW, \"make coccicheck\" with what I happen to have\n> > notices it.\n> \n> Oops, that was a serious typo. \"notices\" -> \"fails to notice\".\n\nHm, that's strange. 1.1.1 did notice this transformation for me, as\ndid 1.1.0, 1.0.8, 1.0.7, and, contrary to what I wrote in the commit\nmessage, even 1.0.6 as well. (IIRC Ubuntu 18.04 has 1.0.4, which I\ndidn't try, but if that could notice it, then we should have heard\nabout it from CI.)\n\n$ spatch --version\nspatch version 1.1.1 compiled with OCaml version 4.08.1\nFlags passed to the configure script: [none]\nOCaml scripting support: yes\nPython scripting support: yes\nSyntax of regular expressions: PCRE\n\n\n> > $ spatch version\n> > spatch version 1.1.1 compiled with OCaml version 4.13.1\n> > Flags passed to the configure script: --prefix=/usr --sysconfdir=/etc --libdir=/usr/lib --enable-ocaml --enable-python --enable-opt\n> > OCaml scripting support: yes\n> > Python scripting support: yes\n> > Syntax of regular expressions: PCRE\n\n"},{"id":"461844","messageId":"20220823092156.GB1735@szeder.dev","threadId":"58347","inReplyTo":"20220823070417.GA1735@szeder.dev","subject":"Re: [PATCH] promisor-remote: fix xcalloc() argument order","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-08-23T09:21:56Z","receivedAt":"2022-08-23T11:20:09Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Aug 23, 2022 at 09:04:17AM +0200, SZEDER Gábor wrote:\n> On Mon, Aug 22, 2022 at 06:09:41PM -0700, Junio C Hamano wrote:\n> > Junio C Hamano writes:\n> > \n> > > ... FWIW, \"make coccicheck\" with what I happen to have\n> > > notices it.\n> > \n> > Oops, that was a serious typo. \"notices\" -> \"fails to notice\".\n> \n> Hm, that's strange. 1.1.1 did notice this transformation for me,\n\nHang on, it's not quite that simple.\nWatch this, it will get weird:\n\n# Batched spatch for faster processing, see 960154b9c1 (coccicheck:\n# optionally batch spatch invocations, 2019-05-06)\n$ grep SPATCH_BATCH_SIZE config.mak\nSPATCH_BATCH_SIZE = 32\n$ time make contrib/coccinelle/xcalloc.cocci.patch \n SPATCH contrib/coccinelle/xcalloc.cocci\n SPATCH result: contrib/coccinelle/xcalloc.cocci.patch\n\nreal\t0m35.902s\nuser\t0m34.840s\nsys\t0m1.030s\n# Found it, good.\n$ make -s cocciclean\n# Turn off batched processing (this is the default)\n$ sed -i -e '/SPATCH_BATCH_SIZE/ s/= .*/= 1/' config.mak\n$ time make contrib/coccinelle/xcalloc.cocci.patch \n SPATCH contrib/coccinelle/xcalloc.cocci\n\nreal\t0m24.553s\nuser\t0m21.468s\nsys\t0m3.099s\n# Not only missed the transformation, but it's faster than batched\n# processing?!\n\n# Let's invoke spatch directly.\n$ spatch --all-includes --sp-file contrib/coccinelle/xcalloc.cocci --patch . promisor-remote.c \nwarning: Can't find macro file: /usr/local/bin/lib/coccinelle/standard.h\nwarning: Can't find default iso file: /usr/local/bin/lib/coccinelle/standard.iso\nHANDLING: promisor-remote.c\n# Nope.\n$ spatch --all-includes --sp-file contrib/coccinelle/xcalloc.cocci --patch . promisor-remote.c lockfile.c\nwarning: Can't find macro file: /usr/local/bin/lib/coccinelle/standard.h\nwarning: Can't find default iso file: /usr/local/bin/lib/coccinelle/standard.iso\nHANDLING: promisor-remote.c lockfile.c\n$ spatch --all-includes --sp-file contrib/coccinelle/xcalloc.cocci --patch . promisor-remote.c git.c \nwarning: Can't find macro file: /usr/local/bin/lib/coccinelle/standard.h\nwarning: Can't find default iso file: /usr/local/bin/lib/coccinelle/standard.iso\nHANDLING: promisor-remote.c git.c\n$ spatch --all-includes --sp-file contrib/coccinelle/xcalloc.cocci --patch . promisor-remote.c usage.c\nwarning: Can't find macro file: /usr/local/bin/lib/coccinelle/standard.h\nwarning: Can't find default iso file: /usr/local/bin/lib/coccinelle/standard.iso\nHANDLING: promisor-remote.c usage.c\n# Nope, nope, nope.\n$ spatch --all-includes --sp-file contrib/coccinelle/xcalloc.cocci --patch . promisor-remote.c builtin/*.c\n[...]\n# Nope!\n\n# But watch this!\n$ spatch --all-includes --sp-file contrib/coccinelle/xcalloc.cocci --patch . promisor-remote.c config.c\nwarning: Can't find macro file: /usr/local/bin/lib/coccinelle/standard.h\nwarning: Can't find default iso file: /usr/local/bin/lib/coccinelle/standard.iso\nHANDLING: promisor-remote.c config.c\ndiff = \ndiff -u -p a/promisor-remote.c b/promisor-remote.c\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -146,7 +146,7 @@ static void promisor_remote_init(struct\n \tif (r->promisor_remote_config)\n \t\treturn;\n \tconfig = r->promisor_remote_config =\n-\t\txcalloc(sizeof(*r->promisor_remote_config), 1);\n+\t\txcalloc(1, sizeof(*r->promisor_remote_config));\n \tconfig->promisors_tail = &config->promisors;\n \n \trepo_config(r, promisor_remote_config, config);\n\n# Huh?! (;\n\nFWIW, I see this with Coccinelle 1.1.1, 1.0.8 and 1.0.6 as well.\n\n"},{"id":"461845","messageId":"20220823095602.GC1735@szeder.dev","threadId":"58347","inReplyTo":"20220823092156.GB1735@szeder.dev","subject":"Re: [PATCH] promisor-remote: fix xcalloc() argument order","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-08-23T09:56:02Z","receivedAt":"2022-08-23T12:49:29Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Aug 23, 2022 at 11:21:56AM +0200, SZEDER Gábor wrote:\n> On Tue, Aug 23, 2022 at 09:04:17AM +0200, SZEDER Gábor wrote:\n> > On Mon, Aug 22, 2022 at 06:09:41PM -0700, Junio C Hamano wrote:\n> > > Junio C Hamano writes:\n> > > \n> > > > ... FWIW, \"make coccicheck\" with what I happen to have\n> > > > notices it.\n> > > \n> > > Oops, that was a serious typo. \"notices\" -> \"fails to notice\".\n> > \n> > Hm, that's strange. 1.1.1 did notice this transformation for me,\n\n\n> # Let's invoke spatch directly.\n> $ spatch --all-includes --sp-file contrib/coccinelle/xcalloc.cocci --patch . promisor-remote.c \n> warning: Can't find macro file: /usr/local/bin/lib/coccinelle/standard.h\n> warning: Can't find default iso file: /usr/local/bin/lib/coccinelle/standard.iso\n> HANDLING: promisor-remote.c\n> # Nope.\n> $ spatch --all-includes --sp-file contrib/coccinelle/xcalloc.cocci --patch . promisor-remote.c lockfile.c\n> warning: Can't find macro file: /usr/local/bin/lib/coccinelle/standard.h\n> warning: Can't find default iso file: /usr/local/bin/lib/coccinelle/standard.iso\n> HANDLING: promisor-remote.c lockfile.c\n> $ spatch --all-includes --sp-file contrib/coccinelle/xcalloc.cocci --patch . promisor-remote.c git.c \n> warning: Can't find macro file: /usr/local/bin/lib/coccinelle/standard.h\n> warning: Can't find default iso file: /usr/local/bin/lib/coccinelle/standard.iso\n> HANDLING: promisor-remote.c git.c\n> $ spatch --all-includes --sp-file contrib/coccinelle/xcalloc.cocci --patch . promisor-remote.c usage.c\n> warning: Can't find macro file: /usr/local/bin/lib/coccinelle/standard.h\n> warning: Can't find default iso file: /usr/local/bin/lib/coccinelle/standard.iso\n> HANDLING: promisor-remote.c usage.c\n> # Nope, nope, nope.\n> $ spatch --all-includes --sp-file contrib/coccinelle/xcalloc.cocci --patch . promisor-remote.c builtin/*.c\n> [...]\n> # Nope!\n> \n> # But watch this!\n> $ spatch --all-includes --sp-file contrib/coccinelle/xcalloc.cocci --patch . promisor-remote.c config.c\n> warning: Can't find macro file: /usr/local/bin/lib/coccinelle/standard.h\n> warning: Can't find default iso file: /usr/local/bin/lib/coccinelle/standard.iso\n> HANDLING: promisor-remote.c config.c\n> diff = \n> diff -u -p a/promisor-remote.c b/promisor-remote.c\n> --- a/promisor-remote.c\n> +++ b/promisor-remote.c\n> @@ -146,7 +146,7 @@ static void promisor_remote_init(struct\n> \tif (r->promisor_remote_config)\n> \t\treturn;\n> \tconfig = r->promisor_remote_config =\n> -\t\txcalloc(sizeof(*r->promisor_remote_config), 1);\n> +\t\txcalloc(1, sizeof(*r->promisor_remote_config));\n> \tconfig->promisors_tail = &config->promisors;\n> \n> \trepo_config(r, promisor_remote_config, config);\n> \n> # Huh?! (;\n> \n> FWIW, I see this with Coccinelle 1.1.1, 1.0.8 and 1.0.6 as well.\n\nOk, I think I got this.\n\nThe transformation involves 'r->promisor_remote_config', where 'r' is\na 'struct repository*'. However, 'promisor-remote.c' doesn't include\n'repository.h' directly:\n\n $ grep '^#include' promisor-remote.c \n #include \"cache.h\"\n #include \"object-store.h\"\n #include \"promisor-remote.h\"\n #include \"config.h\"\n #include \"transport.h\"\n #include \"strvec.h\"\n\nIf I add an '#include \"repository.h\" to 'promisor-remote.c', then\nCoccinelle finds the transformation even when it processes this source\nfile on is own:\n\n $ spatch --all-includes --sp-file contrib/coccinelle/xcalloc.cocci --patch . promisor-remote.c\n init_defs_builtins: /usr/local/lib/coccinelle/standard.h\n HANDLING: promisor-remote.c\n diff = \n diff -u -p a/promisor-remote.c b/promisor-remote.c\n --- a/promisor-remote.c\n +++ b/promisor-remote.c\n @@ -149,7 +149,7 @@ static void promisor_remote_init(struct\n \tif (r->promisor_remote_config)\n \t\treturn;\n \tconfig = r->promisor_remote_config =\n -\t\txcalloc(sizeof(*r->promisor_remote_config), 1);\n +\t\txcalloc(1, sizeof(*r->promisor_remote_config));\n \tconfig->promisors_tail = &config->promisors;\n \n \trepo_config(r, promisor_remote_config, config);\n\n"},{"id":"461846","messageId":"20220823095733.58685-1-szeder.dev@gmail.com","threadId":"58347","inReplyTo":"20220822213408.662482-1-szeder.dev@gmail.com","subject":"[PATCH v2] promisor-remote: fix xcalloc() argument order","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-08-23T09:57:33Z","receivedAt":"2022-08-23T12:51:51Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Pass the number of elements first and their size second, as expected\nby xcalloc().\n\nPatch generated with:\n\n make SPATCH_FLAGS=--recursive-includes contrib/coccinelle/xcalloc.cocci.patch\n\nOur default SPATCH_FLAGS ('--all-includes') doesn't catch this\ntransformation by default, unless used in combination with a large-ish\nSPATCH_BATCH_SIZE which happens to put 'promisor-remote.c' with a file\nthat includes 'repository.h' directly in the same batch.\n\nSigned-off-by: SZEDER Gábor \n---\n promisor-remote.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 5b33f88bca..68f46f5ec7 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -146,7 +146,7 @@ static void promisor_remote_init(struct repository *r)\n \tif (r->promisor_remote_config)\n \t\treturn;\n \tconfig = r->promisor_remote_config =\n-\t\txcalloc(sizeof(*r->promisor_remote_config), 1);\n+\t\txcalloc(1, sizeof(*r->promisor_remote_config));\n \tconfig->promisors_tail = &config->promisors;\n \n \trepo_config(r, promisor_remote_config, config);\n-- \n2.37.2.880.g3b8fb2ce68\n\n"},{"id":"461891","messageId":"YwXiKXONiTt+fIdi@coredump.intra.peff.net","threadId":"58347","inReplyTo":"20220823095733.58685-1-szeder.dev@gmail.com","subject":"Re: [PATCH v2] promisor-remote: fix xcalloc() argument order","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-24T08:32:41Z","receivedAt":"2022-08-24T08:32:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 23, 2022 at 11:57:33AM +0200, SZEDER Gábor wrote:\n\n> Pass the number of elements first and their size second, as expected\n> by xcalloc().\n> \n> Patch generated with:\n> \n> make SPATCH_FLAGS=--recursive-includes contrib/coccinelle/xcalloc.cocci.patch\n\nThanks for digging here. I think it probably explains a lot of \"wait,\nwhy did this result change\" head-scratching I did back when we started\nbatching a few years ago.\n\nIs there any reason we wouldn't want --recursive-includes to be added to\nthe default SPATCH_FLAGS?\n\nI suspect we'd still want to leave --all-includes there to get as much\ntype information as possible. If I understand correctly, the two are\northogonal (one is \"follow includes of includes\" and the other is\n\"follow system includes in angle brackets\").\n\nDoing so doesn't seem to find any other missed entries in the current\ncodebase, but I'm pretty sure there are some it would have caught in a\nless mysterious fashion over the years.\n\n-Peff\n"},{"id":"461894","messageId":"20220824113901.GD1735@szeder.dev","threadId":"58347","inReplyTo":"YwXiKXONiTt+fIdi@coredump.intra.peff.net","subject":"Re: [PATCH v2] promisor-remote: fix xcalloc() argument order","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-08-24T11:39:01Z","receivedAt":"2022-08-24T11:39:10Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Aug 24, 2022 at 04:32:41AM -0400, Jeff King wrote:\n> On Tue, Aug 23, 2022 at 11:57:33AM +0200, SZEDER Gábor wrote:\n> \n> > Pass the number of elements first and their size second, as expected\n> > by xcalloc().\n> > \n> > Patch generated with:\n> > \n> > make SPATCH_FLAGS=--recursive-includes contrib/coccinelle/xcalloc.cocci.patch\n> \n> Thanks for digging here. I think it probably explains a lot of \"wait,\n> why did this result change\" head-scratching I did back when we started\n> batching a few years ago.\n> \n> Is there any reason we wouldn't want --recursive-includes to be added to\n> the default SPATCH_FLAGS?\n\nI ran some runtime measurements yesterday, and there is a considerable\nruntime increase in the unbatched case:\n\n The tables below show the runtimes of applying each of our semantic\n patches separately with Coccinelle v1.1.1, with the '--all-includes'\n or '--recursive-includes' flags and with batch sizes 1 (no batching)\n or 32, i.e.:\n \n time make SPATCH_FLAGS=$flag SPATCH_BATCH_SIZE=$size $cocci.patch\n \n SPATCH_BATCH_SIZE=1:\n \n semantic patch | all | recursive |\n -----------------+-----------+-----------+--------\n array | 69.90s | 140.12s | +100%\n commit | 94.44s | 223.63s | +137%\n equals-null | 86.93s | 205.40s | +136%\n flex_alloc | 11.32s | 16.45s | +45%\n free | 70.47s | 159.75s | +127%\n hashmap | 83.48s | 199.70s | +139%\n object_id | 107.83s | 241.69s | +124%\n preincr | 79.33s | 202.98s | +156%\n qsort | 16.20s | 33.86s | +109%\n strbuf | 60.54s | 129.93s | +115%\n swap | 81.70s | 200.75s | +146%\n unused | 499.19s | 626.35s | +25%\n xcalloc | 26.71s | 57.63s | +116%\n xopen | 30.92s | 59.26s | +92%\n xstrdup_or_null | 5.05s | 6.94s | +37%\n -----------------+-----------+-----------+--------\n all | 1324s | 2504s | +89%\n \n SPATCH_BATCH_SIZE=32:\n \n semantic patch | all | recursive |\n -----------------+-----------+-----------+--------\n array | 43.81s | 52.83s | +21%\n commit | 50.16s | 52.76s | +5%\n equals-null | 47.77s | 50.86s | +6%\n flex_alloc | 41.00s | 43.64s | +6%\n free | 43.12s | 46.68s | +8%\n hashmap | 42.76s | 46.12s | +8%\n object_id | 56.17s | 60.00s | +7%\n preincr | 39.82s | 42.57s | +7%\n qsort | 39.48s | 53.09s | +34%\n strbuf | 51.27s | 49.38s | -4%\n swap | 41.93s | 58.17s | +39%\n unused | 440.86s | 445.47s | +1%\n xcalloc | 39.90s | 42.22s | +6%\n xopen | 40.26s | 43.19s | +7%\n xstrdup_or_null | 39.14s | 41.72s | +7%\n -----------------+-----------+-----------+--------\n all | 1057s | 1129s | +7%\n \n I don't have meaningful numbers about the impact of\n '--recursive-includes' on the runtime of a parallel 'make -j\n coccicheck', because they're severely skewed by 'unused.cocci' and\n 'make's scheduler: applying 'unused.cocci' takes 5-10x as long as\n other semantic patches (accounting for about 38-42% of the total\n sequential runtime), and 'make' on my system usually schedules it near\n the end, meaning that for a significant part there is no parallel\n processing at all.\n\nSo I'm not sure about making '--recursive-includes' the default, at\nleast as long as we don't batch by default. However, we could still\nuse this flag in CI...\n\n> I suspect we'd still want to leave --all-includes there to get as much\n> type information as possible. If I understand correctly, the two are\n> orthogonal (one is \"follow includes of includes\" and the other is\n> \"follow system includes in angle brackets\").\n\n'spatch --help' tells me:\n\n --recursive-includes causes all available include files, both\n those included in the C file(s) and those\n included in header files, to be used\n --all-includes causes all available include files included\n in the C file(s) to be used\n\nSo I think the difference is not about system includes, but rather\nabout \"consider includes of includes\" vs. \"consider only direct\nincludes in the C files\", so it seems '--recursive-includes' is a\nsuperset of '--all-includes'. Oh, and let's not forget the rather\nsurprising behavior that if spatch processes multiple C files at once,\nthen for each of the C files it considers all includes from all C\nfiles; this explains why '--all-includes promisor-remote.c config.c'\nworks, because 'config.c' does include 'repository.h', supplying the\nnecessary type information to catch the previously missed\ntransformation in 'promisor-remote.c'.\n\n\nThese descriptions don't make it clear (to me) whether the two options\nare orthogonal, exclusive, or something else. However, trying out various\ncombinations indicates that \"last one wins\", e.g.:\n\n$ spatch --recursive-includes --all-includes --sp-file contrib/coccinelle/xcalloc.cocci --patch . promisor-remote.c\nwarning: Can't find macro file: /usr/local/bin/lib/coccinelle/standard.h\nwarning: Can't find default iso file: /usr/local/bin/lib/coccinelle/standard.iso\nHANDLING: promisor-remote.c\n$ spatch --all-includes --recursive-includes --sp-file contrib/coccinelle/xcalloc.cocci --patch . promisor-remote.c\nwarning: Can't find macro file: /usr/local/bin/lib/coccinelle/standard.h\nwarning: Can't find default iso file: /usr/local/bin/lib/coccinelle/standard.iso\nHANDLING: promisor-remote.c\ndiff = \ndiff -u -p a/promisor-remote.c b/promisor-remote.c\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -146,7 +146,7 @@ static void promisor_remote_init(struct\n \tif (r->promisor_remote_config)\n \t\treturn;\n \tconfig = r->promisor_remote_config =\n-\t\txcalloc(sizeof(*r->promisor_remote_config), 1);\n+\t\txcalloc(1, sizeof(*r->promisor_remote_config));\n \tconfig->promisors_tail = &config->promisors;\n \n \trepo_config(r, promisor_remote_config, config);\n\n"},{"id":"461899","messageId":"xmqqwnax8wgy.fsf@gitster.g","threadId":"58347","inReplyTo":"20220823095733.58685-1-szeder.dev@gmail.com","subject":"Re: [PATCH v2] promisor-remote: fix xcalloc() argument order","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-24T15:58:21Z","receivedAt":"2022-08-24T15:58:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> Patch generated with:\n>\n> make SPATCH_FLAGS=--recursive-includes contrib/coccinelle/xcalloc.cocci.patch\n>\n> Our default SPATCH_FLAGS ('--all-includes') doesn't catch this\n> transformation by default, unless used in combination with a large-ish\n> SPATCH_BATCH_SIZE which happens to put 'promisor-remote.c' with a file\n> that includes 'repository.h' directly in the same batch.\n\nOur default SPATCH_FLAGS is actually\n\n SPATCH_FLAGS = --all-includes --patch .\n\nand I am wondering how \"--patch .\" part (or droppage thereof due to\noverriding it from the command line) affects the outcome.\n\nIn any case, good finding how the header file inclusion was affected\nwith batch-size and this option that solved a mystery. Thanks.\n"},{"id":"461902","messageId":"20220824173307.GF1735@szeder.dev","threadId":"58347","inReplyTo":"xmqqwnax8wgy.fsf@gitster.g","subject":"Re: [PATCH v2] promisor-remote: fix xcalloc() argument order","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-08-24T17:33:07Z","receivedAt":"2022-08-24T17:33:14Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Aug 24, 2022 at 08:58:21AM -0700, Junio C Hamano wrote:\n> SZEDER Gábor writes:\n> \n> > Patch generated with:\n> >\n> > make SPATCH_FLAGS=--recursive-includes contrib/coccinelle/xcalloc.cocci.patch\n> >\n> > Our default SPATCH_FLAGS ('--all-includes') doesn't catch this\n> > transformation by default, unless used in combination with a large-ish\n> > SPATCH_BATCH_SIZE which happens to put 'promisor-remote.c' with a file\n> > that includes 'repository.h' directly in the same batch.\n> \n> Our default SPATCH_FLAGS is actually\n> \n> SPATCH_FLAGS = --all-includes --patch .\n> \n> and I am wondering how \"--patch .\" part (or droppage thereof due to\n> overriding it from the command line) affects the outcome.\n\n'--patch .' is not part of SPATCH_FLAGS anymore, and for a good\nreason, see the recent 7b63ea5750 (Makefile: remove mandatory \"spatch\"\narguments from SPATCH_FLAGS, 2022-07-05).\n\n"},{"id":"461914","messageId":"xmqqbks95oq2.fsf@gitster.g","threadId":"58347","inReplyTo":"20220824173307.GF1735@szeder.dev","subject":"Re: [PATCH v2] promisor-remote: fix xcalloc() argument order","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-24T21:13:57Z","receivedAt":"2022-08-24T21:14:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> On Wed, Aug 24, 2022 at 08:58:21AM -0700, Junio C Hamano wrote:\n>> SZEDER Gábor writes:\n>> \n>> > Patch generated with:\n>> >\n>> > make SPATCH_FLAGS=--recursive-includes contrib/coccinelle/xcalloc.cocci.patch\n>> >\n>> > Our default SPATCH_FLAGS ('--all-includes') doesn't catch this\n>> > transformation by default, unless used in combination with a large-ish\n>> > SPATCH_BATCH_SIZE which happens to put 'promisor-remote.c' with a file\n>> > that includes 'repository.h' directly in the same batch.\n>> \n>> Our default SPATCH_FLAGS is actually\n>> \n>> SPATCH_FLAGS = --all-includes --patch .\n>> \n>> and I am wondering how \"--patch .\" part (or droppage thereof due to\n>> overriding it from the command line) affects the outcome.\n>\n> '--patch .' is not part of SPATCH_FLAGS anymore, and for a good\n> reason, see the recent 7b63ea5750 (Makefile: remove mandatory \"spatch\"\n> arguments from SPATCH_FLAGS, 2022-07-05).\n\nAhh, that makes my life a bit more cumbersome, as I was looking at\nhow we did things on the 'maint' branch.\n\nThanks, mystery solved.\n"},{"id":"461941","messageId":"YwdRqP1CyUAzCEn2@coredump.intra.peff.net","threadId":"58347","inReplyTo":"20220824113901.GD1735@szeder.dev","subject":"Re: [PATCH v2] promisor-remote: fix xcalloc() argument order","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-25T10:40:40Z","receivedAt":"2022-08-25T10:40:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 24, 2022 at 01:39:01PM +0200, SZEDER Gábor wrote:\n\n> I ran some runtime measurements yesterday, and there is a considerable\n> runtime increase in the unbatched case:\n\nLet me condense and rearrange this a bit to the part I think is\nimportant:\n\n> semantic patch | all | recursive |\n> -----------------+-----------+-----------+--------\n> SPATCH_BATCH_SIZE=1:\n> all | 1324s | 2504s | +89%\n> SPATCH_BATCH_SIZE=32:\n> all | 1057s | 1129s | +7%\n\nRight, that's not surprising. \"recursive\" means we're looking at more\nheader files than before. For the batch case it has less impact because\nit's more likely that some other file we're processing in the batch\n_also_ was looking at that header, so we saw it anyway.\n\nWith an infinite bath size, adding --recursive-includes should make no\ndifference (or almost none; it's possible there are headers which\nare _only_ included recursively across the whole code base).\n\nMy numbers for array.cocci are:\n\n SPATCH_BATCH_SIZE=1, 133s\n SPATCH_BATCH_SIZE=32, 34s\n SPATCH_BATCH_SIZE=0, 31s\n\nOf course there we run into potential memory problems. Based on glancing\nat top occasionally, the first one peaked around 100MB of memory, the\nsecond around 200MB, and the third one around 2GB. So \"32\" is probably\nstill a reasonable middle ground for CPU vs memory.\n\nSpending extra CPU, especially for people stuck BATCH_SIZE=1, does suck.\nBut the tool is already terribly slow. Even if we spend twice as much\nCPU (which is what your numbers above show), isn't it worth it to get\nthe _correct_ result? To me there is not much point in running it at all\nif it misses cases.\n\nIt would be nice if there was some way to tell coccinelle not to\nactually _analyze_ included header files, but just to include them to\npick up types, etc (and then we'd add the headers themselves to\nCOCCI_SOURCES to check them). But I don't see a way to do that. And I\nwouldn't be surprised if the code simply isn't written in a way to make\nthat easy internally.\n\nAnother alternative would be --no-includes (and again, listing the\nheaders manually via COCCI_SOURCES). But as the example you found shows,\nit really does need the headers in order to properly process the code in\nthe C files themselves.\n\n> I don't have meaningful numbers about the impact of\n> '--recursive-includes' on the runtime of a parallel 'make -j\n> coccicheck', because they're severely skewed by 'unused.cocci' and\n> 'make's scheduler: applying 'unused.cocci' takes 5-10x as long as\n> other semantic patches (accounting for about 38-42% of the total\n> sequential runtime), and 'make' on my system usually schedules it near\n> the end, meaning that for a significant part there is no parallel\n> processing at all.\n\nIt may be that unused.cocci can be optimized. Once upon a time I spent\nsome effort optimizing some of the rules files. I found that if you\nrelied on having python support, it helped quite a bit, because without\nit you are stuck with the \"...\" operator, which can be very expensive.\nMore details in this thread:\n\n https://lore.kernel.org/git/20180824064228.GA3183@sigill.intra.peff.net/\n\nBack then the debian package wasn't being actively updated, and it\nwasn't clear if python support would remain. But these days it seems a\nbit more stable. And with the CI job available, I'd prefer to optimize\nover making the tool pleasant to use for people who do use it, rather\nthan worrying that everybody on every platform/build can use it.\n\nAll that said, I am not about to get sucked into the rabbit hole of\ncoccinelle optimization anytime soon. So good luck to you, if you're\ninterested. ;)\n\n> > I suspect we'd still want to leave --all-includes there to get as much\n> > type information as possible. If I understand correctly, the two are\n> > orthogonal (one is \"follow includes of includes\" and the other is\n> > \"follow system includes in angle brackets\").\n> \n> 'spatch --help' tells me:\n> \n> --recursive-includes causes all available include files, both\n> those included in the C file(s) and those\n> included in header files, to be used\n> --all-includes causes all available include files included\n> in the C file(s) to be used\n> \n> So I think the difference is not about system includes, but rather\n> about \"consider includes of includes\" vs. \"consider only direct\n> includes in the C files\", so it seems '--recursive-includes' is a\n> superset of '--all-includes'.\n\nI'm not sure, because there's also --local-includes and --no-includes,\nwhich are grouped with --all-includes. (The grouping is doubly confusing\nbecause my copy of the manpage has _two_ options sections, and only some\nof the options are mentioned together in the first one).\n\n> Oh, and let's not forget the rather surprising behavior that if spatch\n> processes multiple C files at once, then for each of the C files it\n> considers all includes from all C files; this explains why\n> '--all-includes promisor-remote.c config.c' works, because 'config.c'\n> does include 'repository.h', supplying the necessary type information\n> to catch the previously missed transformation in 'promisor-remote.c'.\n\nRight. I have a feeling it is sucking in as much data as possible to a\nglobal symbol table, and then doing the analysis. At least that's how\nI'd implement it. ;)\n\n> These descriptions don't make it clear (to me) whether the two options\n> are orthogonal, exclusive, or something else. However, trying out various\n> combinations indicates that \"last one wins\", e.g.:\n\nOK, that's compelling evidence. In that case it wouldn't hurt to say:\n\n --all-includes --recursive-includes\n\nversus just\n\n --recursive-includes\n\nbut it also probably isn't doing anything. ;)\n\n-Peff\n"},{"id":"461949","messageId":"220825.86a67s4e6r.gmgdl@evledraar.gmail.com","threadId":"58347","inReplyTo":"YwXiKXONiTt+fIdi@coredump.intra.peff.net","subject":"Re: [PATCH v2] promisor-remote: fix xcalloc() argument order","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-08-25T13:51:21Z","receivedAt":"2022-08-25T13:59:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Aug 24 2022, Jeff King wrote:\n\n> On Tue, Aug 23, 2022 at 11:57:33AM +0200, SZEDER Gábor wrote:\n>\n>> Pass the number of elements first and their size second, as expected\n>> by xcalloc().\n>> \n>> Patch generated with:\n>> \n>> make SPATCH_FLAGS=--recursive-includes contrib/coccinelle/xcalloc.cocci.patch\n>\n> Thanks for digging here. I think it probably explains a lot of \"wait,\n> why did this result change\" head-scratching I did back when we started\n> batching a few years ago.\n>\n> Is there any reason we wouldn't want --recursive-includes to be added to\n> the default SPATCH_FLAGS?\n>\n> I suspect we'd still want to leave --all-includes there to get as much\n> type information as possible. If I understand correctly, the two are\n> orthogonal (one is \"follow includes of includes\" and the other is\n> \"follow system includes in angle brackets\").\n>\n> Doing so doesn't seem to find any other missed entries in the current\n> codebase, but I'm pretty sure there are some it would have caught in a\n> less mysterious fashion over the years.\n\nThis feels to me like hacks around other issues we should fix the root\ncause of.\n\nSo, first of all, I think this is a perfectly good fix, and something we\nshould do more of in general. It'll apply the wanted change *and* speed\nup the run:\n\t\n\tdiff --git a/contrib/coccinelle/xcalloc.cocci b/contrib/coccinelle/xcalloc.cocci\n\tindex c291011607e..bd51e33af83 100644\n\t--- a/contrib/coccinelle/xcalloc.cocci\n\t+++ b/contrib/coccinelle/xcalloc.cocci\n\t@@ -1,10 +1,8 @@\n\t @@\n\t-type T;\n\t-T *ptr;\n\t expression n;\n\t @@\n\t xcalloc(\n\t + n,\n\t- \\( sizeof(T) \\| sizeof(*ptr) \\)\n\t+ sizeof(...)\n\t - , n\n\t )\n\nBut it's *not* functionally the same thing, pedantically speaking, but I\nthink it's fine. I pointed this out at the the time in [1].\n\nAlso, more generally we could avoid includes in headers, and use forward\ndecls. This would make e.g. the \"iwyu\" tool report the missing include.\n\nUnfortunately it seems we added it in this case due to\n\"the_repository\". I wonder if this hack would be better:\n\n\tdiff --git a/Makefile b/Makefile\n\tindex 9410a587fc0..92d5726b392 100644\n\t--- a/Makefile\n\t+++ b/Makefile\n\t@@ -3119,6 +3119,7 @@ HCC = $(HCO:hco=hcc)\n\t \n\t %.hcc: %.h\n\t \t@echo '#include \"git-compat-util.h\"' >$@\n\t+\t@echo \"extern struct repository *the_repository;\" >>$@\n\t \t@echo '#include \"$<\"' >>$@\n\t \n\t $(HCO): %.hco: %.hcc FORCE\n\tdiff --git a/promisor-remote.h b/promisor-remote.h\n\tindex edc45ab0f5f..e864c093a44 100644\n\t--- a/promisor-remote.h\n\t+++ b/promisor-remote.h\n\t@@ -1,7 +1,7 @@\n\t #ifndef PROMISOR_REMOTE_H\n\t #define PROMISOR_REMOTE_H\n\t \n\t-#include \"repository.h\"\n\t+struct repository;\n\t \n\t struct object_id;\n\t \n\t@@ -23,6 +23,7 @@ static inline void promisor_remote_reinit(void)\n\t \trepo_promisor_remote_reinit(the_repository);\n\t }\n\t \n\t+struct promisor_remote_config;\n\t void promisor_remote_clear(struct promisor_remote_config *config);\n\t \n\t struct promisor_remote *repo_promisor_remote_find(struct repository *r, const char *remote_name);\n\nSo yeah, one way to deal with this is --recursive-includes, but I think\nwe're better off heading in the direction of having the includes for a\ngiven *.c file be what it actually needs, and not rely on includes by\nproxy, or in this case to pollute the namespace of everyone using\npromisor-remote.h with remote.h (probably doesn't matter in that case,\nbut in the general case...).\n\n1. https://lore.kernel.org/git/87ft18tcog.fsf@evledraar.gmail.com/\n"}]}