git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] Add MEMZERO_ARRAY() macro and use it in coccinelle
@ 2025-12-10 13:13 Toon Claes
  2025-12-10 13:13 ` [PATCH 1/2] git-compat-util: introduce MEMZERO_ARRAY() macro Toon Claes
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Toon Claes @ 2025-12-10 13:13 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Toon Claes

A bug was found[1] in git-last-modified(1), caused by uninitialized
memory. While the bug is fixed, in the discussion after that[2] the
suggestion was made to introduce a macro that simplifies zeroing a
dynamically allocated array.

In the first patch I'm addressing the outcome of the discussion on the
patch, and in the second patch I'm fixing an edge-case I've encountered
while using coccinelle.

There's one /oddball/ in add-patch.c that doesn't get caught by the
coccinelle rules, around line 960:

    memset(hunk + 1, 0, (splittable_into - 1) * sizeof(*hunk));

Because there's some quirky pointer math going on, it think it's better
to keep it like it is.

There were some mixed opinions about naming it either CLEAR_ARRAY() or
MEMZERO_ARRAY(). I choose the latter because I wanted to avoid confusion
that "clear" would shrink the array to zero elements.

[1]: <4dc4c8cd-c0cc-4784-8fcf-defa3a051087@mit.edu>
[2]: <20251208201501.GA216526@coredump.intra.peff.net>

Signed-off-by: Toon Claes <toon@iotcl.com>
---
Toon Claes (2):
      git-compat-util: introduce MEMZERO_ARRAY() macro
      contrib/coccinelle: pass include paths to spatch(1)

 Makefile                       |  2 +-
 builtin/last-modified.c        |  2 +-
 compat/simple-ipc/ipc-win32.c  |  2 +-
 contrib/coccinelle/array.cocci | 20 ++++++++++++++++++++
 contrib/coccinelle/meson.build |  6 ++++++
 diff-delta.c                   |  2 +-
 ewah/bitmap.c                  |  7 +++----
 git-compat-util.h              |  1 +
 hashmap.c                      |  2 +-
 pack-revindex.c                |  2 +-
 10 files changed, 36 insertions(+), 10 deletions(-)



---
base-commit: 011ce54c26318d725db1d8971d157656eb965d88
change-id: 20251210-toon-cocci-memzero-2b6185e08ac4


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/2] git-compat-util: introduce MEMZERO_ARRAY() macro
  2025-12-10 13:13 [PATCH 0/2] Add MEMZERO_ARRAY() macro and use it in coccinelle Toon Claes
@ 2025-12-10 13:13 ` Toon Claes
  2025-12-11  3:18   ` Junio C Hamano
  2025-12-10 13:13 ` [PATCH 2/2] contrib/coccinelle: pass include paths to spatch(1) Toon Claes
  2025-12-13  0:18 ` [PATCH 0/2] Add MEMZERO_ARRAY() macro and use it in coccinelle Junio C Hamano
  2 siblings, 1 reply; 10+ messages in thread
From: Toon Claes @ 2025-12-10 13:13 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Toon Claes

Introduce a new macro MEMZERO_ARRAY() that zeroes the memory allocated
by ALLOC_ARRAY() and friends. And add coccinelle rule to enforce the use
of this macro.

Signed-off-by: Toon Claes <toon@iotcl.com>
---
 builtin/last-modified.c        |  2 +-
 compat/simple-ipc/ipc-win32.c  |  2 +-
 contrib/coccinelle/array.cocci | 20 ++++++++++++++++++++
 diff-delta.c                   |  2 +-
 ewah/bitmap.c                  |  7 +++----
 git-compat-util.h              |  1 +
 hashmap.c                      |  2 +-
 pack-revindex.c                |  2 +-
 8 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/builtin/last-modified.c b/builtin/last-modified.c
index cc5fd2e795..ac5387e861 100644
--- a/builtin/last-modified.c
+++ b/builtin/last-modified.c
@@ -327,7 +327,7 @@ static void process_parent(struct last_modified *lm,
 	if (!(parent->object.flags & PARENT1))
 		active_paths_free(lm, parent);
 
-	memset(lm->scratch->words, 0x0, lm->scratch->word_alloc * sizeof(eword_t));
+	MEMZERO_ARRAY(lm->scratch->words, lm->scratch->word_alloc);
 	diff_queue_clear(&diff_queued_diff);
 }
 
diff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c
index a8fc812adf..4a3e7df9c7 100644
--- a/compat/simple-ipc/ipc-win32.c
+++ b/compat/simple-ipc/ipc-win32.c
@@ -686,7 +686,7 @@ static LPSECURITY_ATTRIBUTES get_sa(struct my_sa_data *d)
 		goto fail;
 	}
 
-	memset(ea, 0, NR_EA * sizeof(EXPLICIT_ACCESS));
+	MEMZERO_ARRAY(ea, NR_EA);
 
 	ea[0].grfAccessPermissions = GENERIC_READ | GENERIC_WRITE;
 	ea[0].grfAccessMode = SET_ACCESS;
diff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci
index 27a3b479c9..d306f6a21e 100644
--- a/contrib/coccinelle/array.cocci
+++ b/contrib/coccinelle/array.cocci
@@ -101,3 +101,23 @@ expression dst, src, n;
 -ALLOC_ARRAY(dst, n);
 -COPY_ARRAY(dst, src, n);
 +DUP_ARRAY(dst, src, n);
+
+@@
+type T;
+T *ptr;
+expression n;
+@@
+- memset(ptr, \( 0x0 \| 0 \), n * \( sizeof(T)
+-                                 \| sizeof(*ptr)
+-                                 \) )
++ MEMZERO_ARRAY(ptr, n)
+
+@@
+type T;
+T[] ptr;
+expression n;
+@@
+- memset(ptr, \( 0x0 \| 0 \), n * \( sizeof(T)
+-                                 \| sizeof(*ptr)
+-                                 \) )
++ MEMZERO_ARRAY(ptr, n)
diff --git a/diff-delta.c b/diff-delta.c
index 71d37368d6..43c339f010 100644
--- a/diff-delta.c
+++ b/diff-delta.c
@@ -171,7 +171,7 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize)
 	mem = hash + hsize;
 	entry = mem;
 
-	memset(hash, 0, hsize * sizeof(*hash));
+	MEMZERO_ARRAY(hash, hsize);
 
 	/* allocate an array to count hash entries */
 	hash_count = calloc(hsize, sizeof(*hash_count));
diff --git a/ewah/bitmap.c b/ewah/bitmap.c
index 55928dada8..bf878bf876 100644
--- a/ewah/bitmap.c
+++ b/ewah/bitmap.c
@@ -46,8 +46,7 @@ static void bitmap_grow(struct bitmap *self, size_t word_alloc)
 {
 	size_t old_size = self->word_alloc;
 	ALLOC_GROW(self->words, word_alloc, self->word_alloc);
-	memset(self->words + old_size, 0x0,
-	       (self->word_alloc - old_size) * sizeof(eword_t));
+	MEMZERO_ARRAY(self->words + old_size, (self->word_alloc - old_size));
 }
 
 void bitmap_set(struct bitmap *self, size_t pos)
@@ -192,8 +191,8 @@ void bitmap_or_ewah(struct bitmap *self, struct ewah_bitmap *other)
 	if (self->word_alloc < other_final) {
 		self->word_alloc = other_final;
 		REALLOC_ARRAY(self->words, self->word_alloc);
-		memset(self->words + original_size, 0x0,
-			(self->word_alloc - original_size) * sizeof(eword_t));
+		MEMZERO_ARRAY(self->words + original_size,
+		              (self->word_alloc - original_size));
 	}
 
 	ewah_iterator_init(&it, other);
diff --git a/git-compat-util.h b/git-compat-util.h
index 398e0fac4f..2b8192fd2e 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -726,6 +726,7 @@ static inline uint64_t u64_add(uint64_t a, uint64_t b)
 #define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), (alloc)))
 #define CALLOC_ARRAY(x, alloc) (x) = xcalloc((alloc), sizeof(*(x)))
 #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), st_mult(sizeof(*(x)), (alloc)))
+#define MEMZERO_ARRAY(x, alloc) memset((x), 0x0, st_mult(sizeof(*(x)), (alloc)))
 
 #define COPY_ARRAY(dst, src, n) copy_array((dst), (src), (n), sizeof(*(dst)) + \
 	BARF_UNLESS_COPYABLE((dst), (src)))
diff --git a/hashmap.c b/hashmap.c
index a711377853..3b5d6f14bc 100644
--- a/hashmap.c
+++ b/hashmap.c
@@ -194,7 +194,7 @@ void hashmap_partial_clear_(struct hashmap *map, ssize_t entry_offset)
 		return;
 	if (entry_offset >= 0)  /* called by hashmap_clear_entries */
 		free_individual_entries(map, entry_offset);
-	memset(map->table, 0, map->tablesize * sizeof(struct hashmap_entry *));
+	MEMZERO_ARRAY(map->table, map->tablesize);
 	map->shrink_at = 0;
 	map->private_size = 0;
 }
diff --git a/pack-revindex.c b/pack-revindex.c
index d0791cc493..8598b941c8 100644
--- a/pack-revindex.c
+++ b/pack-revindex.c
@@ -75,7 +75,7 @@ static void sort_revindex(struct revindex_entry *entries, unsigned n, off_t max)
 	for (bits = 0; max >> bits; bits += DIGIT_SIZE) {
 		unsigned i;
 
-		memset(pos, 0, BUCKETS * sizeof(*pos));
+		MEMZERO_ARRAY(pos, BUCKETS);
 
 		/*
 		 * We want pos[i] to store the index of the last element that

-- 
2.52.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] contrib/coccinelle: pass include paths to spatch(1)
  2025-12-10 13:13 [PATCH 0/2] Add MEMZERO_ARRAY() macro and use it in coccinelle Toon Claes
  2025-12-10 13:13 ` [PATCH 1/2] git-compat-util: introduce MEMZERO_ARRAY() macro Toon Claes
