Hi Victoria, excellent work! The patch looks very good to me. I just want to add a little context below, and thank you _so much_ for letting me sleep while you tie it all up in a neat patch. On Thu, 4 Nov 2021, Victoria Dye via GitGitGadget wrote: > [...] This image includes a version of `glibc` with the attribute > `__attr_access_none` added to `pthread_setspecific` [1], the > implementation of which only exists for GCC 11.X - the version included > in the Fedora image. The attribute requires that the pointer provided in > the second argument of `pthread_getspecific` must, if not NULL, be a > pointer to a valid object. My initial reading last night was that the `none` mode means that the pointer does not have to be valid, but you're right, the documentation at https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html clearly spells it out: Unless the pointer is null the pointed-to object must exist and have at least the size as denoted by the size-index argument. The object need not be initialized. The mode is intended to be used as a means to help validate the expected object size, for example in functions that call __builtin_object_size. Which means that yes, `(void *)1` was incorrect and _had_ to be changed. The patch is therefore a fix, not a work-around (contrary to my initial impression). So then I got puzzled by this while sleeping: why does it fail on Fedora, when it does _not_ fail in Git for Windows' SDK (where we also recently upgraded to GCC v11.x, thanks to the hard work of the MSYS2 project)? My best guess is that my GCC (which is v11.2.0) and Fedora's GCC (which is v11.2.1 plus the usual Fedora customization) behave slightly differently with respect to that optional `size-index` argument: `pthread_setspecific()` is essentially declared without a `size-index`, so I guess GCC v11.2.1 probably defaults to `size-index = 1`. > diff --git a/run-command.c b/run-command.c > index 7ef5cc712a9..f40df01c772 100644 > --- a/run-command.c > +++ b/run-command.c > @@ -1099,7 +1099,7 @@ static NORETURN void die_async(const char *err, va_list params) > static int async_die_is_recursing(void) > { > void *ret = pthread_getspecific(async_die_counter); > - pthread_setspecific(async_die_counter, (void *)1); > + pthread_setspecific(async_die_counter, &async_die_counter); /* set to any non-NULL valid pointer */ This looks good! To make the intention even clearer, we could change the line above to `int ret = !!pthread_getspecific(async_die_counter);`, but as we are _really_ late in the -rc cycle, I am very much in favor of leaving out such clean-ups that do not fix any bug. Again, thank you so much for your hard work, it was fun debugging this with you, Dscho > return ret != NULL; > } > > > base-commit: 876b1423317071f43c99666f3fc3db3642dfbe14 > -- > gitgitgadget >
