nptl: Open libgcc.so with RTLD_NOW during pthread_cancel

Message ID 20180110104903.A4F88439942E1@oldenburg.str.redhat.com
State New
Headers show
Series
  • nptl: Open libgcc.so with RTLD_NOW during pthread_cancel
Related show

Commit Message

Florian Weimer Jan. 10, 2018, 10:49 a.m.
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.

Comments

Adhemerval Zanella Jan. 10, 2018, 11:10 a.m. | #1
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
>
Carlos O'Donell Jan. 10, 2018, 7:09 p.m. | #2
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 ?
Florian Weimer Jan. 10, 2018, 7:20 p.m. | #3
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
Carlos O'Donell Jan. 10, 2018, 7:29 p.m. | #4
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?
Florian Weimer Jan. 10, 2018, 8:03 p.m. | #5
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);
Carlos O'Donell Jan. 10, 2018, 8:21 p.m. | #6
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>

Patch

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