From: Victoria Dye via GitGitGadget <hidden> Date: 2021-11-04 01:47:50
From: Victoria Dye <redacted>
Correct an issue encountered in the `dockerized(pedantic, fedora)` CI build,
first appearing after the release of v36 as the latest stable version of the
Fedora docker image. This image includes a version of `glibc` with the
attribute `__attr_access_none` added to `pthread_getspecific` [1], the
implementation of which only exists for GCC 11.X - the version included in
Fedora. 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. In the usage in `async_die_is_recursing`, `(void *)1` is not valid,
resulting in the error.
The fix imitates a workaround added in SELinux [2] by using the pointer to
`ret` as the second argument to `pthread_getspecific`. This guaranteed
non-NULL, valid pointer adheres to the current intended usage of
`pthread_getspecific` while not triggering the build error.
[1] https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=a1561c3bbe8
[2] https://lore.kernel.org/all/20211021140519.6593-1-cgzones@googlemail.com/
Co-authored-by: Johannes Schindelin [off-list ref]
Signed-off-by: Victoria Dye <redacted>
---
async_die_is_recursing: fix use of pthread_getspecific for Fedora
Following up on Johannes' report earlier [1], this patch fixes a
compiler error in the Fedora CI build (the same issue was identified in
a local developer build about a week ago [2]). This fix changes the
second argument in the call to pthread_getspecific from '(void *)1' to a
valid pointer, thus preventing the error in the use of
__attr_access_none.
[1]
https://lore.kernel.org/git/nycvar.QRO.7.76.6.2111040007170.56@tvgsbejvaqbjf.bet/
[2]
https://lore.kernel.org/git/43fe6d5c-bdb2-585c-c601-1da7a1b3ff8b@archlinux.org/
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1072%2Fvdye%2Ffix-fedora-build-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1072/vdye/fix-fedora-build-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1072
run-command.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
@@ -1099,7 +1099,7 @@ static NORETURN void die_async(const char *err, va_list params)staticintasync_die_is_recursing(void){void*ret=pthread_getspecific(async_die_counter);-pthread_setspecific(async_die_counter,(void*)1);+pthread_setspecific(async_die_counter,&ret);/* set to any non-NULL valid pointer */
I guess this would work, since the pointer is never dereferenced, but
the use of (void *)1 was hacky, and this warning seems like the right
time to make it less so.
Would a dynamically allocated pthread_local variable be a better
option, or even a static global, since we don't care about its value
so no need to worry about any races?
If neither, would MAP_FAILED also trigger the warning?
Carlo
@@ -1099,7 +1099,7 @@ static NORETURN void die_async(const char *err, va_list params)staticintasync_die_is_recursing(void){void*ret=pthread_getspecific(async_die_counter);-pthread_setspecific(async_die_counter,(void*)1);+pthread_setspecific(async_die_counter,&ret);/* set to any non-NULL valid pointer */
I guess this would work, since the pointer is never dereferenced, but
the use of (void *)1 was hacky, and this warning seems like the right
time to make it less so.
Would a dynamically allocated pthread_local variable be a better
option, or even a static global, since we don't care about its value
so no need to worry about any races?
Yeah, I had the same thought. I think what's in the patch above is OK in
practice, but it sure _feels_ wrong to store the address of an auto
variable that goes out of scope.
I'm OK with it as a minimal fix, though, to get things unstuck. The
commit message nicely explains what's going on, and the original (which
it looks like blames to me ;) ) is pretty gross, too.
Keeping an actual counter variable would be the least-confusing thing,
IMHO, but that implies allocating per-thread storage (which means having
to clean it up). And we really only care about counting up to "1", so
the boolean "do we have a pointer" is fine. The static variable you
suggest might be a good middle ground there, and we could even use it
for the comparison to make things more clear. Something like:
static int async_die_is_recursing(void)
{
static int async_recursing_flag;
void *ret = pthread_getspecific(async_die_counter);
pthread_setspecific(async_die_counter, &async_recursing_flag);
return ret == &async_recursing_flag;
}
But I don't feel that strongly either way.
-Peff
@@ -1099,7 +1099,7 @@ static NORETURN void die_async(const char *err, va_list params)staticintasync_die_is_recursing(void){void*ret=pthread_getspecific(async_die_counter);-pthread_setspecific(async_die_counter,(void*)1);+pthread_setspecific(async_die_counter,&ret);/* set to any non-NULL valid pointer */
I guess this would work, since the pointer is never dereferenced, but
the use of (void *)1 was hacky, and this warning seems like the right
time to make it less so.
Would a dynamically allocated pthread_local variable be a better
option, or even a static global, since we don't care about its value
so no need to worry about any races?
Yeah, I had the same thought. I think what's in the patch above is OK in
practice, but it sure _feels_ wrong to store the address of an auto
variable that goes out of scope.
I'm OK with it as a minimal fix, though, to get things unstuck. The
commit message nicely explains what's going on, and the original (which
it looks like blames to me ;) ) is pretty gross, too.
Agree it is OK as a minimal fix, but also AFAIK nothing is really
stuck either, so something as simple as :
s/&ret/&async_die_counter/g
Would make it as minimal, and less likely to trigger something else in
the future (I am surprised nothing warns about local variables being
used out of scope).
Carlo
From: Victoria Dye via GitGitGadget <hidden> Date: 2021-11-04 04:01:08
From: Victoria Dye <redacted>
This fix corrects an issue found in the `dockerized(pedantic, fedora)` CI
build, first appearing after the introduction of a new version of the Fedora
docker image version. 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. In the usage in `async_die_is_recursing`, `(void *)1` is not
valid, causing the error.
This fix imitates a workaround added in SELinux [2] by using the pointer to
the static `async_die_counter` itself as the second argument to
`pthread_setspecific`. This guaranteed non-NULL, valid pointer matches the
intent of the current usage while not triggering the build error.
[1] https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=a1561c3bbe8
[2] https://lore.kernel.org/all/20211021140519.6593-1-cgzones@googlemail.com/
Co-authored-by: Johannes Schindelin [off-list ref]
Signed-off-by: Victoria Dye <redacted>
---
async_die_is_recursing: fix use of pthread_getspecific for Fedora
Following up on Johannes' report earlier [1], this patch fixes a
compiler error in the Fedora CI build (the same issue was identified in
a local developer build about a week ago [2]). This fix changes the
second argument in the call to pthread_setspecific from '(void *)1' to a
valid pointer, thus preventing the error in the use of
__attr_access_none.
Changes since V1
================
* Change &ret to &async_die_counter ("dummy" argument to
pthread_setspecific) so that it's using a global (rather than local)
variable
* Fix typo in commit message (pthread_getspecific ->
pthread_setspecific)
[1]
https://lore.kernel.org/git/nycvar.QRO.7.76.6.2111040007170.56@tvgsbejvaqbjf.bet/
[2]
https://lore.kernel.org/git/43fe6d5c-bdb2-585c-c601-1da7a1b3ff8b@archlinux.org/
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1072%2Fvdye%2Ffix-fedora-build-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1072/vdye/fix-fedora-build-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1072
Range-diff vs v1:
1: efe0b398f54 ! 1: 7f10e09269a async_die_is_recursing: fix use of pthread_getspecific for Fedora
@@ Metadata
Author: Victoria Dye [off-list ref]
## Commit message ##
- async_die_is_recursing: fix use of pthread_getspecific for Fedora
+ async_die_is_recursing: work around GCC v11.x issue on Fedora
- Correct an issue encountered in the `dockerized(pedantic, fedora)` CI build,
- first appearing after the release of v36 as the latest stable version of the
- Fedora docker image. This image includes a version of `glibc` with the
- attribute `__attr_access_none` added to `pthread_getspecific` [1], the
+ This fix corrects an issue found in the `dockerized(pedantic, fedora)` CI
+ build, first appearing after the introduction of a new version of the Fedora
+ docker image version. 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
- Fedora. 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. In the usage in `async_die_is_recursing`, `(void *)1` is not valid,
- resulting in the error.
+ 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. In the usage in `async_die_is_recursing`, `(void *)1` is not
+ valid, causing the error.
- The fix imitates a workaround added in SELinux [2] by using the pointer to
- `ret` as the second argument to `pthread_getspecific`. This guaranteed
- non-NULL, valid pointer adheres to the current intended usage of
- `pthread_getspecific` while not triggering the build error.
+ This fix imitates a workaround added in SELinux [2] by using the pointer to
+ the static `async_die_counter` itself as the second argument to
+ `pthread_setspecific`. This guaranteed non-NULL, valid pointer matches the
+ intent of the current usage while not triggering the build error.
[1] https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=a1561c3bbe8
[2] https://lore.kernel.org/all/20211021140519.6593-1-cgzones@googlemail.com/
@@ run-command.c: static NORETURN void die_async(const char *err, va_list params)
{
void *ret = pthread_getspecific(async_die_counter);
- pthread_setspecific(async_die_counter, (void *)1);
-+ pthread_setspecific(async_die_counter, &ret); /* set to any non-NULL valid pointer */
++ pthread_setspecific(async_die_counter, &async_die_counter); /* set to any non-NULL valid pointer */
return ret != NULL;
}
run-command.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Jeff King <hidden> Date: 2021-11-04 05:56:13
On Wed, Nov 03, 2021 at 08:20:56PM -0700, Carlo Arenas wrote:
Agree it is OK as a minimal fix, but also AFAIK nothing is really
stuck either, so something as simple as :
s/&ret/&async_die_counter/g
Would make it as minimal, and less likely to trigger something else in
the future (I am surprised nothing warns about local variables being
used out of scope).
It's in scope when we call the function. The potential for bugs only
happens because we know pthread_setspecific() is going to hold on to
it. So the compiler complaining would imply that it knows the semantics
of the pthread function.
-Peff
From: Jeff King <hidden> Date: 2021-11-04 05:58:05
On Thu, Nov 04, 2021 at 04:01:03AM +0000, Victoria Dye via GitGitGadget wrote:
Changes since V1
================
* Change &ret to &async_die_counter ("dummy" argument to
pthread_setspecific) so that it's using a global (rather than local)
variable
* Fix typo in commit message (pthread_getspecific ->
pthread_setspecific)
From: Johannes Schindelin <hidden> Date: 2021-11-04 08:43:42
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`.
@@ -1099,7 +1099,7 @@ static NORETURN void die_async(const char *err, va_list params)staticintasync_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
From: Carlo Arenas <hidden> Date: 2021-11-04 09:46:16
On Thu, Nov 4, 2021 at 1:43 AM Johannes Schindelin
[off-list ref] wrote:
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`.
Got to read the whole explanation that Victoria put together.
for the warning to trigger on that gcc, you got also to have the
attribute in the header as shown by the link[1] she provided, and
which I am sure could be a nice thing to send toward those helpful
folks of the MSYS2 project so it can be added to their WinPthread
headers, as was done in glibc.
Carlo
[1] https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=a1561c3bbe8
From: Johannes Schindelin <hidden> Date: 2021-11-04 16:33:24
Hi Carlo,
On Thu, 4 Nov 2021, Carlo Arenas wrote:
On Thu, Nov 4, 2021 at 1:43 AM Johannes Schindelin
[off-list ref] wrote:
quoted
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`.
Got to read the whole explanation that Victoria put together.
for the warning to trigger on that gcc, you got also to have the
attribute in the header as shown by the link[1] she provided, and
which I am sure could be a nice thing to send toward those helpful
folks of the MSYS2 project so it can be added to their WinPthread
headers, as was done in glibc.
D'oh. Of course we do not have glibc in the Git for Windows SDK! Thanks
for helping me understand this.
Ciao,
Dscho