@ 2025-12-10 13:13 ` Toon Claes
  2025-12-11  6:19   ` Patrick Steinhardt
  2025-12-13  1:22   ` Junio C Hamano
  2025-12-13  0:18 ` [PATCH 0/2] Add MEMZERO_ARRAY() macro and use it in coccinelle Junio C Hamano
  2 siblings, 2 replies; 10+ messages in thread
From: Toon Claes @ 2025-12-10 13:13 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Toon Claes

In the previous commit a new coccinelle rule is added. But neiter
`make coccicheck` nor `meson compile coccicheck` did detect a case in
builtin/last-modified.c.

This case involves the field `scratch` in `struct last_modified`. This
field is of type `struct bitmap` and that struct has a member
`eword_t *words`. Both are defined in `ewah/ewok.h`. Now, while
builtin/last-modified.c does include that header (with the subdir in the
#include directive), it seems coccinelle does not process it. So it's
unaware of the type of `words` in the bitmap, and it doesn't recognize
the rule from previous commit that uses:

    type T;
    T *ptr;

Fix coccicheck by passing all possible include paths inside the Git
project so spatch(1) can find the headers and can determine the types.

Signed-off-by: Toon Claes <toon@iotcl.com>
---
 Makefile                       | 2 +-
 contrib/coccinelle/meson.build | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 6fc322ff88..46d07b2d52 100644
--- a/Makefile
+++ b/Makefile
@@ -981,7 +981,7 @@ SANITIZE_LEAK =
 SANITIZE_ADDRESS =
 
 # For the 'coccicheck' target
-SPATCH_INCLUDE_FLAGS = --all-includes
+SPATCH_INCLUDE_FLAGS = --all-includes $(addprefix -I ,compat ewah refs sha256 trace2 win32 xdiff)
 SPATCH_FLAGS =
 SPATCH_TEST_FLAGS =
 
diff --git a/contrib/coccinelle/meson.build b/contrib/coccinelle/meson.build
index dc3f73c2e7..ae7f5b5460 100644
--- a/contrib/coccinelle/meson.build
+++ b/contrib/coccinelle/meson.build
@@ -50,6 +50,11 @@ foreach header : headers_to_check
   coccinelle_headers += meson.project_source_root() / header
 endforeach
 
+coccinelle_includes = []
+foreach path : ['compat', 'ewah', 'refs', 'sha256', 'trace2', 'win32', 'xdiff']
+  coccinelle_includes += ['-I', meson.project_source_root() / path]
+endforeach
+
 patches = [ ]
 foreach source : coccinelle_sources
   patches += custom_target(
@@ -58,6 +63,7 @@ foreach source : coccinelle_sources
       '--all-includes',
       '--sp-file', concatenated_rules,
       '--patch', meson.project_source_root(),
+      coccinelle_includes,
       '@INPUT@',
     ],
     input: meson.project_source_root() / source,

-- 
2.52.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] git-compat-util: introduce MEMZERO_ARRAY() macro
  2025-12-10 13:13 ` [PATCH 1/2] git-compat-util: introduce MEMZERO_ARRAY() macro Toon Claes
@ 2025-12-11  3:18   ` Junio C Hamano
  2025-12-12 13:02     ` René Scharfe
  0 siblings, 1 reply; 10+ messages in thread
From: Junio C Hamano @ 2025-12-11  3:18 UTC (permalink / raw)
  To: Toon Claes; +Cc: git, Jeff King

Toon Claes <toon@iotcl.com> writes:

> +@@
> +- memset(ptr, \( 0x0 \| 0 \), n * \( sizeof(T)
> +-                                 \| sizeof(*ptr)
> +-                                 \) )
> ++ MEMZERO_ARRAY(ptr, n)

Shouldn't we be also catching

	memset(array, '\0', sizeof(array[0]) * ARRAY_SIZE(array));

in addition to "0" and "0x0"?

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] contrib/coccinelle: pass include paths to spatch(1)
  2025-12-10 13:13 ` [PATCH 2/2] contrib/coccinelle: pass include paths to spatch(1) Toon Claes
