Message ID | 20180208175818.GA8519@gmail.com |
---|---|
State | New |
Headers | show |
Series | Define GEN_AS_CONST_HEADERS when generating header files [BZ #22792] | expand |
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
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.
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.
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
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.
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.
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