[PATCH] nptl: Open libgcc.so with RTLD_NOW during pthread_cancel

[PATCH] nptl: Open libgcc.so with RTLD_NOW during pthread_cancel

[Florian Weimer]

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

Re: [PATCH] nptl: Open libgcc.so with RTLD_NOW during pthread_cancel

[Adhemerval Zanella]

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
> 

Re: [PATCH] nptl: Open libgcc.so with RTLD_NOW during pthread_cancel

[Carlos O'Donell]

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 ?

-- 
Cheers,
Carlos.

Re: [PATCH] nptl: Open libgcc.so with RTLD_NOW during pthread_cancel

[Florian Weimer]

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

Re: [PATCH] nptl: Open libgcc.so with RTLD_NOW during pthread_cancel

[Carlos O'Donell]

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?

-- 
Cheers,
Carlos.

Re: [PATCH] nptl: Open libgcc.so with RTLD_NOW during pthread_cancel

[Florian Weimer]

[-- Attachment #1: Type: text/plain, Size: 293 bytes --]

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

[-- Attachment #2: unwind.patch --]
[-- Type: text/x-patch, Size: 1226 bytes --]

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);

Re: [PATCH] nptl: Open libgcc.so with RTLD_NOW during pthread_cancel

[Carlos O'Donell]

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>

-- 
Cheers,
Carlos.