@ 2025-12-11  6:19   ` Patrick Steinhardt
  2025-12-12  5:04     ` Junio C Hamano
  2025-12-13  1:22   ` Junio C Hamano
  1 sibling, 1 reply; 10+ messages in thread
From: Patrick Steinhardt @ 2025-12-11  6:19 UTC (permalink / raw)
  To: Toon Claes; +Cc: git, Jeff King

On Wed, Dec 10, 2025 at 02:13:02PM +0100, Toon Claes wrote:
> In the previous commit a new coccinelle rule is added. But neiter
> `make coccicheck` nor `meson compile coccicheck` did detect a case in
> builtin/last-modified.c.
> 
> This case involves the field `scratch` in `struct last_modified`. This
> field is of type `struct bitmap` and that struct has a member
> `eword_t *words`. Both are defined in `ewah/ewok.h`. Now, while
> builtin/last-modified.c does include that header (with the subdir in the
> #include directive), it seems coccinelle does not process it. So it's
> unaware of the type of `words` in the bitmap, and it doesn't recognize
> the rule from previous commit that uses:
> 
>     type T;
>     T *ptr;
> 
> Fix coccicheck by passing all possible include paths inside the Git
> project so spatch(1) can find the headers and can determine the types.
> 
> Signed-off-by: Toon Claes <toon@iotcl.com>
> ---
>  Makefile                       | 2 +-
>  contrib/coccinelle/meson.build | 6 ++++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index 6fc322ff88..46d07b2d52 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -981,7 +981,7 @@ SANITIZE_LEAK =
>  SANITIZE_ADDRESS =
>  
>  # For the 'coccicheck' target
> -SPATCH_INCLUDE_FLAGS = --all-includes
> +SPATCH_INCLUDE_FLAGS = --all-includes $(addprefix -I ,compat ewah refs sha256 trace2 win32 xdiff)

This feels weird to me. We never pass any of these includes to the
compiler, either. So why should Coccinelle require them?

Coming back to your example of `eword_t`, Git knows to always include
"ewah/ewok.h", and that include is relative to the root directory of Git
itself. And as the header doesn't have any includes itself, this cannot
be the root cause, either.

So I'm a bit puzzled why this patch would fix the observed issue.

Patrick

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] contrib/coccinelle: pass include paths to spatch(1)
  2025-12-11  6:19   ` Patrick Steinhardt
