diff mbox series

libgomp: Include <limits.h> early to avoid link failure with glibc 2.34

Message ID 87bl78djjs.fsf@oldenburg.str.redhat.com
State Not applicable
Headers
Series libgomp: Include <limits.h> early to avoid link failure with glibc 2.34 |

Checks

Context Check Description
dj/TryBot-apply_patch fail Patch failed to apply to master at the time it was sent
dj/TryBot-32bit fail Patch series failed to apply

Commit Message

Florian Weimer July 12, 2021, 8:26 a.m. UTC 
  <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(+)

Comments

Florian Weimer July 12, 2021, 1:11 p.m. UTC | #1 
* 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
Jakub Jelinek July 12, 2021, 1:25 p.m. UTC | #2 
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
Florian Weimer July 12, 2021, 1:42 p.m. UTC | #3 
* 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 July 12, 2021, 2:35 p.m. UTC | #4 
* 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 July 12, 2021, 3:29 p.m. UTC | #5 
* 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
Jakub Jelinek July 13, 2021, 7:54 a.m. UTC | #6 
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 mbox series

Patch

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