Message ID | 87bl78djjs.fsf@oldenburg.str.redhat.com |
---|---|
State | New |
Headers | show |
Series | libgomp: Include <limits.h> early to avoid link failure with glibc 2.34 | expand |
* Florian Weimer: > <limits.h> is included indirectly in the #pragma GCC visibility hidden > block. With glibc 2.34, <limits.h> needs a declaration of the sysconf > function, and including it under hidden visibility turns other calls > to sysconf into hidden references, leading to a linker failure. > > libgomp/ChangeLog: > > * libgomp.h: Include <limits.h>. > > --- > libgomp/libgomp.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/libgomp/libgomp.h b/libgomp/libgomp.h > index 8d25dc8e2a8..1fe209429d1 100644 > --- a/libgomp/libgomp.h > +++ b/libgomp/libgomp.h > @@ -46,6 +46,7 @@ > #include "libgomp-plugin.h" > #include "gomp-constants.h" > > +#include <limits.h> > #ifdef HAVE_PTHREAD_H > #include <pthread.h> > #endif I think this is a real libgomp bug, but if this glibc patch is accepted, at least libgomp will build again: Reduce <limits.h> pollution due to dynamic PTHREAD_STACK_MIN <https://sourceware.org/pipermail/libc-alpha/2021-July/128940.html> So while I still think libgomp should be fixed, it won't need backporting to release branches (assuming the glibc workaround goes in, of course). Thanks, Florian
On Mon, Jul 12, 2021 at 10:26:47AM +0200, Florian Weimer via Gcc-patches wrote: > <limits.h> is included indirectly in the #pragma GCC visibility hidden > block. With glibc 2.34, <limits.h> needs a declaration of the sysconf > function, and including it under hidden visibility turns other calls > to sysconf into hidden references, leading to a linker failure. > > libgomp/ChangeLog: > > * libgomp.h: Include <limits.h>. If this is because of the config/linux/sem.h #include <limits.h>, I'd prefer not to include that header instead, we rely on being compiled by GCC anyway (and clang/icc support __INT_MAX__ anyway). Or e.g. config/posix/sem.h uses #ifdef HAVE_ATTRIBUTE_VISIBILITY # pragma GCC visibility push(default) #endif #include <semaphore.h> #ifdef HAVE_ATTRIBUTE_VISIBILITY # pragma GCC visibility pop #endif 2021-07-12 Jakub Jelinek <jakub@redhat.com> Florian Weimer <fweimer@redhat.com> * config/linux/sem.h: Don't include limits.h. (SEM_WAIT): Define to -__INT_MAX__ - 1 instead of INT_MIN. --- libgomp/config/linux/sem.h.jj 2021-01-18 07:18:42.360339646 +0100 +++ libgomp/config/linux/sem.h 2021-07-12 15:18:10.121178404 +0200 @@ -33,10 +33,8 @@ #ifndef GOMP_SEM_H #define GOMP_SEM_H 1 -#include <limits.h> /* For INT_MIN */ - typedef int gomp_sem_t; -#define SEM_WAIT INT_MIN +#define SEM_WAIT (-__INT_MAX__ - 1) #define SEM_INC 1 extern void gomp_sem_wait_slow (gomp_sem_t *, int); Jakub
* Jakub Jelinek: > On Mon, Jul 12, 2021 at 10:26:47AM +0200, Florian Weimer via Gcc-patches wrote: >> <limits.h> is included indirectly in the #pragma GCC visibility hidden >> block. With glibc 2.34, <limits.h> needs a declaration of the sysconf >> function, and including it under hidden visibility turns other calls >> to sysconf into hidden references, leading to a linker failure. >> >> libgomp/ChangeLog: >> >> * libgomp.h: Include <limits.h>. > > If this is because of the config/linux/sem.h #include <limits.h>, > I'd prefer not to include that header instead, we rely on being compiled > by GCC anyway (and clang/icc support __INT_MAX__ anyway). > > Or e.g. config/posix/sem.h uses > #ifdef HAVE_ATTRIBUTE_VISIBILITY > # pragma GCC visibility push(default) > #endif > > #include <semaphore.h> > > #ifdef HAVE_ATTRIBUTE_VISIBILITY > # pragma GCC visibility pop > #endif > > 2021-07-12 Jakub Jelinek <jakub@redhat.com> > Florian Weimer <fweimer@redhat.com> > > * config/linux/sem.h: Don't include limits.h. > (SEM_WAIT): Define to -__INT_MAX__ - 1 instead of INT_MIN. > > --- libgomp/config/linux/sem.h.jj 2021-01-18 07:18:42.360339646 +0100 > +++ libgomp/config/linux/sem.h 2021-07-12 15:18:10.121178404 +0200 > @@ -33,10 +33,8 @@ > #ifndef GOMP_SEM_H > #define GOMP_SEM_H 1 > > -#include <limits.h> /* For INT_MIN */ > - > typedef int gomp_sem_t; > -#define SEM_WAIT INT_MIN > +#define SEM_WAIT (-__INT_MAX__ - 1) > #define SEM_INC 1 > > extern void gomp_sem_wait_slow (gomp_sem_t *, int); I tested this on csky-linux-gnuabiv2 with the glibc version that failed before, and it works. So I guess your version is fine, too. Thanks, Florian
* Florian Weimer: > I tested this on csky-linux-gnuabiv2 with the glibc version that failed > before, and it works. So I guess your version is fine, too. Build on powerpc64-linux-gnu and other targets now fails with: /home/bmg/src/gcc/libgomp/config/linux/affinity.c: In function ‘gomp_init_affini ty’: /home/bmg/src/gcc/libgomp/config/linux/affinity.c:53:41: error: ‘ULONG_MAX’ unde clared (first use in this function) 53 | if (!gomp_affinity_init_level (1, ULONG_MAX, true)) | ^~~~~~~~~ So affinity.c will need to include <limits.h>. Thanks, Florian
* Florian Weimer: > * Florian Weimer: > >> I tested this on csky-linux-gnuabiv2 with the glibc version that failed >> before, and it works. So I guess your version is fine, too. > > Build on powerpc64-linux-gnu and other targets now fails with: > > /home/bmg/src/gcc/libgomp/config/linux/affinity.c: In function ‘gomp_init_affini > ty’: > /home/bmg/src/gcc/libgomp/config/linux/affinity.c:53:41: error: ‘ULONG_MAX’ unde > clared (first use in this function) > 53 | if (!gomp_affinity_init_level (1, ULONG_MAX, true)) > | ^~~~~~~~~ > > So affinity.c will need to include <limits.h>. I verifed that this change on top successfully builds GCC for all glibc targets: diff --git a/libgomp/config/linux/affinity.c b/libgomp/config/linux/affinity.c index c5abdce23..1b636c613 100644 --- a/libgomp/config/linux/affinity.c +++ b/libgomp/config/linux/affinity.c @@ -35,6 +35,7 @@ #include <stdio.h> #include <string.h> #include <unistd.h> +#include <limits.h> #ifdef HAVE_PTHREAD_AFFINITY_NP Thanks, Florian
On Mon, Jul 12, 2021 at 05:29:36PM +0200, Florian Weimer wrote: > I verifed that this change on top successfully builds GCC for all glibc > targets: Here is what I've committed after testing overnight: 2021-07-13 Jakub Jelinek <jakub@redhat.com> Florian Weimer <fweimer@redhat.com> * config/linux/sem.h: Don't include limits.h. (SEM_WAIT): Define to -__INT_MAX__ - 1 instead of INT_MIN. * config/linux/affinity.c: Include limits.h. --- libgomp/config/linux/sem.h.jj 2021-01-18 07:18:42.360339646 +0100 +++ libgomp/config/linux/sem.h 2021-07-12 15:18:10.121178404 +0200 @@ -33,10 +33,8 @@ #ifndef GOMP_SEM_H #define GOMP_SEM_H 1 -#include <limits.h> /* For INT_MIN */ - typedef int gomp_sem_t; -#define SEM_WAIT INT_MIN +#define SEM_WAIT (-__INT_MAX__ - 1) #define SEM_INC 1 extern void gomp_sem_wait_slow (gomp_sem_t *, int); --- libgomp/config/linux/affinity.c.jj 2021-01-04 10:25:56.160037625 +0100 +++ libgomp/config/linux/affinity.c 2021-07-12 17:19:07.280429144 +0200 @@ -35,6 +35,7 @@ #include <stdio.h> #include <string.h> #include <unistd.h> +#include <limits.h> #ifdef HAVE_PTHREAD_AFFINITY_NP Jakub
diff --git a/libgomp/libgomp.h b/libgomp/libgomp.h index 8d25dc8e2a8..1fe209429d1 100644 --- a/libgomp/libgomp.h +++ b/libgomp/libgomp.h @@ -46,6 +46,7 @@ #include "libgomp-plugin.h" #include "gomp-constants.h" +#include <limits.h> #ifdef HAVE_PTHREAD_H #include <pthread.h> #endif