@ 2025-12-12  5:04     ` Junio C Hamano
  0 siblings, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2025-12-12  5:04 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: Toon Claes, git, Jeff King

Patrick Steinhardt <ps@pks.im> writes:

>>  # For the 'coccicheck' target
>> -SPATCH_INCLUDE_FLAGS = --all-includes
>> +SPATCH_INCLUDE_FLAGS = --all-includes $(addprefix -I ,compat ewah refs sha256 trace2 win32 xdiff)
>
> This feels weird to me. We never pass any of these includes to the
> compiler, either. So why should Coccinelle require them?
>
> Coming back to your example of `eword_t`, Git knows to always include
> "ewah/ewok.h", and that include is relative to the root directory of Git
> itself. And as the header doesn't have any includes itself, this cannot
> be the root cause, either.
>
> So I'm a bit puzzled why this patch would fix the observed issue.

Indeed it is puzzling..

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] git-compat-util: introduce MEMZERO_ARRAY() macro
  2025-12-11  3:18   ` Junio C Hamano
@ 2025-12-12 13:02     ` René Scharfe
  2025-12-19  9:17       ` Toon Claes
  0 siblings, 1 reply; 10+ messages in thread
From: René Scharfe @ 2025-12-12 13:02 UTC (permalink / raw)
  To: Junio C Hamano, Toon Claes; +Cc: git, Jeff King

