Re: [PATCH v2] async_die_is_recursing: work around GCC v11.x issue on Fedora

"Victoria Dye via GitGitGadget" [off-list ref] writes:

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>
---

Looks like they timed their update to disrupt us most effectively,
but we are gifted with watchful eyes and competent hands ;-).

Thanks for coming up with a clearly written description and a clean
fix so quickly.

Will apply.

On Wed, Nov 03 2021, Junio C Hamano wrote:

"Victoria Dye via GitGitGadget" [off-list ref] writes:

quoted

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>
---

Looks like they timed their update to disrupt us most effectively,
but we are gifted with watchful eyes and competent hands ;-).

Thanks for coming up with a clearly written description and a clean
fix so quickly.

Will apply.

I don't know what this would be for the "fedora" and other images, but
it seems to me like the below is something we should do. This replaces
"latest" with whatever "latest" currently maps onto.

I.e. I don't think it's a good thing that the carpet gets swept from
under you as far as these CI images go. We could subscribe to some feed
of when these images are bumped to see when to update, but having our
base change from under us just leads to a waste of time for a bunch of
people wondering why their CI is failing, which now they'll need to
rebase on some on-list or only-in-upstream-master patch to have it
"pass".

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 6ed6a9e8076..6b7dab01269 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml

@@ -7,7 +7,7 @@ env:
 
 jobs:
   ci-config:
-    runs-on: ubuntu-latest
+    runs-on: ubuntu-20.04
     outputs:
       enabled: ${{ steps.check-ref.outputs.enabled }}${{ steps.skip-if-redundant.outputs.enabled }}
     steps:

@@ -79,7 +79,7 @@ jobs:
   windows-build:
     needs: ci-config
     if: needs.ci-config.outputs.enabled == 'yes'
-    runs-on: windows-latest
+    runs-on: windows-2019
     steps:
     - uses: actions/checkout@v2
     - uses: git-for-windows/setup-git-for-windows-sdk@v1

@@ -97,7 +97,7 @@ jobs:
         name: windows-artifacts
         path: artifacts
   windows-test:
-    runs-on: windows-latest
+    runs-on: windows-2019
     needs: [windows-build]
     strategy:
       fail-fast: false

@@ -132,7 +132,7 @@ jobs:
     env:
       NO_PERL: 1
       GIT_CONFIG_PARAMETERS: "'user.name=CI' 'user.email=ci@git'"
-    runs-on: windows-latest
+    runs-on: windows-2019
     steps:
     - uses: actions/checkout@v2
     - uses: git-for-windows/setup-git-for-windows-sdk@v1

@@ -178,7 +178,7 @@ jobs:
         name: vs-artifacts
         path: artifacts
   vs-test:
-    runs-on: windows-latest
+    runs-on: windows-2019
     needs: vs-build
     strategy:
       fail-fast: false

@@ -218,22 +218,22 @@ jobs:
         vector:
           - jobname: linux-clang
             cc: clang
-            pool: ubuntu-latest
+            pool: ubuntu-20.04
           - jobname: linux-gcc
             cc: gcc
-            pool: ubuntu-latest
+            pool: ubuntu-20.04
           - jobname: osx-clang
             cc: clang
-            pool: macos-latest
+            pool: macos-10.15
           - jobname: osx-gcc
             cc: gcc
-            pool: macos-latest
+            pool: macos-10.15
           - jobname: linux-gcc-default
             cc: gcc
-            pool: ubuntu-latest
+            pool: ubuntu-20.04
           - jobname: linux-leaks
             cc: gcc
-            pool: ubuntu-latest
+            pool: ubuntu-20.04
     env:
       CC: ${{matrix.vector.cc}}
       jobname: ${{matrix.vector.jobname}}

@@ -265,7 +265,7 @@ jobs:
           image: fedora
     env:
       jobname: ${{matrix.vector.jobname}}
-    runs-on: ubuntu-latest
+    runs-on: ubuntu-20.04
     container: ${{matrix.vector.image}}
     steps:
     - uses: actions/checkout@v1

@@ -314,7 +314,7 @@ jobs:
     if: needs.ci-config.outputs.enabled == 'yes'
     env:
       jobname: Documentation
-    runs-on: ubuntu-latest
+    runs-on: ubuntu-20.04
     steps:
     - uses: actions/checkout@v2
     - run: ci/install-dependencies.sh

On 11/4/2021 5:42 AM, Ævar Arnfjörð Bjarmason wrote:

On Wed, Nov 03 2021, Junio C Hamano wrote:

quoted

"Victoria Dye via GitGitGadget" [off-list ref] writes:

quoted

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>
---

Looks like they timed their update to disrupt us most effectively,
but we are gifted with watchful eyes and competent hands ;-).

Thanks for coming up with a clearly written description and a clean
fix so quickly.

Will apply.

I don't know what this would be for the "fedora" and other images, but
it seems to me like the below is something we should do. This replaces
"latest" with whatever "latest" currently maps onto.

I.e. I don't think it's a good thing that the carpet gets swept from
under you as far as these CI images go. We could subscribe to some feed
of when these images are bumped to see when to update, but having our
base change from under us just leads to a waste of time for a bunch of
people wondering why their CI is failing, which now they'll need to
rebase on some on-list or only-in-upstream-master patch to have it
"pass".

Having our CI go red because the 'latest' feed changed something is
probably the best "feed" to subscribe to, since we only get notified
if it matters.

Better to have automation go red than for us to not realize our code
doesn't work on newer platforms because our CI hasn't been updated.

Thanks,
-Stolee