Define GEN_AS_CONST_HEADERS when generating header files [BZ #22792]

Message ID 20180208175818.GA8519@gmail.com
State New
Headers show
Series
  • Define GEN_AS_CONST_HEADERS when generating header files [BZ #22792]
Related show

Commit Message

H.J. Lu Feb. 8, 2018, 5:58 p.m.
Glibc build generates header files to define constants from special .sym
files.  If a .sym file includes the same header file which it generates,
it leads to circular dependency.  Define GEN_AS_CONST_HEADERS when
generating header files to avoid circular dependency.

<tcb-offsets.h> is needed for i686 and it isn't needed for x86-64 at
least since glibc 2.23.

Tested on i686 and x86-64.

OK for trunk?

H.J.
---
	[BZ #22792]
	* Makerules ($(common-objpfx)%.h): Pass -DGEN_AS_CONST_HEADERS
	to $(CC).
	* sysdeps/unix/sysv/linux/i386/lowlevellock.h: Include
	<tcb-offsets.h> only if GEN_AS_CONST_HEADERS isn't defined.
	* sysdeps/unix/sysv/linux/x86_64/lowlevellock.h: Don't include
	<tcb-offsets.h>.
---
 Makerules                                     | 4 +++-
 sysdeps/unix/sysv/linux/i386/lowlevellock.h   | 4 +++-
 sysdeps/unix/sysv/linux/x86_64/lowlevellock.h | 1 -
 3 files changed, 6 insertions(+), 3 deletions(-)

-- 
2.14.3

Comments

Florian Weimer Feb. 21, 2018, 7:09 a.m. | #1
On 02/08/2018 06:58 PM, H.J. Lu wrote:
> 	[BZ #22792]

> 	* Makerules ($(common-objpfx)%.h): Pass -DGEN_AS_CONST_HEADERS

> 	to $(CC).

> 	* sysdeps/unix/sysv/linux/i386/lowlevellock.h: Include

> 	<tcb-offsets.h> only if GEN_AS_CONST_HEADERS isn't defined.

> 	* sysdeps/unix/sysv/linux/x86_64/lowlevellock.h: Don't include

> 	<tcb-offsets.h>.


It looks to me that a cleaner fix would be to move the definition of the 
TCB types to a separate header.

The circular dependency is just a cosmetic warning from make, right?

Thanks,
Florian
H.J. Lu Feb. 21, 2018, 3:40 p.m. | #2
On Tue, Feb 20, 2018 at 11:09 PM, Florian Weimer <fweimer@redhat.com> wrote:
> On 02/08/2018 06:58 PM, H.J. Lu wrote:

>>

>>         [BZ #22792]

>>         * Makerules ($(common-objpfx)%.h): Pass -DGEN_AS_CONST_HEADERS

>>         to $(CC).

>>         * sysdeps/unix/sysv/linux/i386/lowlevellock.h: Include

>>         <tcb-offsets.h> only if GEN_AS_CONST_HEADERS isn't defined.

>>         * sysdeps/unix/sysv/linux/x86_64/lowlevellock.h: Don't include

>>         <tcb-offsets.h>.

>

>

> It looks to me that a cleaner fix would be to move the definition of the TCB

> types to a separate header.


I will take a look.

> The circular dependency is just a cosmetic warning from make, right?


It depends on how unlucky you are.  In one case on a many-core machine
with make -j60, I had to kill glibc build since it never stopped.


-- 
H.J.
H.J. Lu Feb. 21, 2018, 5:19 p.m. | #3
On Wed, Feb 21, 2018 at 7:40 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Feb 20, 2018 at 11:09 PM, Florian Weimer <fweimer@redhat.com> wrote:

>> On 02/08/2018 06:58 PM, H.J. Lu wrote:

>>>

>>>         [BZ #22792]

>>>         * Makerules ($(common-objpfx)%.h): Pass -DGEN_AS_CONST_HEADERS

>>>         to $(CC).

>>>         * sysdeps/unix/sysv/linux/i386/lowlevellock.h: Include

>>>         <tcb-offsets.h> only if GEN_AS_CONST_HEADERS isn't defined.

>>>         * sysdeps/unix/sysv/linux/x86_64/lowlevellock.h: Don't include

>>>         <tcb-offsets.h>.

>>

>>

>> It looks to me that a cleaner fix would be to move the definition of the TCB

>> types to a separate header.

>

> I will take a look.


sysdeps/i386/nptl/tcb-offsets.sym has

#include <sysdep.h>
#include <tls.h>
#include <kernel-features.h>

RESULT offsetof (struct pthread, result)
TID offsetof (struct pthread, tid)
CANCELHANDLING offsetof (struct pthread, cancelhandling)
CLEANUP_JMP_BUF offsetof (struct pthread, cleanup_jmp_buf)
MULTIPLE_THREADS_OFFSET offsetof (tcbhead_t, multiple_threads)
SYSINFO_OFFSET offsetof (tcbhead_t, sysinfo)
CLEANUP offsetof (struct pthread, cleanup)
CLEANUP_PREV offsetof (struct _pthread_cleanup_buffer, __prev)
MUTEX_FUTEX offsetof (pthread_mutex_t, __data.__lock)
POINTER_GUARD offsetof (tcbhead_t, pointer_guard)
#ifndef __ASSUME_PRIVATE_FUTEX
PRIVATE_FUTEX offsetof (tcbhead_t, private_futex)
#endif

It covers more than just tcbhead_t.  A separate header file for tcbhead_t
won't help here.

>> The circular dependency is just a cosmetic warning from make, right?

>

> It depends on how unlucky you are.  In one case on a many-core machine

> with make -j60, I had to kill glibc build since it never stopped.

>

>

> --

> H.J.




-- 
H.J.
Florian Weimer Feb. 22, 2018, 3:11 p.m. | #4
On 02/21/2018 06:19 PM, H.J. Lu wrote:
> On Wed, Feb 21, 2018 at 7:40 AM, H.J. Lu <hjl.tools@gmail.com> wrote:

>> On Tue, Feb 20, 2018 at 11:09 PM, Florian Weimer <fweimer@redhat.com> wrote:

>>> On 02/08/2018 06:58 PM, H.J. Lu wrote:

>>>>

>>>>          [BZ #22792]

>>>>          * Makerules ($(common-objpfx)%.h): Pass -DGEN_AS_CONST_HEADERS

>>>>          to $(CC).

>>>>          * sysdeps/unix/sysv/linux/i386/lowlevellock.h: Include

>>>>          <tcb-offsets.h> only if GEN_AS_CONST_HEADERS isn't defined.

>>>>          * sysdeps/unix/sysv/linux/x86_64/lowlevellock.h: Don't include

>>>>          <tcb-offsets.h>.

>>>

>>>

>>> It looks to me that a cleaner fix would be to move the definition of the TCB

>>> types to a separate header.

>>

>> I will take a look.

> 

> sysdeps/i386/nptl/tcb-offsets.sym has

> 

> #include <sysdep.h>

> #include <tls.h>

> #include <kernel-features.h>

> 

> RESULT offsetof (struct pthread, result)

> TID offsetof (struct pthread, tid)

> CANCELHANDLING offsetof (struct pthread, cancelhandling)

> CLEANUP_JMP_BUF offsetof (struct pthread, cleanup_jmp_buf)

> MULTIPLE_THREADS_OFFSET offsetof (tcbhead_t, multiple_threads)

> SYSINFO_OFFSET offsetof (tcbhead_t, sysinfo)

> CLEANUP offsetof (struct pthread, cleanup)

> CLEANUP_PREV offsetof (struct _pthread_cleanup_buffer, __prev)

> MUTEX_FUTEX offsetof (pthread_mutex_t, __data.__lock)

> POINTER_GUARD offsetof (tcbhead_t, pointer_guard)

> #ifndef __ASSUME_PRIVATE_FUTEX

> PRIVATE_FUTEX offsetof (tcbhead_t, private_futex)

> #endif

> 

> It covers more than just tcbhead_t.  A separate header file for tcbhead_t

> won't help here.


Yes, it's more involved.

>>> The circular dependency is just a cosmetic warning from make, right?

>>

>> It depends on how unlucky you are.  In one case on a many-core machine

>> with make -j60, I had to kill glibc build since it never stopped.


If it fixes an actual build issue, then I think your patch is 
acceptable, although I consider it quite hackish.

But please do add a comment explaining the issue before:

+# ifndef GEN_AS_CONST_HEADERS

Thanks,
Florian
Carlos O'Donell Feb. 22, 2018, 4:46 p.m. | #5
On 02/22/2018 07:11 AM, Florian Weimer wrote:
> On 02/21/2018 06:19 PM, H.J. Lu wrote:

>> On Wed, Feb 21, 2018 at 7:40 AM, H.J. Lu <hjl.tools@gmail.com> wrote:

>>> On Tue, Feb 20, 2018 at 11:09 PM, Florian Weimer <fweimer@redhat.com> wrote:

>>>> On 02/08/2018 06:58 PM, H.J. Lu wrote:

>>>>>

>>>>>          [BZ #22792]

>>>>>          * Makerules ($(common-objpfx)%.h): Pass -DGEN_AS_CONST_HEADERS

>>>>>          to $(CC).

>>>>>          * sysdeps/unix/sysv/linux/i386/lowlevellock.h: Include

>>>>>          <tcb-offsets.h> only if GEN_AS_CONST_HEADERS isn't defined.

>>>>>          * sysdeps/unix/sysv/linux/x86_64/lowlevellock.h: Don't include

>>>>>          <tcb-offsets.h>.

>>>>

>>>>

>>>> It looks to me that a cleaner fix would be to move the definition of the TCB

>>>> types to a separate header.

>>>

>>> I will take a look.

>>

>> sysdeps/i386/nptl/tcb-offsets.sym has

>>

>> #include <sysdep.h>

>> #include <tls.h>

>> #include <kernel-features.h>

>>

>> RESULT offsetof (struct pthread, result)

>> TID offsetof (struct pthread, tid)

>> CANCELHANDLING offsetof (struct pthread, cancelhandling)

>> CLEANUP_JMP_BUF offsetof (struct pthread, cleanup_jmp_buf)

>> MULTIPLE_THREADS_OFFSET offsetof (tcbhead_t, multiple_threads)

>> SYSINFO_OFFSET offsetof (tcbhead_t, sysinfo)

>> CLEANUP offsetof (struct pthread, cleanup)

>> CLEANUP_PREV offsetof (struct _pthread_cleanup_buffer, __prev)

>> MUTEX_FUTEX offsetof (pthread_mutex_t, __data.__lock)

>> POINTER_GUARD offsetof (tcbhead_t, pointer_guard)

>> #ifndef __ASSUME_PRIVATE_FUTEX

>> PRIVATE_FUTEX offsetof (tcbhead_t, private_futex)

>> #endif

>>

>> It covers more than just tcbhead_t.  A separate header file for tcbhead_t

>> won't help here.

> 

> Yes, it's more involved.

> 

>>>> The circular dependency is just a cosmetic warning from make, right?

>>>

>>> It depends on how unlucky you are.  In one case on a many-core machine

>>> with make -j60, I had to kill glibc build since it never stopped.

> 

> If it fixes an actual build issue, then I think your patch is acceptable, although I consider it quite hackish.

> 

> But please do add a comment explaining the issue before:

> 

> +# ifndef GEN_AS_CONST_HEADERS


Agreed, I think the hack is OK, but needs a multi-line comment
explaining *why* the simple route of header disentanglement
is not easily possible and why this is needed to bootstrap
the automatic header building without a circular dependency.

-- 
Cheers,
Carlos.
H.J. Lu Feb. 23, 2018, 7:13 p.m. | #6
On Thu, Feb 22, 2018 at 8:46 AM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 02/22/2018 07:11 AM, Florian Weimer wrote:

>> On 02/21/2018 06:19 PM, H.J. Lu wrote:

>>> On Wed, Feb 21, 2018 at 7:40 AM, H.J. Lu <hjl.tools@gmail.com> wrote:

>>>> On Tue, Feb 20, 2018 at 11:09 PM, Florian Weimer <fweimer@redhat.com> wrote:

>>>>> On 02/08/2018 06:58 PM, H.J. Lu wrote:

>>>>>>

>>>>>>          [BZ #22792]

>>>>>>          * Makerules ($(common-objpfx)%.h): Pass -DGEN_AS_CONST_HEADERS

>>>>>>          to $(CC).

>>>>>>          * sysdeps/unix/sysv/linux/i386/lowlevellock.h: Include

>>>>>>          <tcb-offsets.h> only if GEN_AS_CONST_HEADERS isn't defined.

>>>>>>          * sysdeps/unix/sysv/linux/x86_64/lowlevellock.h: Don't include

>>>>>>          <tcb-offsets.h>.

>>>>>

>>>>>

>>>>> It looks to me that a cleaner fix would be to move the definition of the TCB

>>>>> types to a separate header.

>>>>

>>>> I will take a look.

>>>

>>> sysdeps/i386/nptl/tcb-offsets.sym has

>>>

>>> #include <sysdep.h>

>>> #include <tls.h>

>>> #include <kernel-features.h>

>>>

>>> RESULT offsetof (struct pthread, result)

>>> TID offsetof (struct pthread, tid)

>>> CANCELHANDLING offsetof (struct pthread, cancelhandling)

>>> CLEANUP_JMP_BUF offsetof (struct pthread, cleanup_jmp_buf)

>>> MULTIPLE_THREADS_OFFSET offsetof (tcbhead_t, multiple_threads)

>>> SYSINFO_OFFSET offsetof (tcbhead_t, sysinfo)

>>> CLEANUP offsetof (struct pthread, cleanup)

>>> CLEANUP_PREV offsetof (struct _pthread_cleanup_buffer, __prev)

>>> MUTEX_FUTEX offsetof (pthread_mutex_t, __data.__lock)

>>> POINTER_GUARD offsetof (tcbhead_t, pointer_guard)

>>> #ifndef __ASSUME_PRIVATE_FUTEX

>>> PRIVATE_FUTEX offsetof (tcbhead_t, private_futex)

>>> #endif

>>>

>>> It covers more than just tcbhead_t.  A separate header file for tcbhead_t

>>> won't help here.

>>

>> Yes, it's more involved.

>>

>>>>> The circular dependency is just a cosmetic warning from make, right?

>>>>

>>>> It depends on how unlucky you are.  In one case on a many-core machine

>>>> with make -j60, I had to kill glibc build since it never stopped.

>>

>> If it fixes an actual build issue, then I think your patch is acceptable, although I consider it quite hackish.

>>

>> But please do add a comment explaining the issue before:

>>

>> +# ifndef GEN_AS_CONST_HEADERS

>

> Agreed, I think the hack is OK, but needs a multi-line comment

> explaining *why* the simple route of header disentanglement

> is not easily possible and why this is needed to bootstrap

> the automatic header building without a circular dependency.

>


This is the patch I am checking in.

Thanks.


-- 
H.J.
From 55a426ef8c54eb1d6e48b5858eb43496b1742797 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Thu, 8 Feb 2018 08:37:54 -0800
Subject: [PATCH] Define GEN_AS_CONST_HEADERS when generating header files [BZ
 #22792]

Glibc build generates header files to define constants from special .sym
files.  If a .sym file includes the same header file which it generates,
it leads to circular dependency which may lead to build hang on a
many-core machine.  Define GEN_AS_CONST_HEADERS when generating header
files to avoid circular dependency.

<tcb-offsets.h> is needed for i686 and it isn't needed for x86-64 at
least since glibc 2.23.

Tested on i686 and x86-64.

	[BZ #22792]
	* Makerules ($(common-objpfx)%.h): Pass -DGEN_AS_CONST_HEADERS
	to $(CC).
	* sysdeps/unix/sysv/linux/i386/lowlevellock.h: Include
	<tcb-offsets.h> only if GEN_AS_CONST_HEADERS isn't defined.
	* sysdeps/unix/sysv/linux/x86_64/lowlevellock.h: Don't include
	<tcb-offsets.h>.
---
 Makerules                                     | 9 ++++++++-
 sysdeps/unix/sysv/linux/i386/lowlevellock.h   | 9 ++++++++-
 sysdeps/unix/sysv/linux/x86_64/lowlevellock.h | 1 -
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/Makerules b/Makerules
index ef6abeac6d..b2c2724fcb 100644
--- a/Makerules
+++ b/Makerules
@@ -276,10 +276,17 @@ ifdef gen-as-const-headers
 # Generating headers for assembly constants.
 # We need this defined early to get into before-compile before
 # it's used in sysd-rules, below.
+# Define GEN_AS_CONST_HEADERS to avoid circular dependency [BZ #22792].
+# NB: <tcb-offsets.h> is generated from tcb-offsets.sym to define
+# offsets and sizes of types in <tls.h> and maybe <pthread.h> which
+# may include <tcb-offsets.h>.  Target header files can check if
+# GEN_AS_CONST_HEADERS is defined to avoid circular dependency which
+# may lead to build hang on a many-core machine.
 $(common-objpfx)%.h $(common-objpfx)%.h.d: $(..)scripts/gen-as-const.awk \
 					   %.sym $(common-before-compile)
 	$(AWK) -f $< $(filter %.sym,$^) \
-	| $(CC) -S -o $(@:.h.d=.h)T3 $(CFLAGS) $(CPPFLAGS) -x c - \
+	| $(CC) -S -o $(@:.h.d=.h)T3 $(CFLAGS) $(CPPFLAGS) \
+		-DGEN_AS_CONST_HEADERS -x c - \
 		-MD -MP -MF $(@:.h=.h.d)T -MT '$(@:.h=.h.d) $(@:.h.d=.h)'
 	sed -n 's/^.*@@@name@@@\([^@]*\)@@@value@@@[^0-9Xxa-fA-F-]*\([0-9Xxa-fA-F-][0-9Xxa-fA-F-]*\).*@@@end@@@.*$$/#define \1 \2/p' \
 		$(@:.h.d=.h)T3 > $(@:.h.d=.h)T
diff --git a/sysdeps/unix/sysv/linux/i386/lowlevellock.h b/sysdeps/unix/sysv/linux/i386/lowlevellock.h
index fb59b57934..38fbc2556f 100644
--- a/sysdeps/unix/sysv/linux/i386/lowlevellock.h
+++ b/sysdeps/unix/sysv/linux/i386/lowlevellock.h
@@ -26,7 +26,14 @@
 # include <sys/param.h>
 # include <bits/pthreadtypes.h>
 # include <kernel-features.h>
-# include <tcb-offsets.h>
+/* <tcb-offsets.h> is generated from tcb-offsets.sym to define offsets
+   and sizes of types in <tls.h> as well as <pthread.h> which includes
+   <lowlevellock.h> via nptl/descr.h.  Don't include <tcb-offsets.h>
+   when generating <tcb-offsets.h> to avoid circular dependency which
+   may lead to build hang on a many-core machine.  */
+# ifndef GEN_AS_CONST_HEADERS
+#  include <tcb-offsets.h>
+# endif
 
 # ifndef LOCK_INSTR
 #  ifdef UP
diff --git a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h
index a8bcfbe4a3..eedb6fc990 100644
--- a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h
+++ b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h
@@ -26,7 +26,6 @@
 # include <sys/param.h>
 # include <bits/pthreadtypes.h>
 # include <kernel-features.h>
-# include <tcb-offsets.h>
 
 # ifndef LOCK_INSTR
 #  ifdef UP

Patch

diff --git a/Makerules b/Makerules
index ef6abeac6d..10a2e83468 100644
--- a/Makerules
+++ b/Makerules
@@ -276,10 +276,12 @@  ifdef gen-as-const-headers
 # Generating headers for assembly constants.
 # We need this defined early to get into before-compile before
 # it's used in sysd-rules, below.
+# Define GEN_AS_CONST_HEADERS to avoid circular dependency [BZ #22792].
 $(common-objpfx)%.h $(common-objpfx)%.h.d: $(..)scripts/gen-as-const.awk \
 					   %.sym $(common-before-compile)
 	$(AWK) -f $< $(filter %.sym,$^) \
-	| $(CC) -S -o $(@:.h.d=.h)T3 $(CFLAGS) $(CPPFLAGS) -x c - \
+	| $(CC) -S -o $(@:.h.d=.h)T3 $(CFLAGS) $(CPPFLAGS) \
+		-DGEN_AS_CONST_HEADERS -x c - \
 		-MD -MP -MF $(@:.h=.h.d)T -MT '$(@:.h=.h.d) $(@:.h.d=.h)'
 	sed -n 's/^.*@@@name@@@\([^@]*\)@@@value@@@[^0-9Xxa-fA-F-]*\([0-9Xxa-fA-F-][0-9Xxa-fA-F-]*\).*@@@end@@@.*$$/#define \1 \2/p' \
 		$(@:.h.d=.h)T3 > $(@:.h.d=.h)T
diff --git a/sysdeps/unix/sysv/linux/i386/lowlevellock.h b/sysdeps/unix/sysv/linux/i386/lowlevellock.h
index fb59b57934..9f081e8a05 100644
--- a/sysdeps/unix/sysv/linux/i386/lowlevellock.h
+++ b/sysdeps/unix/sysv/linux/i386/lowlevellock.h
@@ -26,7 +26,9 @@ 
 # include <sys/param.h>
 # include <bits/pthreadtypes.h>
 # include <kernel-features.h>
-# include <tcb-offsets.h>
+# ifndef GEN_AS_CONST_HEADERS
+#  include <tcb-offsets.h>
+# endif
 
 # ifndef LOCK_INSTR
 #  ifdef UP
diff --git a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h
index a8bcfbe4a3..eedb6fc990 100644
--- a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h
+++ b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h
@@ -26,7 +26,6 @@ 
 # include <sys/param.h>
 # include <bits/pthreadtypes.h>
 # include <kernel-features.h>
-# include <tcb-offsets.h>
 
 # ifndef LOCK_INSTR
 #  ifdef UP