On 12/11/25 4:18 AM, Junio C Hamano wrote:
> Toon Claes <toon@iotcl.com> writes:
> 
>> +@@
>> +- memset(ptr, \( 0x0 \| 0 \), n * \( sizeof(T)
>> +-                                 \| sizeof(*ptr)
>> +-                                 \) )
>> ++ MEMZERO_ARRAY(ptr, n)
> 
> Shouldn't we be also catching
> 
> 	memset(array, '\0', sizeof(array[0]) * ARRAY_SIZE(array));
> 
> in addition to "0" and "0x0"?

Good idea to match "sizeof(ptr[...])", even though we currently don't have
matching code.

Good idea also to match "'\0'".  There's code with that pattern in
compat/regex/.

You can drop "0x0", though, "0" matches it already (at least for me, I have
"spatch version 1.3-dirty compiled with OCaml version 5.1.1" from Homebrew).

If you put parentheses around "n" in the pre-image then Coccinelle will
remove them if present and still match code without them.  They are no
longer needed without the multiplication.  Their removal would improve the
result for ewah/bitmap.c.

René


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/2] Add MEMZERO_ARRAY() macro and use it in coccinelle
  2025-12-10 13:13 [PATCH 0/2] Add MEMZERO_ARRAY() macro and use it in coccinelle Toon Claes
  2025-12-10 13:13 ` [PATCH 1/2] git-compat-util: introduce MEMZERO_ARRAY() macro Toon Claes
  2025-12-10 13:13 ` [PATCH 2/2] contrib/coccinelle: pass include paths to spatch(1) Toon Claes
