| Message ID | 20180110104903.A4F88439942E1@oldenburg.str.redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | nptl: Open libgcc.so with RTLD_NOW during pthread_cancel | expand |
On 10/01/2018 08:49, Florian Weimer wrote: > Disabling lazy binding reduces stack usage during unwinding. > > Note that RTLD_NOW only makes a difference if libgcc.so has not > already been loaded, so this is only a partial fix. > > 2018-01-10 Florian Weimer <fweimer@redhat.com> > > [BZ #22636] > * sysdeps/nptl/unwind-forcedunwind.c (pthread_cancel_init): Open > libgcc.so with RTLD_NOW, to avoid lazy binding during unwind. Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> > > diff --git a/sysdeps/nptl/unwind-forcedunwind.c b/sysdeps/nptl/unwind-forcedunwind.c > index ab4350de99..67b8e74b53 100644 > --- a/sysdeps/nptl/unwind-forcedunwind.c > +++ b/sysdeps/nptl/unwind-forcedunwind.c > @@ -49,7 +49,7 @@ pthread_cancel_init (void) > return; > } > > - handle = __libc_dlopen (LIBGCC_S_SO); > + handle = __libc_dlopen_mode (LIBGCC_S_SO, RTLD_NOW | __RTLD_DLOPEN); > > if (handle == NULL > || (resume = __libc_dlsym (handle, "_Unwind_Resume")) == NULL >
On 01/10/2018 02:49 AM, Florian Weimer wrote: > Disabling lazy binding reduces stack usage during unwinding. > > Note that RTLD_NOW only makes a difference if libgcc.so has not > already been loaded, so this is only a partial fix. > > 2018-01-10 Florian Weimer <fweimer@redhat.com> > > [BZ #22636] > * sysdeps/nptl/unwind-forcedunwind.c (pthread_cancel_init): Open > libgcc.so with RTLD_NOW, to avoid lazy binding during unwind. > > diff --git a/sysdeps/nptl/unwind-forcedunwind.c b/sysdeps/nptl/unwind-forcedunwind.c > index ab4350de99..67b8e74b53 100644 > --- a/sysdeps/nptl/unwind-forcedunwind.c > +++ b/sysdeps/nptl/unwind-forcedunwind.c > @@ -49,7 +49,7 @@ pthread_cancel_init (void) > return; > } > > - handle = __libc_dlopen (LIBGCC_S_SO); > + handle = __libc_dlopen_mode (LIBGCC_S_SO, RTLD_NOW | __RTLD_DLOPEN); > > if (handle == NULL > || (resume = __libc_dlsym (handle, "_Unwind_Resume")) == NULL > Should we not also do this for sysdeps/gnu/unwind-resume.c ?
On 01/10/2018 08:09 PM, Carlos O'Donell wrote: > On 01/10/2018 02:49 AM, Florian Weimer wrote: >> Disabling lazy binding reduces stack usage during unwinding. >> >> Note that RTLD_NOW only makes a difference if libgcc.so has not >> already been loaded, so this is only a partial fix. >> >> 2018-01-10 Florian Weimer <fweimer@redhat.com> >> >> [BZ #22636] >> * sysdeps/nptl/unwind-forcedunwind.c (pthread_cancel_init): Open >> libgcc.so with RTLD_NOW, to avoid lazy binding during unwind. >> >> diff --git a/sysdeps/nptl/unwind-forcedunwind.c b/sysdeps/nptl/unwind-forcedunwind.c >> index ab4350de99..67b8e74b53 100644 >> --- a/sysdeps/nptl/unwind-forcedunwind.c >> +++ b/sysdeps/nptl/unwind-forcedunwind.c >> @@ -49,7 +49,7 @@ pthread_cancel_init (void) >> return; >> } >> >> - handle = __libc_dlopen (LIBGCC_S_SO); >> + handle = __libc_dlopen_mode (LIBGCC_S_SO, RTLD_NOW | __RTLD_DLOPEN); >> >> if (handle == NULL >> || (resume = __libc_dlsym (handle, "_Unwind_Resume")) == NULL >> > > Should we not also do this for sysdeps/gnu/unwind-resume.c ? Interesting. _Unwind_Resume is only called from a landing pad after unwinding has already begun, so the code never actually loads libgcc_s, and RTLD_NOW does not have any effect at this point (even with lazy binding, most of the unwinder will already have been bound by this point). In contrast, the nptl unwinding code used by pthread_exit/pthread_cancel actually initiates unwinding. There is certainly a cleanup opportunity here, but changing the dlopen flags is not needed at this point. Thanks, Florian
On 01/10/2018 11:20 AM, Florian Weimer wrote: > On 01/10/2018 08:09 PM, Carlos O'Donell wrote: >> On 01/10/2018 02:49 AM, Florian Weimer wrote: >>> Disabling lazy binding reduces stack usage during unwinding. >>> >>> Note that RTLD_NOW only makes a difference if libgcc.so has not >>> already been loaded, so this is only a partial fix. >>> >>> 2018-01-10 Florian Weimer <fweimer@redhat.com> >>> >>> [BZ #22636] >>> * sysdeps/nptl/unwind-forcedunwind.c (pthread_cancel_init): Open >>> libgcc.so with RTLD_NOW, to avoid lazy binding during unwind. >>> >>> diff --git a/sysdeps/nptl/unwind-forcedunwind.c b/sysdeps/nptl/unwind-forcedunwind.c >>> index ab4350de99..67b8e74b53 100644 >>> --- a/sysdeps/nptl/unwind-forcedunwind.c >>> +++ b/sysdeps/nptl/unwind-forcedunwind.c >>> @@ -49,7 +49,7 @@ pthread_cancel_init (void) >>> return; >>> } >>> - handle = __libc_dlopen (LIBGCC_S_SO); >>> + handle = __libc_dlopen_mode (LIBGCC_S_SO, RTLD_NOW | __RTLD_DLOPEN); >>> if (handle == NULL >>> || (resume = __libc_dlsym (handle, "_Unwind_Resume")) == NULL >>> >> >> Should we not also do this for sysdeps/gnu/unwind-resume.c ? > > Interesting. > > _Unwind_Resume is only called from a landing pad after unwinding has > already begun, so the code never actually loads libgcc_s, and > RTLD_NOW does not have any effect at this point (even with lazy > binding, most of the unwinder will already have been bound by this > point). In contrast, the nptl unwinding code used by > pthread_exit/pthread_cancel actually initiates unwinding. > > There is certainly a cleanup opportunity here, but changing the > dlopen flags is not needed at this point. What I want to avoid is two similar pieces of code that do distinct things. Is there any reason we shouldn't just checkin a change here that makes both the same, thus avoiding differences?
On 01/10/2018 08:29 PM, Carlos O'Donell wrote: > What I want to avoid is two similar pieces of code that do distinct > things. Is there any reason we shouldn't just checkin a change here > that makes both the same, thus avoiding differences? Okay, then let's add a comment. Thanks, Florian Subject: [PATCH] csu: Update __libgcc_s_init comment To: libc-alpha@sourceware.org 2018-01-10 Florian Weimer <fweimer@redhat.com> * sysdeps/gnu/unwind-resume.c (__libgcc_s_init): Update comment and error message. diff --git a/sysdeps/gnu/unwind-resume.c b/sysdeps/gnu/unwind-resume.c index 94704c9a2b..7f9a1bf2c7 100644 --- a/sysdeps/gnu/unwind-resume.c +++ b/sysdeps/gnu/unwind-resume.c @@ -35,13 +35,17 @@ __libgcc_s_init (void) void *resume, *personality; void *handle; - handle = __libc_dlopen (LIBGCC_S_SO); + /* Use RTLD_NOW here for consistency with pthread_cancel_init. + RTLD_NOW will rarely make a difference here because unwinding is + already in progress, so libgcc_s.so has already been loaded if + its unwinder is used. */ + handle = __libc_dlopen_mode (LIBGCC_S_SO, RTLD_NOW | __RTLD_DLOPEN); if (handle == NULL || (resume = __libc_dlsym (handle, "_Unwind_Resume")) == NULL || (personality = __libc_dlsym (handle, "__gcc_personality_v0")) == NULL) __libc_fatal (LIBGCC_S_SO - " must be installed for pthread_cancel to work\n"); + " must be installed for unwinding to work\n"); #ifdef PTR_MANGLE PTR_MANGLE (resume);
On 01/10/2018 12:03 PM, Florian Weimer wrote: > On 01/10/2018 08:29 PM, Carlos O'Donell wrote: > >> What I want to avoid is two similar pieces of code that do distinct >> things. Is there any reason we shouldn't just checkin a change here >> that makes both the same, thus avoiding differences? > > Okay, then let's add a comment. Perfect. LGTM. Reviewed-by: Carlos O'Donell <carlos@redhat.com>
diff --git a/sysdeps/nptl/unwind-forcedunwind.c b/sysdeps/nptl/unwind-forcedunwind.c index ab4350de99..67b8e74b53 100644 --- a/sysdeps/nptl/unwind-forcedunwind.c +++ b/sysdeps/nptl/unwind-forcedunwind.c @@ -49,7 +49,7 @@ pthread_cancel_init (void) return; } - handle = __libc_dlopen (LIBGCC_S_SO); + handle = __libc_dlopen_mode (LIBGCC_S_SO, RTLD_NOW | __RTLD_DLOPEN); if (handle == NULL || (resume = __libc_dlsym (handle, "_Unwind_Resume")) == NULL