{"thread":{"id":"56839","subject":"[PATCH] async_die_is_recursing: fix use of pthread_getspecific for Fedora","startedAt":"2021-11-04T01:47:50Z","lastAt":"2021-11-05T21:45:52Z","messageCount":15,"participants":["Victoria Dye via GitGitGadget","Carlo Arenas","Jeff King","Junio C Hamano","Johannes Schindelin","Ævar Arnfjörð Bjarmason","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"440421","messageId":"pull.1072.git.1635990465854.gitgitgadget@gmail.com","threadId":"56839","inReplyTo":null,"subject":"[PATCH] async_die_is_recursing: fix use of pthread_getspecific for Fedora","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-04T01:47:45Z","receivedAt":"2021-11-04T01:47:50Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nCorrect an issue encountered in the `dockerized(pedantic, fedora)` CI build,\nfirst appearing after the release of v36 as the latest stable version of the\nFedora docker image. This image includes a version of `glibc` with the\nattribute `__attr_access_none` added to `pthread_getspecific` [1], the\nimplementation of which only exists for GCC 11.X - the version included in\nFedora. The attribute requires that the pointer provided in the second\nargument of `pthread_getspecific` must, if not NULL, be a pointer to a valid\nobject. In the usage in `async_die_is_recursing`, `(void *)1` is not valid,\nresulting in the error.\n\nThe fix imitates a workaround added in SELinux [2] by using the pointer to\n`ret` as the second argument to `pthread_getspecific`. This guaranteed\nnon-NULL, valid pointer adheres to the current intended usage of\n`pthread_getspecific` while not triggering the build error.\n\n[1] https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=a1561c3bbe8\n[2] https://lore.kernel.org/all/20211021140519.6593-1-cgzones@googlemail.com/\n\nCo-authored-by: Johannes Schindelin \nSigned-off-by: Victoria Dye \n---\n async_die_is_recursing: fix use of pthread_getspecific for Fedora\n \n Following up on Johannes' report earlier [1], this patch fixes a\n compiler error in the Fedora CI build (the same issue was identified in\n a local developer build about a week ago [2]). This fix changes the\n second argument in the call to pthread_getspecific from '(void *)1' to a\n valid pointer, thus preventing the error in the use of\n __attr_access_none.\n \n [1]\n https://lore.kernel.org/git/nycvar.QRO.7.76.6.2111040007170.56@tvgsbejvaqbjf.bet/\n [2]\n https://lore.kernel.org/git/43fe6d5c-bdb2-585c-c601-1da7a1b3ff8b@archlinux.org/\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1072%2Fvdye%2Ffix-fedora-build-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1072/vdye/fix-fedora-build-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1072\n\n run-command.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 7ef5cc712a9..a82cf69e7d3 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1099,7 +1099,7 @@ static NORETURN void die_async(const char *err, va_list params)\n static int async_die_is_recursing(void)\n {\n \tvoid *ret = pthread_getspecific(async_die_counter);\n-\tpthread_setspecific(async_die_counter, (void *)1);\n+\tpthread_setspecific(async_die_counter, &ret); /* set to any non-NULL valid pointer */\n \treturn ret != NULL;\n }\n \n\nbase-commit: 876b1423317071f43c99666f3fc3db3642dfbe14\n-- \ngitgitgadget\n"},{"id":"440424","messageId":"CAPUEspjw4zypiywCBRc=y9uC4G5CKTE35GLVf_wDY436oO0C5w@mail.gmail.com","threadId":"56839","inReplyTo":"pull.1072.git.1635990465854.gitgitgadget@gmail.com","subject":"Re: [PATCH] async_die_is_recursing: fix use of pthread_getspecific for Fedora","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-11-04T02:23:23Z","receivedAt":"2021-11-04T02:23:37Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Nov 3, 2021 at 6:48 PM Victoria Dye via GitGit\n> The fix imitates a workaround added in SELinux [2] by using the pointer to\n> `ret` as the second argument to `pthread_getspecific`.\n\nthe SELinux workaround uses a valid global pointer though; while this won't\n\n> diff --git a/run-command.c b/run-command.c\n> index 7ef5cc712a9..a82cf69e7d3 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -1099,7 +1099,7 @@ static NORETURN void die_async(const char *err, va_list params)\n> static int async_die_is_recursing(void)\n> {\n> void *ret = pthread_getspecific(async_die_counter);\n> - pthread_setspecific(async_die_counter, (void *)1);\n> + pthread_setspecific(async_die_counter, &ret); /* set to any non-NULL valid pointer */\n\nI guess this would work, since the pointer is never dereferenced, but\nthe use of (void *)1 was hacky, and this warning seems like the right\ntime to make it less so.\n\nWould a dynamically allocated pthread_local variable be a better\noption, or even a static global, since we don't care about its value\nso no need to worry about any races?\n\nIf neither, would MAP_FAILED also trigger the warning?\n\nCarlo\n"},{"id":"440425","messageId":"YYNHb0qq2n5OWC+R@coredump.intra.peff.net","threadId":"56839","inReplyTo":"CAPUEspjw4zypiywCBRc=y9uC4G5CKTE35GLVf_wDY436oO0C5w@mail.gmail.com","subject":"Re: [PATCH] async_die_is_recursing: fix use of pthread_getspecific for Fedora","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-04T02:37:35Z","receivedAt":"2021-11-04T02:37:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 03, 2021 at 07:23:23PM -0700, Carlo Arenas wrote:\n\n> > diff --git a/run-command.c b/run-command.c\n> > index 7ef5cc712a9..a82cf69e7d3 100644\n> > --- a/run-command.c\n> > +++ b/run-command.c\n> > @@ -1099,7 +1099,7 @@ static NORETURN void die_async(const char *err, va_list params)\n> > static int async_die_is_recursing(void)\n> > {\n> > void *ret = pthread_getspecific(async_die_counter);\n> > - pthread_setspecific(async_die_counter, (void *)1);\n> > + pthread_setspecific(async_die_counter, &ret); /* set to any non-NULL valid pointer */\n> \n> I guess this would work, since the pointer is never dereferenced, but\n> the use of (void *)1 was hacky, and this warning seems like the right\n> time to make it less so.\n> \n> Would a dynamically allocated pthread_local variable be a better\n> option, or even a static global, since we don't care about its value\n> so no need to worry about any races?\n\nYeah, I had the same thought. I think what's in the patch above is OK in\npractice, but it sure _feels_ wrong to store the address of an auto\nvariable that goes out of scope.\n\nI'm OK with it as a minimal fix, though, to get things unstuck. The\ncommit message nicely explains what's going on, and the original (which\nit looks like blames to me ;) ) is pretty gross, too.\n\nKeeping an actual counter variable would be the least-confusing thing,\nIMHO, but that implies allocating per-thread storage (which means having\nto clean it up). And we really only care about counting up to \"1\", so\nthe boolean \"do we have a pointer\" is fine. The static variable you\nsuggest might be a good middle ground there, and we could even use it\nfor the comparison to make things more clear. Something like:\n\n static int async_die_is_recursing(void)\n {\n\t static int async_recursing_flag;\n void *ret = pthread_getspecific(async_die_counter);\n\t pthread_setspecific(async_die_counter, &async_recursing_flag);\n\t return ret == &async_recursing_flag;\n }\n\nBut I don't feel that strongly either way.\n\n-Peff\n"},{"id":"440426","messageId":"CAPUEspjnu8ySiM5damzCoPWhLv+azZFxsZun5Lztjwd-fDopaw@mail.gmail.com","threadId":"56839","inReplyTo":"YYNHb0qq2n5OWC+R@coredump.intra.peff.net","subject":"Re: [PATCH] async_die_is_recursing: fix use of pthread_getspecific for Fedora","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-11-04T03:20:56Z","receivedAt":"2021-11-04T03:21:13Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Nov 3, 2021 at 7:37 PM Jeff King wrote:\n> On Wed, Nov 03, 2021 at 07:23:23PM -0700, Carlo Arenas wrote:\n> > > diff --git a/run-command.c b/run-command.c\n> > > index 7ef5cc712a9..a82cf69e7d3 100644\n> > > --- a/run-command.c\n> > > +++ b/run-command.c\n> > > @@ -1099,7 +1099,7 @@ static NORETURN void die_async(const char *err, va_list params)\n> > > static int async_die_is_recursing(void)\n> > > {\n> > > void *ret = pthread_getspecific(async_die_counter);\n> > > - pthread_setspecific(async_die_counter, (void *)1);\n> > > + pthread_setspecific(async_die_counter, &ret); /* set to any non-NULL valid pointer */\n> >\n> > I guess this would work, since the pointer is never dereferenced, but\n> > the use of (void *)1 was hacky, and this warning seems like the right\n> > time to make it less so.\n> >\n> > Would a dynamically allocated pthread_local variable be a better\n> > option, or even a static global, since we don't care about its value\n> > so no need to worry about any races?\n>\n> Yeah, I had the same thought. I think what's in the patch above is OK in\n> practice, but it sure _feels_ wrong to store the address of an auto\n> variable that goes out of scope.\n>\n> I'm OK with it as a minimal fix, though, to get things unstuck. The\n> commit message nicely explains what's going on, and the original (which\n> it looks like blames to me ;) ) is pretty gross, too.\n\nAgree it is OK as a minimal fix, but also AFAIK nothing is really\nstuck either, so something as simple as :\n\n s/&ret/&async_die_counter/g\n\nWould make it as minimal, and less likely to trigger something else in\nthe future (I am surprised nothing warns about local variables being\nused out of scope).\n\nCarlo\n"},{"id":"440427","messageId":"pull.1072.v2.git.1635998463474.gitgitgadget@gmail.com","threadId":"56839","inReplyTo":"pull.1072.git.1635990465854.gitgitgadget@gmail.com","subject":"[PATCH v2] async_die_is_recursing: work around GCC v11.x issue on Fedora","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-04T04:01:03Z","receivedAt":"2021-11-04T04:01:08Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nThis fix corrects an issue found in the `dockerized(pedantic, fedora)` CI\nbuild, first appearing after the introduction of a new version of the Fedora\ndocker image version. This image includes a version of `glibc` with the\nattribute `__attr_access_none` added to `pthread_setspecific` [1], the\nimplementation of which only exists for GCC 11.X - the version included in\nthe Fedora image. The attribute requires that the pointer provided in the\nsecond argument of `pthread_getspecific` must, if not NULL, be a pointer to\na valid object. In the usage in `async_die_is_recursing`, `(void *)1` is not\nvalid, causing the error.\n\nThis fix imitates a workaround added in SELinux [2] by using the pointer to\nthe static `async_die_counter` itself as the second argument to\n`pthread_setspecific`. This guaranteed non-NULL, valid pointer matches the\nintent of the current usage while not triggering the build error.\n\n[1] https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=a1561c3bbe8\n[2] https://lore.kernel.org/all/20211021140519.6593-1-cgzones@googlemail.com/\n\nCo-authored-by: Johannes Schindelin \nSigned-off-by: Victoria Dye \n---\n async_die_is_recursing: fix use of pthread_getspecific for Fedora\n \n Following up on Johannes' report earlier [1], this patch fixes a\n compiler error in the Fedora CI build (the same issue was identified in\n a local developer build about a week ago [2]). This fix changes the\n second argument in the call to pthread_setspecific from '(void *)1' to a\n valid pointer, thus preventing the error in the use of\n __attr_access_none.\n \n \n Changes since V1\n ================\n \n * Change &ret to &async_die_counter (\"dummy\" argument to\n pthread_setspecific) so that it's using a global (rather than local)\n variable\n * Fix typo in commit message (pthread_getspecific ->\n pthread_setspecific)\n \n [1]\n https://lore.kernel.org/git/nycvar.QRO.7.76.6.2111040007170.56@tvgsbejvaqbjf.bet/\n [2]\n https://lore.kernel.org/git/43fe6d5c-bdb2-585c-c601-1da7a1b3ff8b@archlinux.org/\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1072%2Fvdye%2Ffix-fedora-build-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1072/vdye/fix-fedora-build-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1072\n\nRange-diff vs v1:\n\n 1: efe0b398f54 ! 1: 7f10e09269a async_die_is_recursing: fix use of pthread_getspecific for Fedora\n @@ Metadata\n Author: Victoria Dye \n \n ## Commit message ##\n - async_die_is_recursing: fix use of pthread_getspecific for Fedora\n + async_die_is_recursing: work around GCC v11.x issue on Fedora\n \n - Correct an issue encountered in the `dockerized(pedantic, fedora)` CI build,\n - first appearing after the release of v36 as the latest stable version of the\n - Fedora docker image. This image includes a version of `glibc` with the\n - attribute `__attr_access_none` added to `pthread_getspecific` [1], the\n + This fix corrects an issue found in the `dockerized(pedantic, fedora)` CI\n + build, first appearing after the introduction of a new version of the Fedora\n + docker image version. This image includes a version of `glibc` with the\n + attribute `__attr_access_none` added to `pthread_setspecific` [1], the\n implementation of which only exists for GCC 11.X - the version included in\n - Fedora. The attribute requires that the pointer provided in the second\n - argument of `pthread_getspecific` must, if not NULL, be a pointer to a valid\n - object. In the usage in `async_die_is_recursing`, `(void *)1` is not valid,\n - resulting in the error.\n + the Fedora image. The attribute requires that the pointer provided in the\n + second argument of `pthread_getspecific` must, if not NULL, be a pointer to\n + a valid object. In the usage in `async_die_is_recursing`, `(void *)1` is not\n + valid, causing the error.\n \n - The fix imitates a workaround added in SELinux [2] by using the pointer to\n - `ret` as the second argument to `pthread_getspecific`. This guaranteed\n - non-NULL, valid pointer adheres to the current intended usage of\n - `pthread_getspecific` while not triggering the build error.\n + This fix imitates a workaround added in SELinux [2] by using the pointer to\n + the static `async_die_counter` itself as the second argument to\n + `pthread_setspecific`. This guaranteed non-NULL, valid pointer matches the\n + intent of the current usage while not triggering the build error.\n \n [1] https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=a1561c3bbe8\n [2] https://lore.kernel.org/all/20211021140519.6593-1-cgzones@googlemail.com/\n @@ run-command.c: static NORETURN void die_async(const char *err, va_list params)\n {\n \tvoid *ret = pthread_getspecific(async_die_counter);\n -\tpthread_setspecific(async_die_counter, (void *)1);\n -+\tpthread_setspecific(async_die_counter, &ret); /* set to any non-NULL valid pointer */\n ++\tpthread_setspecific(async_die_counter, &async_die_counter); /* set to any non-NULL valid pointer */\n \treturn ret != NULL;\n }\n \n\n\n run-command.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 7ef5cc712a9..f40df01c772 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1099,7 +1099,7 @@ static NORETURN void die_async(const char *err, va_list params)\n static int async_die_is_recursing(void)\n {\n \tvoid *ret = pthread_getspecific(async_die_counter);\n-\tpthread_setspecific(async_die_counter, (void *)1);\n+\tpthread_setspecific(async_die_counter, &async_die_counter); /* set to any non-NULL valid pointer */\n \treturn ret != NULL;\n }\n \n\nbase-commit: 876b1423317071f43c99666f3fc3db3642dfbe14\n-- \ngitgitgadget\n"},{"id":"440428","messageId":"YYN1+tonXZgL9dDa@coredump.intra.peff.net","threadId":"56839","inReplyTo":"CAPUEspjnu8ySiM5damzCoPWhLv+azZFxsZun5Lztjwd-fDopaw@mail.gmail.com","subject":"Re: [PATCH] async_die_is_recursing: fix use of pthread_getspecific for Fedora","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-04T05:56:10Z","receivedAt":"2021-11-04T05:56:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 03, 2021 at 08:20:56PM -0700, Carlo Arenas wrote:\n\n> Agree it is OK as a minimal fix, but also AFAIK nothing is really\n> stuck either, so something as simple as :\n> \n> s/&ret/&async_die_counter/g\n> \n> Would make it as minimal, and less likely to trigger something else in\n> the future (I am surprised nothing warns about local variables being\n> used out of scope).\n\nIt's in scope when we call the function. The potential for bugs only\nhappens because we know pthread_setspecific() is going to hold on to\nit. So the compiler complaining would imply that it knows the semantics\nof the pthread function.\n\n-Peff\n"},{"id":"440429","messageId":"YYN2ahdpbBEUfRNe@coredump.intra.peff.net","threadId":"56839","inReplyTo":"pull.1072.v2.git.1635998463474.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] async_die_is_recursing: work around GCC v11.x issue on Fedora","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-04T05:58:02Z","receivedAt":"2021-11-04T05:58:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 04, 2021 at 04:01:03AM +0000, Victoria Dye via GitGitGadget wrote:\n\n> Changes since V1\n> ================\n> \n> * Change &ret to &async_die_counter (\"dummy\" argument to\n> pthread_setspecific) so that it's using a global (rather than local)\n> variable\n> * Fix typo in commit message (pthread_getspecific ->\n> pthread_setspecific)\n\nThanks, this version looks good to me!\n\n-Peff\n"},{"id":"440431","messageId":"xmqqzgqk2zsl.fsf@gitster.g","threadId":"56839","inReplyTo":"pull.1072.v2.git.1635998463474.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] async_die_is_recursing: work around GCC v11.x issue on Fedora","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-04T06:35:54Z","receivedAt":"2021-11-04T06:36:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Victoria Dye via GitGitGadget\" writes:\n\n> From: Victoria Dye \n>\n> This fix corrects an issue found in the `dockerized(pedantic, fedora)` CI\n> build, first appearing after the introduction of a new version of the Fedora\n> docker image version. This image includes a version of `glibc` with the\n> attribute `__attr_access_none` added to `pthread_setspecific` [1], the\n> implementation of which only exists for GCC 11.X - the version included in\n> the Fedora image. The attribute requires that the pointer provided in the\n> second argument of `pthread_getspecific` must, if not NULL, be a pointer to\n> a valid object. In the usage in `async_die_is_recursing`, `(void *)1` is not\n> valid, causing the error.\n>\n> This fix imitates a workaround added in SELinux [2] by using the pointer to\n> the static `async_die_counter` itself as the second argument to\n> `pthread_setspecific`. This guaranteed non-NULL, valid pointer matches the\n> intent of the current usage while not triggering the build error.\n>\n> [1] https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=a1561c3bbe8\n> [2] https://lore.kernel.org/all/20211021140519.6593-1-cgzones@googlemail.com/\n>\n> Co-authored-by: Johannes Schindelin \n> Signed-off-by: Victoria Dye \n> ---\n\nLooks like they timed their update to disrupt us most effectively,\nbut we are gifted with watchful eyes and competent hands ;-).\n\nThanks for coming up with a clearly written description and a clean\nfix so quickly.\n\nWill apply.\n"},{"id":"440433","messageId":"nycvar.QRO.7.76.6.2111040823270.56@tvgsbejvaqbjf.bet","threadId":"56839","inReplyTo":"pull.1072.v2.git.1635998463474.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] async_die_is_recursing: work around GCC v11.x issue on Fedora","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-04T08:43:25Z","receivedAt":"2021-11-04T08:43:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Victoria,\n\nexcellent work! The patch looks very good to me. I just want to add a\nlittle context below, and thank you _so much_ for letting me sleep while\nyou tie it all up in a neat patch.\n\nOn Thu, 4 Nov 2021, Victoria Dye via GitGitGadget wrote:\n\n> [...] This image includes a version of `glibc` with the attribute\n> `__attr_access_none` added to `pthread_setspecific` [1], the\n> implementation of which only exists for GCC 11.X - the version included\n> in the Fedora image. The attribute requires that the pointer provided in\n> the second argument of `pthread_getspecific` must, if not NULL, be a\n> pointer to a valid object.\n\nMy initial reading last night was that the `none` mode means that the\npointer does not have to be valid, but you're right, the documentation at\nhttps://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html\nclearly spells it out:\n\n\tUnless the pointer is null the pointed-to object must exist and\n\thave at least the size as denoted by the size-index argument. The\n\tobject need not be initialized. The mode is intended to be used as\n\ta means to help validate the expected object size, for example in\n\tfunctions that call __builtin_object_size.\n\nWhich means that yes, `(void *)1` was incorrect and _had_ to be changed.\nThe patch is therefore a fix, not a work-around (contrary to my initial\nimpression).\n\nSo then I got puzzled by this while sleeping: why does it fail on Fedora,\nwhen it does _not_ fail in Git for Windows' SDK (where we also recently\nupgraded to GCC v11.x, thanks to the hard work of the MSYS2 project)?\n\nMy best guess is that my GCC (which is v11.2.0) and Fedora's GCC (which is\nv11.2.1 plus the usual Fedora customization) behave slightly differently\nwith respect to that optional `size-index` argument:\n`pthread_setspecific()` is essentially declared without a `size-index`, so\nI guess GCC v11.2.1 probably defaults to `size-index = 1`.\n\n> diff --git a/run-command.c b/run-command.c\n> index 7ef5cc712a9..f40df01c772 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -1099,7 +1099,7 @@ static NORETURN void die_async(const char *err, va_list params)\n> static int async_die_is_recursing(void)\n> {\n> \tvoid *ret = pthread_getspecific(async_die_counter);\n> -\tpthread_setspecific(async_die_counter, (void *)1);\n> +\tpthread_setspecific(async_die_counter, &async_die_counter); /* set to any non-NULL valid pointer */\n\nThis looks good! To make the intention even clearer, we could change the\nline above to `int ret = !!pthread_getspecific(async_die_counter);`, but\nas we are _really_ late in the -rc cycle, I am very much in favor of\nleaving out such clean-ups that do not fix any bug.\n\nAgain, thank you so much for your hard work, it was fun debugging this\nwith you,\nDscho\n\n> \treturn ret != NULL;\n> }\n>\n>\n> base-commit: 876b1423317071f43c99666f3fc3db3642dfbe14\n> --\n> gitgitgadget\n>\n"},{"id":"440437","messageId":"211104.86v918i78r.gmgdl@evledraar.gmail.com","threadId":"56839","inReplyTo":"xmqqzgqk2zsl.fsf@gitster.g","subject":"Re: [PATCH v2] async_die_is_recursing: work around GCC v11.x issue on Fedora","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-04T09:42:13Z","receivedAt":"2021-11-04T09:46:02Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Nov 03 2021, Junio C Hamano wrote:\n\n> \"Victoria Dye via GitGitGadget\" writes:\n>\n>> From: Victoria Dye \n>>\n>> This fix corrects an issue found in the `dockerized(pedantic, fedora)` CI\n>> build, first appearing after the introduction of a new version of the Fedora\n>> docker image version. This image includes a version of `glibc` with the\n>> attribute `__attr_access_none` added to `pthread_setspecific` [1], the\n>> implementation of which only exists for GCC 11.X - the version included in\n>> the Fedora image. The attribute requires that the pointer provided in the\n>> second argument of `pthread_getspecific` must, if not NULL, be a pointer to\n>> a valid object. In the usage in `async_die_is_recursing`, `(void *)1` is not\n>> valid, causing the error.\n>>\n>> This fix imitates a workaround added in SELinux [2] by using the pointer to\n>> the static `async_die_counter` itself as the second argument to\n>> `pthread_setspecific`. This guaranteed non-NULL, valid pointer matches the\n>> intent of the current usage while not triggering the build error.\n>>\n>> [1] https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=a1561c3bbe8\n>> [2] https://lore.kernel.org/all/20211021140519.6593-1-cgzones@googlemail.com/\n>>\n>> Co-authored-by: Johannes Schindelin \n>> Signed-off-by: Victoria Dye \n>> ---\n>\n> Looks like they timed their update to disrupt us most effectively,\n> but we are gifted with watchful eyes and competent hands ;-).\n>\n> Thanks for coming up with a clearly written description and a clean\n> fix so quickly.\n>\n> Will apply.\n\nI don't know what this would be for the \"fedora\" and other images, but\nit seems to me like the below is something we should do. This replaces\n\"latest\" with whatever \"latest\" currently maps onto.\n\nI.e. I don't think it's a good thing that the carpet gets swept from\nunder you as far as these CI images go. We could subscribe to some feed\nof when these images are bumped to see when to update, but having our\nbase change from under us just leads to a waste of time for a bunch of\npeople wondering why their CI is failing, which now they'll need to\nrebase on some on-list or only-in-upstream-master patch to have it\n\"pass\".\n\ndiff --git a/.github/workflows/main.yml b/.github/workflows/main.yml\nindex 6ed6a9e8076..6b7dab01269 100644\n--- a/.github/workflows/main.yml\n+++ b/.github/workflows/main.yml\n@@ -7,7 +7,7 @@ env:\n \n jobs:\n ci-config:\n- runs-on: ubuntu-latest\n+ runs-on: ubuntu-20.04\n outputs:\n enabled: ${{ steps.check-ref.outputs.enabled }}${{ steps.skip-if-redundant.outputs.enabled }}\n steps:\n@@ -79,7 +79,7 @@ jobs:\n windows-build:\n needs: ci-config\n if: needs.ci-config.outputs.enabled == 'yes'\n- runs-on: windows-latest\n+ runs-on: windows-2019\n steps:\n - uses: actions/checkout@v2\n - uses: git-for-windows/setup-git-for-windows-sdk@v1\n@@ -97,7 +97,7 @@ jobs:\n name: windows-artifacts\n path: artifacts\n windows-test:\n- runs-on: windows-latest\n+ runs-on: windows-2019\n needs: [windows-build]\n strategy:\n fail-fast: false\n@@ -132,7 +132,7 @@ jobs:\n env:\n NO_PERL: 1\n GIT_CONFIG_PARAMETERS: \"'user.name=CI' 'user.email=ci@git'\"\n- runs-on: windows-latest\n+ runs-on: windows-2019\n steps:\n - uses: actions/checkout@v2\n - uses: git-for-windows/setup-git-for-windows-sdk@v1\n@@ -178,7 +178,7 @@ jobs:\n name: vs-artifacts\n path: artifacts\n vs-test:\n- runs-on: windows-latest\n+ runs-on: windows-2019\n needs: vs-build\n strategy:\n fail-fast: false\n@@ -218,22 +218,22 @@ jobs:\n vector:\n - jobname: linux-clang\n cc: clang\n- pool: ubuntu-latest\n+ pool: ubuntu-20.04\n - jobname: linux-gcc\n cc: gcc\n- pool: ubuntu-latest\n+ pool: ubuntu-20.04\n - jobname: osx-clang\n cc: clang\n- pool: macos-latest\n+ pool: macos-10.15\n - jobname: osx-gcc\n cc: gcc\n- pool: macos-latest\n+ pool: macos-10.15\n - jobname: linux-gcc-default\n cc: gcc\n- pool: ubuntu-latest\n+ pool: ubuntu-20.04\n - jobname: linux-leaks\n cc: gcc\n- pool: ubuntu-latest\n+ pool: ubuntu-20.04\n env:\n CC: ${{matrix.vector.cc}}\n jobname: ${{matrix.vector.jobname}}\n@@ -265,7 +265,7 @@ jobs:\n image: fedora\n env:\n jobname: ${{matrix.vector.jobname}}\n- runs-on: ubuntu-latest\n+ runs-on: ubuntu-20.04\n container: ${{matrix.vector.image}}\n steps:\n - uses: actions/checkout@v1\n@@ -314,7 +314,7 @@ jobs:\n if: needs.ci-config.outputs.enabled == 'yes'\n env:\n jobname: Documentation\n- runs-on: ubuntu-latest\n+ runs-on: ubuntu-20.04\n steps:\n - uses: actions/checkout@v2\n - run: ci/install-dependencies.sh\n"},{"id":"440438","messageId":"CAPUEspiJW8ee_1zoMg6JzJ0ow-2tmPh8iUZK41kfvt6etzEfjQ@mail.gmail.com","threadId":"56839","inReplyTo":"nycvar.QRO.7.76.6.2111040823270.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2] async_die_is_recursing: work around GCC v11.x issue on Fedora","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-11-04T09:46:02Z","receivedAt":"2021-11-04T09:46:16Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, Nov 4, 2021 at 1:43 AM Johannes Schindelin\n wrote:\n\n> My best guess is that my GCC (which is v11.2.0) and Fedora's GCC (which is\n> v11.2.1 plus the usual Fedora customization) behave slightly differently\n> with respect to that optional `size-index` argument:\n> `pthread_setspecific()` is essentially declared without a `size-index`, so\n> I guess GCC v11.2.1 probably defaults to `size-index = 1`.\n\nGot to read the whole explanation that Victoria put together.\n\nfor the warning to trigger on that gcc, you got also to have the\nattribute in the header as shown by the link[1] she provided, and\nwhich I am sure could be a nice thing to send toward those helpful\nfolks of the MSYS2 project so it can be added to their WinPthread\nheaders, as was done in glibc.\n\nCarlo\n\n[1] https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=a1561c3bbe8\n"},{"id":"440449","messageId":"29114bd7-1c81-7580-c8c0-88904dd013db@gmail.com","threadId":"56839","inReplyTo":"211104.86v918i78r.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2] async_die_is_recursing: work around GCC v11.x issue on Fedora","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-11-04T13:08:00Z","receivedAt":"2021-11-04T13:08:05Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/4/2021 5:42 AM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, Nov 03 2021, Junio C Hamano wrote:\n> \n>> \"Victoria Dye via GitGitGadget\" writes:\n>>\n>>> From: Victoria Dye \n>>>\n>>> This fix corrects an issue found in the `dockerized(pedantic, fedora)` CI\n>>> build, first appearing after the introduction of a new version of the Fedora\n>>> docker image version. This image includes a version of `glibc` with the\n>>> attribute `__attr_access_none` added to `pthread_setspecific` [1], the\n>>> implementation of which only exists for GCC 11.X - the version included in\n>>> the Fedora image. The attribute requires that the pointer provided in the\n>>> second argument of `pthread_getspecific` must, if not NULL, be a pointer to\n>>> a valid object. In the usage in `async_die_is_recursing`, `(void *)1` is not\n>>> valid, causing the error.\n>>>\n>>> This fix imitates a workaround added in SELinux [2] by using the pointer to\n>>> the static `async_die_counter` itself as the second argument to\n>>> `pthread_setspecific`. This guaranteed non-NULL, valid pointer matches the\n>>> intent of the current usage while not triggering the build error.\n>>>\n>>> [1] https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=a1561c3bbe8\n>>> [2] https://lore.kernel.org/all/20211021140519.6593-1-cgzones@googlemail.com/\n>>>\n>>> Co-authored-by: Johannes Schindelin \n>>> Signed-off-by: Victoria Dye \n>>> ---\n>>\n>> Looks like they timed their update to disrupt us most effectively,\n>> but we are gifted with watchful eyes and competent hands ;-).\n>>\n>> Thanks for coming up with a clearly written description and a clean\n>> fix so quickly.\n>>\n>> Will apply.\n> \n> I don't know what this would be for the \"fedora\" and other images, but\n> it seems to me like the below is something we should do. This replaces\n> \"latest\" with whatever \"latest\" currently maps onto.\n> \n> I.e. I don't think it's a good thing that the carpet gets swept from\n> under you as far as these CI images go. We could subscribe to some feed\n> of when these images are bumped to see when to update, but having our\n> base change from under us just leads to a waste of time for a bunch of\n> people wondering why their CI is failing, which now they'll need to\n> rebase on some on-list or only-in-upstream-master patch to have it\n> \"pass\".\nHaving our CI go red because the 'latest' feed changed something is\nprobably the best \"feed\" to subscribe to, since we only get notified\nif it matters.\n\nBetter to have automation go red than for us to not realize our code\ndoesn't work on newer platforms because our CI hasn't been updated.\n\nThanks,\n-Stolee\n"},{"id":"440465","messageId":"nycvar.QRO.7.76.6.2111041732290.56@tvgsbejvaqbjf.bet","threadId":"56839","inReplyTo":"CAPUEspiJW8ee_1zoMg6JzJ0ow-2tmPh8iUZK41kfvt6etzEfjQ@mail.gmail.com","subject":"Re: [PATCH v2] async_die_is_recursing: work around GCC v11.x issue on Fedora","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-04T16:33:05Z","receivedAt":"2021-11-04T16:33:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Carlo,\n\nOn Thu, 4 Nov 2021, Carlo Arenas wrote:\n\n> On Thu, Nov 4, 2021 at 1:43 AM Johannes Schindelin\n> wrote:\n>\n> > My best guess is that my GCC (which is v11.2.0) and Fedora's GCC (which is\n> > v11.2.1 plus the usual Fedora customization) behave slightly differently\n> > with respect to that optional `size-index` argument:\n> > `pthread_setspecific()` is essentially declared without a `size-index`, so\n> > I guess GCC v11.2.1 probably defaults to `size-index = 1`.\n>\n> Got to read the whole explanation that Victoria put together.\n>\n> for the warning to trigger on that gcc, you got also to have the\n> attribute in the header as shown by the link[1] she provided, and\n> which I am sure could be a nice thing to send toward those helpful\n> folks of the MSYS2 project so it can be added to their WinPthread\n> headers, as was done in glibc.\n\nD'oh. Of course we do not have glibc in the Git for Windows SDK! Thanks\nfor helping me understand this.\n\nCiao,\nDscho\n\n>\n> Carlo\n>\n> [1] https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=a1561c3bbe8\n>\n"},{"id":"440474","messageId":"xmqqcznf3jbj.fsf@gitster.g","threadId":"56839","inReplyTo":"29114bd7-1c81-7580-c8c0-88904dd013db@gmail.com","subject":"Re: [PATCH v2] async_die_is_recursing: work around GCC v11.x issue on Fedora","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-04T17:46:24Z","receivedAt":"2021-11-04T17:46:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> Having our CI go red because the 'latest' feed changed something is\n> probably the best \"feed\" to subscribe to, since we only get notified\n> if it matters.\n>\n> Better to have automation go red than for us to not realize our code\n> doesn't work on newer platforms because our CI hasn't been updated.\n\nI consider this a better implementation of what Æver suggested ;-)\n\nAnd an incident like this is one of the reasons why I like the \"CI\ndoes not stop after seeing the first problem\" behaviour. In a short\nterm, people can ignore a particular known failure and ensure they\ndo not introduce any new ones.\n\nThanks.\n"},{"id":"440553","messageId":"xmqqfssawa2f.fsf@gitster.g","threadId":"56839","inReplyTo":"xmqqzgqk2zsl.fsf@gitster.g","subject":"Re: [PATCH v2] async_die_is_recursing: work around GCC v11.x issue on Fedora","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-05T21:45:44Z","receivedAt":"2021-11-05T21:45:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Thanks for coming up with a clearly written description and a clean\n> fix so quickly.\n>\n> Will apply.\n\nFWIW, this is now in all my integration branches that may trigger CI\ntests. Again, thanks for a quick fix.\n\n"}]}