| Message ID | 20220822213408.662482-1-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | promisor-remote: fix xcalloc() argument order | expand |
SZEDER Gábor <szeder.dev@gmail.com> writes: > Pass the number of elements first and their size second, as expected > by xcalloc(). > > Patch generated with 'contrib/coccinelle/xcalloc.cocci' and Coccinelle > v1.0.7 or later (previous Coccinelle versions don't notice this). One thing is that Coccinelle is way too slow on our codebase, compared to the usual compilation, to run every time we make changes. Combined with the fact that our codebase is mostly clean, the cycles feel huge waste of time only to find something small like what this patch fixes. That sadly discourages us from doing "make coccicheck" more often as we should. I _think_ Googlers have 1.1.1 on their linux boxes, so even if our GitHub Actions CI is fixed to Ubuntu 18.04 and does not run more recent Coccinelle, we theoretically should have been able to catch it before it hit the public list, if "1.0.7 or later" was the condition. FWIW, "make coccicheck" with what I happen to have notices it. $ spatch version spatch version 1.1.1 compiled with OCaml version 4.13.1 Flags passed to the configure script: --prefix=/usr --sysconfdir=/etc --libdir=/usr/lib --enable-ocaml --enable-python --enable-opt OCaml scripting support: yes Python scripting support: yes Syntax of regular expressions: PCRE Anyway, the patch is correct. Thanks, will queue. > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> > --- > promisor-remote.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/promisor-remote.c b/promisor-remote.c > index 5b33f88bca..68f46f5ec7 100644 > --- a/promisor-remote.c > +++ b/promisor-remote.c > @@ -146,7 +146,7 @@ static void promisor_remote_init(struct repository *r) > if (r->promisor_remote_config) > return; > config = r->promisor_remote_config = > - xcalloc(sizeof(*r->promisor_remote_config), 1); > + xcalloc(1, sizeof(*r->promisor_remote_config)); > config->promisors_tail = &config->promisors; > > repo_config(r, promisor_remote_config, config);
Junio C Hamano <gitster@pobox.com> writes: > ... FWIW, "make coccicheck" with what I happen to have > notices it. Oops, that was a serious typo. "notices" -> "fails to notice". > $ spatch version > spatch version 1.1.1 compiled with OCaml version 4.13.1 > Flags passed to the configure script: --prefix=/usr --sysconfdir=/etc --libdir=/usr/lib --enable-ocaml --enable-python --enable-opt > OCaml scripting support: yes > Python scripting support: yes > Syntax of regular expressions: PCRE > > Anyway, the patch is correct. > > Thanks, will queue. > >> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> >> --- >> promisor-remote.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/promisor-remote.c b/promisor-remote.c >> index 5b33f88bca..68f46f5ec7 100644 >> --- a/promisor-remote.c >> +++ b/promisor-remote.c >> @@ -146,7 +146,7 @@ static void promisor_remote_init(struct repository *r) >> if (r->promisor_remote_config) >> return; >> config = r->promisor_remote_config = >> - xcalloc(sizeof(*r->promisor_remote_config), 1); >> + xcalloc(1, sizeof(*r->promisor_remote_config)); >> config->promisors_tail = &config->promisors; >> >> repo_config(r, promisor_remote_config, config);
On Mon, Aug 22, 2022 at 06:09:41PM -0700, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > ... FWIW, "make coccicheck" with what I happen to have > > notices it. > > Oops, that was a serious typo. "notices" -> "fails to notice". Hm, that's strange. 1.1.1 did notice this transformation for me, as did 1.1.0, 1.0.8, 1.0.7, and, contrary to what I wrote in the commit message, even 1.0.6 as well. (IIRC Ubuntu 18.04 has 1.0.4, which I didn't try, but if that could notice it, then we should have heard about it from CI.) $ spatch --version spatch version 1.1.1 compiled with OCaml version 4.08.1 Flags passed to the configure script: [none] OCaml scripting support: yes Python scripting support: yes Syntax of regular expressions: PCRE > > $ spatch version > > spatch version 1.1.1 compiled with OCaml version 4.13.1 > > Flags passed to the configure script: --prefix=/usr --sysconfdir=/etc --libdir=/usr/lib --enable-ocaml --enable-python --enable-opt > > OCaml scripting support: yes > > Python scripting support: yes > > Syntax of regular expressions: PCRE
On Tue, Aug 23, 2022 at 09:04:17AM +0200, SZEDER Gábor wrote: > On Mon, Aug 22, 2022 at 06:09:41PM -0700, Junio C Hamano wrote: > > Junio C Hamano <gitster@pobox.com> writes: > > > > > ... FWIW, "make coccicheck" with what I happen to have > > > notices it. > > > > Oops, that was a serious typo. "notices" -> "fails to notice". > > Hm, that's strange. 1.1.1 did notice this transformation for me, Hang on, it's not quite that simple. Watch this, it will get weird: # Batched spatch for faster processing, see 960154b9c1 (coccicheck: # optionally batch spatch invocations, 2019-05-06) $ grep SPATCH_BATCH_SIZE config.mak SPATCH_BATCH_SIZE = 32 $ time make contrib/coccinelle/xcalloc.cocci.patch SPATCH contrib/coccinelle/xcalloc.cocci SPATCH result: contrib/coccinelle/xcalloc.cocci.patch real 0m35.902s user 0m34.840s sys 0m1.030s # Found it, good. $ make -s cocciclean # Turn off batched processing (this is the default) $ sed -i -e '/SPATCH_BATCH_SIZE/ s/= .*/= 1/' config.mak $ time make contrib/coccinelle/xcalloc.cocci.patch SPATCH contrib/coccinelle/xcalloc.cocci real 0m24.553s user 0m21.468s sys 0m3.099s # Not only missed the transformation, but it's faster than batched # processing?! # Let's invoke spatch directly. $ spatch --all-includes --sp-file contrib/coccinelle/xcalloc.cocci --patch . promisor-remote.c warning: Can't find macro file: /usr/local/bin/lib/coccinelle/standard.h warning: Can't find default iso file: /usr/local/bin/lib/coccinelle/standard.iso HANDLING: promisor-remote.c # Nope. $ spatch --all-includes --sp-file contrib/coccinelle/xcalloc.cocci --patch . promisor-remote.c lockfile.c warning: Can't find macro file: /usr/local/bin/lib/coccinelle/standard.h warning: Can't find default iso file: /usr/local/bin/lib/coccinelle/standard.iso HANDLING: promisor-remote.c lockfile.c $ spatch --all-includes --sp-file contrib/coccinelle/xcalloc.cocci --patch . promisor-remote.c git.c warning: Can't find macro file: /usr/local/bin/lib/coccinelle/standard.h warning: Can't find default iso file: /usr/local/bin/lib/coccinelle/standard.iso HANDLING: promisor-remote.c git.c $ spatch --all-includes --sp-file contrib/coccinelle/xcalloc.cocci --patch . promisor-remote.c usage.c warning: Can't find macro file: /usr/local/bin/lib/coccinelle/standard.h warning: Can't find default iso file: /usr/local/bin/lib/coccinelle/standard.iso HANDLING: promisor-remote.c usage.c # Nope, nope, nope. $ spatch --all-includes --sp-file contrib/coccinelle/xcalloc.cocci --patch . promisor-remote.c builtin/*.c [...] # Nope! # But watch this! $ spatch --all-includes --sp-file contrib/coccinelle/xcalloc.cocci --patch . promisor-remote.c config.c warning: Can't find macro file: /usr/local/bin/lib/coccinelle/standard.h warning: Can't find default iso file: /usr/local/bin/lib/coccinelle/standard.iso HANDLING: promisor-remote.c config.c diff = diff -u -p a/promisor-remote.c b/promisor-remote.c --- a/promisor-remote.c +++ b/promisor-remote.c @@ -146,7 +146,7 @@ static void promisor_remote_init(struct if (r->promisor_remote_config) return; config = r->promisor_remote_config = - xcalloc(sizeof(*r->promisor_remote_config), 1); + xcalloc(1, sizeof(*r->promisor_remote_config)); config->promisors_tail = &config->promisors; repo_config(r, promisor_remote_config, config); # Huh?! (; FWIW, I see this with Coccinelle 1.1.1, 1.0.8 and 1.0.6 as well.
On Tue, Aug 23, 2022 at 11:21:56AM +0200, SZEDER Gábor wrote: > On Tue, Aug 23, 2022 at 09:04:17AM +0200, SZEDER Gábor wrote: > > On Mon, Aug 22, 2022 at 06:09:41PM -0700, Junio C Hamano wrote: > > > Junio C Hamano <gitster@pobox.com> writes: > > > > > > > ... FWIW, "make coccicheck" with what I happen to have > > > > notices it. > > > > > > Oops, that was a serious typo. "notices" -> "fails to notice". > > > > Hm, that's strange. 1.1.1 did notice this transformation for me, > # Let's invoke spatch directly. > $ spatch --all-includes --sp-file contrib/coccinelle/xcalloc.cocci --patch . promisor-remote.c > warning: Can't find macro file: /usr/local/bin/lib/coccinelle/standard.h > warning: Can't find default iso file: /usr/local/bin/lib/coccinelle/standard.iso > HANDLING: promisor-remote.c > # Nope. > $ spatch --all-includes --sp-file contrib/coccinelle/xcalloc.cocci --patch . promisor-remote.c lockfile.c > warning: Can't find macro file: /usr/local/bin/lib/coccinelle/standard.h > warning: Can't find default iso file: /usr/local/bin/lib/coccinelle/standard.iso > HANDLING: promisor-remote.c lockfile.c > $ spatch --all-includes --sp-file contrib/coccinelle/xcalloc.cocci --patch . promisor-remote.c git.c > warning: Can't find macro file: /usr/local/bin/lib/coccinelle/standard.h > warning: Can't find default iso file: /usr/local/bin/lib/coccinelle/standard.iso > HANDLING: promisor-remote.c git.c > $ spatch --all-includes --sp-file contrib/coccinelle/xcalloc.cocci --patch . promisor-remote.c usage.c > warning: Can't find macro file: /usr/local/bin/lib/coccinelle/standard.h > warning: Can't find default iso file: /usr/local/bin/lib/coccinelle/standard.iso > HANDLING: promisor-remote.c usage.c > # Nope, nope, nope. > $ spatch --all-includes --sp-file contrib/coccinelle/xcalloc.cocci --patch . promisor-remote.c builtin/*.c > [...] > # Nope! > > # But watch this! > $ spatch --all-includes --sp-file contrib/coccinelle/xcalloc.cocci --patch . promisor-remote.c config.c > warning: Can't find macro file: /usr/local/bin/lib/coccinelle/standard.h > warning: Can't find default iso file: /usr/local/bin/lib/coccinelle/standard.iso > HANDLING: promisor-remote.c config.c > diff = > diff -u -p a/promisor-remote.c b/promisor-remote.c > --- a/promisor-remote.c > +++ b/promisor-remote.c > @@ -146,7 +146,7 @@ static void promisor_remote_init(struct > if (r->promisor_remote_config) > return; > config = r->promisor_remote_config = > - xcalloc(sizeof(*r->promisor_remote_config), 1); > + xcalloc(1, sizeof(*r->promisor_remote_config)); > config->promisors_tail = &config->promisors; > > repo_config(r, promisor_remote_config, config); > > # Huh?! (; > > FWIW, I see this with Coccinelle 1.1.1, 1.0.8 and 1.0.6 as well. Ok, I think I got this. The transformation involves 'r->promisor_remote_config', where 'r' is a 'struct repository*'. However, 'promisor-remote.c' doesn't include 'repository.h' directly: $ grep '^#include' promisor-remote.c #include "cache.h" #include "object-store.h" #include "promisor-remote.h" #include "config.h" #include "transport.h" #include "strvec.h" If I add an '#include "repository.h" to 'promisor-remote.c', then Coccinelle finds the transformation even when it processes this source file on is own: $ spatch --all-includes --sp-file contrib/coccinelle/xcalloc.cocci --patch . promisor-remote.c init_defs_builtins: /usr/local/lib/coccinelle/standard.h HANDLING: promisor-remote.c diff = diff -u -p a/promisor-remote.c b/promisor-remote.c --- a/promisor-remote.c +++ b/promisor-remote.c @@ -149,7 +149,7 @@ static void promisor_remote_init(struct if (r->promisor_remote_config) return; config = r->promisor_remote_config = - xcalloc(sizeof(*r->promisor_remote_config), 1); + xcalloc(1, sizeof(*r->promisor_remote_config)); config->promisors_tail = &config->promisors; repo_config(r, promisor_remote_config, config);
diff --git a/promisor-remote.c b/promisor-remote.c index 5b33f88bca..68f46f5ec7 100644 --- a/promisor-remote.c +++ b/promisor-remote.c @@ -146,7 +146,7 @@ static void promisor_remote_init(struct repository *r) if (r->promisor_remote_config) return; config = r->promisor_remote_config = - xcalloc(sizeof(*r->promisor_remote_config), 1); + xcalloc(1, sizeof(*r->promisor_remote_config)); config->promisors_tail = &config->promisors; repo_config(r, promisor_remote_config, config);
Pass the number of elements first and their size second, as expected by xcalloc(). Patch generated with 'contrib/coccinelle/xcalloc.cocci' and Coccinelle v1.0.7 or later (previous Coccinelle versions don't notice this). Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> --- promisor-remote.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)