@ 2025-12-13  0:18 ` Junio C Hamano
  2 siblings, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2025-12-13  0:18 UTC (permalink / raw)
  To: Toon Claes; +Cc: git, Jeff King

Toon Claes <toon@iotcl.com> writes:

> In the first patch I'm addressing the outcome of the discussion on the
> patch, and in the second patch I'm fixing an edge-case I've encountered
> while using coccinelle.


Without the attached, the failure at CI is impossible to diagnose.
A test that emits "you got some error messages", without showing
what they are, is useless.

 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git i/Makefile w/Makefile
index 0bd502dc01..52e413f59a 100644
--- i/Makefile
+++ w/Makefile
@@ -3534,7 +3534,7 @@ else
 COCCICHECK_PATCH_MUST_BE_EMPTY_FILES = $(COCCICHECK_PATCHES_INTREE)
 endif
 coccicheck: $(COCCICHECK_PATCH_MUST_BE_EMPTY_FILES)
-	! grep -q ^ $(COCCICHECK_PATCH_MUST_BE_EMPTY_FILES) /dev/null
+	! grep ^ $(COCCICHECK_PATCH_MUST_BE_EMPTY_FILES) /dev/null
 
 # See contrib/coccinelle/README
 coccicheck-pending: coccicheck-test


And FYI, with this series merged in, I get the following from
'seen'.  I did not check if they contain any false positives (in
which case the new coccinelle rules for this rewrite may have to be
marked as "pending", not for real checking), or they are real
improvements we should adopt.

Thanks.


diff -u -p a/diffcore-delta.c b/diffcore-delta.c
--- a/diffcore-delta.c
+++ b/diffcore-delta.c
@@ -56,7 +56,7 @@ static struct spanhash_top *spanhash_reh
 			     st_mult(sizeof(struct spanhash), sz)));
 	new_spanhash->alloc_log2 = orig->alloc_log2 + 1;
 	new_spanhash->free = INITIAL_FREE(new_spanhash->alloc_log2);
-	memset(new_spanhash->data, 0, sizeof(struct spanhash) * sz);
+	MEMZERO_ARRAY(new_spanhash->data, sz);
 	for (i = 0; i < osz; i++) {
 		struct spanhash *o = &(orig->data[i]);
 		int bucket;
@@ -135,7 +135,7 @@ static struct spanhash_top *hash_chars(s
 			      st_mult(sizeof(struct spanhash), (size_t)1 << i)));
 	hash->alloc_log2 = i;
 	hash->free = INITIAL_FREE(i);
-	memset(hash->data, 0, sizeof(struct spanhash) * ((size_t)1 << i));
+	MEMZERO_ARRAY(hash->data, ((size_t)1 << i));
 
 	n = 0;
 	accum1 = accum2 = 0;
diff -u -p a/linear-assignment.c b/linear-assignment.c
--- a/linear-assignment.c
+++ b/linear-assignment.c
@@ -20,8 +20,8 @@ void compute_assignment(int column_count
 	int i, j, phase;
 
 	if (column_count < 2) {
-		memset(column2row, 0, sizeof(int) * column_count);
-		memset(row2column, 0, sizeof(int) * row_count);
+		MEMZERO_ARRAY(column2row, column_count);
+		MEMZERO_ARRAY(row2column, row_count);
 		return;
 	}
 
diff -u -p a/shallow.c b/shallow.c
--- a/shallow.c
+++ b/shallow.c
@@ -745,7 +745,7 @@ void assign_shallow_commits_to_refs(stru
 
 	if (used) {
 		int bitmap_size = DIV_ROUND_UP(pi.nr_bits, 32) * sizeof(uint32_t);
-		memset(used, 0, sizeof(*used) * info->shallow->nr);
+		MEMZERO_ARRAY(used, info->shallow->nr);
 		for (i = 0; i < nr_shallow; i++) {
 			const struct commit *c = lookup_commit(the_repository,
 							       &oid[shallow[i]]);
@@ -810,7 +810,7 @@ static void post_assign_shallow(struct s
 
 	trace_printf_key(&trace_shallow, "shallow: post_assign_shallow\n");
 	if (ref_status)
-		memset(ref_status, 0, sizeof(*ref_status) * info->ref->nr);
+		MEMZERO_ARRAY(ref_status, info->ref->nr);
 
 	/* Remove unreachable shallow commits from "theirs" */
 	for (i = dst = 0; i < info->nr_theirs; i++) {

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] contrib/coccinelle: pass include paths to spatch(1)
  2025-12-10 13:13 ` [PATCH 2/2] contrib/coccinelle: pass include paths to spatch(1) Toon Claes
  2025-12-11  6:19   ` Patrick Steinhardt
@ 2025-12-13  1:22   ` Junio C Hamano
  1 sibling, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2025-12-13  1:22 UTC (permalink / raw)
  To: Toon Claes, René Scharfe; +Cc: git, Jeff King

Toon Claes <toon@iotcl.com> writes:

> In the previous commit a new coccinelle rule is added. But neiter
> `make coccicheck` nor `meson compile coccicheck` did detect a case in
> builtin/last-modified.c.

I can reproduce this.  I started with only git-compat-util.h and
contrib/coccinelle/array.cocci from your [1/2] and without [2/2],
and "make coccicheck" produced all other changes contained in [1/2]
and the leftover changes to diffcore-delta.c, linear-assignment.c
and shallow.c I reported earlier in a separate message, but the one
in last-modified.c is left intact.  There are successful rewrites
that involve eword_t in other files, so I am not sure what the
problem is.

I used the following instead of your [1/2], as suggested by René in
an earlier exchange.  I did not see any changes but I did not expect
to, either.


 contrib/coccinelle/array.cocci | 22 ++++++++++++++++++++++
 git-compat-util.h              |  1 +
 2 files changed, 23 insertions(+)

diff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci
index 27a3b479c9..ae23114b68 100644
--- a/contrib/coccinelle/array.cocci
+++ b/contrib/coccinelle/array.cocci
@@ -101,3 +101,25 @@ expression dst, src, n;
 -ALLOC_ARRAY(dst, n);
 -COPY_ARRAY(dst, src, n);
 +DUP_ARRAY(dst, src, n);
