Re: [PATCH v2 2/2] hex: make hash_to_hex_algop() and friends thread-safe
From: Johannes Schindelin <hidden>
Date: 2020-07-16 12:56:41
Subsystem:
the rest · Maintainer:
Linus Torvalds
Hi Matheus, On Fri, 26 Jun 2020, Matheus Tavares wrote:
quoted hunk ↗ jump to hunk
@@ -136,12 +137,49 @@ char *oid_to_hex_r(char *buffer, const struct object_id *oid) return hash_to_hex_algop_r(buffer, oid->hash, the_hash_algo); } +struct hexbuf_array { + int idx; + char bufs[4][GIT_MAX_HEXSZ + 1]; +}; + +#ifdef HAVE_THREADS +static pthread_key_t hexbuf_array_key; +static pthread_once_t hexbuf_array_once = PTHREAD_ONCE_INIT; + +static void init_hexbuf_array_key(void) +{ + if (pthread_key_create(&hexbuf_array_key, free)) + die(_("failed to initialize threads' key for hash to hex conversion")); +} + +#else +static struct hexbuf_array default_hexbuf_array; +#endif + char *hash_to_hex_algop(const unsigned char *hash, const struct git_hash_algo *algop) { - static int bufno; - static char hexbuffer[4][GIT_MAX_HEXSZ + 1]; - bufno = (bufno + 1) % ARRAY_SIZE(hexbuffer); - return hash_to_hex_algop_r(hexbuffer[bufno], hash, algop); + struct hexbuf_array *ha; + +#ifdef HAVE_THREADS + void *value; + + if (pthread_once(&hexbuf_array_once, init_hexbuf_array_key)) + die(_("failed to initialize threads' key for hash to hex conversion")); + + value = pthread_getspecific(hexbuf_array_key); + if (value) { + ha = (struct hexbuf_array *) value; + } else { + ha = xmalloc(sizeof(*ha));
I just realized (while trying to debug something independent) that this leaves `ha->idx` uninitialized. So you will need at least this patch to fix a bug that currently haunts `seen`'s CI builds (you can use `--valgrind`, like I did, to identify the problem): -- snip --
diff --git a/hex.c b/hex.c
index 4f2f163d5e7..365ba94ab11 100644
--- a/hex.c
+++ b/hex.c@@ -171,6 +171,7 @@ char *hash_to_hex_algop(const unsigned char *hash, const struct git_hash_algo *a ha = (struct hexbuf_array *) value; } else { ha = xmalloc(sizeof(*ha)); + ha->idx = 0; if (pthread_setspecific(hexbuf_array_key, (void *)ha)) die(_("failed to set thread buffer for hash to hex conversion")); } -- snap --
But as I mentioned before, I would be much more in favor of abandoning
this thread-local idea (because it is _still_ fragile, as the same thread
could try to make use of more than four hex values in the same `printf()`,
for example) and instead using Coccinelle to convert all those
`oid_to_hex()` calls to `oid_to_hex_r()` calls.
Now, I am _far_ from knowing what I'm doing with Coccinelle, but I think
this here semantic patch should get you going:
-- snipsnap --
@@
expression E;
@@
{
++ char hex[GIT_MAX_HEXSZ + 1];
...
- oid_to_hex(E)
+ oid_to_hex_r(hex, E)
...
}
@@
expression E1, E2;
@@
{
++ char hex1[GIT_MAX_HEXSZ + 1], hex2[GIT_MAX_HEXSZ + 1];
...
- oid_to_hex(E1)
+ oid_to_hex_r(hex1, E1)
...
- oid_to_hex(E2)
+ oid_to_hex_r(hex2, E2)
...
}