+
+@@
+type T;
+T *ptr;
+expression n;
+@@
+- memset(ptr, \( '\0' \| 0 \), n * \( sizeof(T)
+-                                  \| sizeof(*ptr)
+-                                  \| sizeof(ptr[0])
+-                                  \) )
++ MEMZERO_ARRAY(ptr, n)
+
+@@
+type T;
+T[] ptr;
+expression n;
+@@
+- memset(ptr, \( '\0' \| 0 \), n * \( sizeof(T)
+-                                  \| sizeof(*ptr)
+-                                  \| sizeof(ptr[0])
+-                                  \) )
++ MEMZERO_ARRAY(ptr, n)
diff --git a/git-compat-util.h b/git-compat-util.h
index 398e0fac4f..2b8192fd2e 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -726,6 +726,7 @@ static inline uint64_t u64_add(uint64_t a, uint64_t b)
 #define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), (alloc)))
 #define CALLOC_ARRAY(x, alloc) (x) = xcalloc((alloc), sizeof(*(x)))
 #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), st_mult(sizeof(*(x)), (alloc)))
+#define MEMZERO_ARRAY(x, alloc) memset((x), 0x0, st_mult(sizeof(*(x)), (alloc)))
 
 #define COPY_ARRAY(dst, src, n) copy_array((dst), (src), (n), sizeof(*(dst)) + \
 	BARF_UNLESS_COPYABLE((dst), (src)))



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] git-compat-util: introduce MEMZERO_ARRAY() macro
  2025-12-12 13:02     ` René Scharfe
@ 2025-12-19  9:17       ` Toon Claes
  0 siblings, 0 replies; 10+ messages in thread
From: Toon Claes @ 2025-12-19  9:17 UTC (permalink / raw)
  To: René Scharfe, Junio C Hamano; +Cc: git, Jeff King

René Scharfe <l.s.r@web.de> writes:

> On 12/11/25 4:18 AM, Junio C Hamano wrote:
>> Toon Claes <toon@iotcl.com> writes:
>> 
>>> +@@
>>> +- memset(ptr, \( 0x0 \| 0 \), n * \( sizeof(T)
>>> +-                                 \| sizeof(*ptr)
>>> +-                                 \) )
>>> ++ MEMZERO_ARRAY(ptr, n)
>> 
>> Shouldn't we be also catching
>> 
>> 	memset(array, '\0', sizeof(array[0]) * ARRAY_SIZE(array));
>> 
>> in addition to "0" and "0x0"?
>
> Good idea to match "sizeof(ptr[...])", even though we currently don't have
> matching code.

I didn't include that because I didn't see any case that would be
covered by that. But it's good to include it anyway to capture future
code.

> Good idea also to match "'\0'".  There's code with that pattern in
> compat/regex/.

Good find, I didn't think about that.

> You can drop "0x0", though, "0" matches it already (at least for me, I have
> "spatch version 1.3-dirty compiled with OCaml version 5.1.1" from
> Homebrew).

Nice!

> If you put parentheses around "n" in the pre-image then Coccinelle will
> remove them if present and still match code without them.

Well, that's not what I am seeing. With parentheses around "n", it
didn't match the case in builtin/last-modified.c on my machine. I'm
using spatch v1.3

> They are no longer needed without the multiplication. Their removal
> would improve the result for ewah/bitmap.c.

I agree it would be an improvement. Using `\( (n) \| n \)` does the
trick for all cases I've found.


-- 
Cheers,
Toon

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2025-12-19  9:17 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-12-10 13:13 [PATCH 0/2] Add MEMZERO_ARRAY() macro and use it in coccinelle Toon Claes
2025-12-10 13:13 ` [PATCH 1/2] git-compat-util: introduce MEMZERO_ARRAY() macro Toon Claes
2025-12-11  3:18   ` Junio C Hamano
2025-12-12 13:02     ` René Scharfe
2025-12-19  9:17       ` Toon Claes
2025-12-10 13:13 ` [PATCH 2/2] contrib/coccinelle: pass include paths to spatch(1) Toon Claes
2025-12-11  6:19   ` Patrick Steinhardt
2025-12-12  5:04     ` Junio C Hamano
2025-12-13  1:22   ` Junio C Hamano
2025-12-13  0:18 ` [PATCH 0/2] Add MEMZERO_ARRAY() macro and use it in coccinelle Junio C Hamano

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).