Re: [PATCH, libfortran] PR 88137 Initialize backtrace state once

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, libfortran] PR 88137 Initialize backtrace state once

Janne Blomqvist-3
PING!

(for the GCC-7 branch, I'll commit it after the 7.4 release)

On Thu, Nov 22, 2018 at 11:17 AM Janne Blomqvist <[hidden email]>
wrote:

> From backtrace.h for backtrace_create_state:
>
>    Calling this function allocates resources that can not be freed.
>    There is no backtrace_free_state function.  The state is used to
>    cache information that is expensive to recompute.  Programs are
>    expected to call this function at most once and to save the return
>    value for all later calls to backtrace functions.
>
> So instead of calling backtrace_create_state every time we wish to
> show a backtrace, do it once and store the result in a static
> variable.  libbacktrace allows multiple threads to access the state,
> so no need to use TLS.
>
> Regtested on x86_64-pc-linux-gnu, Ok for trunk/8/7?
>
> libgfortran/ChangeLog:
>
> 2018-11-22  Janne Blomqvist  <[hidden email]>
>
>         PR libfortran/88137
>         * runtime/backtrace.c (show_backtrace): Make lbstate a static
>         variable, initialize once.
> ---
>  libgfortran/runtime/backtrace.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/libgfortran/runtime/backtrace.c
> b/libgfortran/runtime/backtrace.c
> index e0c277044b6..3fc973a5e6d 100644
> --- a/libgfortran/runtime/backtrace.c
> +++ b/libgfortran/runtime/backtrace.c
> @@ -146,11 +146,15 @@ full_callback (void *data, uintptr_t pc, const char
> *filename,
>  void
>  show_backtrace (bool in_signal_handler)
>  {
> -  struct backtrace_state *lbstate;
> +  /* Note that libbacktrace allows the state to be accessed from
> +     multiple threads, so we don't need to use a TLS variable for the
> +     state here.  */
> +  static struct backtrace_state *lbstate;
>    struct mystate state = { 0, false, in_signal_handler };
> -
> -  lbstate = backtrace_create_state (NULL, __gthread_active_p (),
> -                                   error_callback, NULL);
> +
> +  if (!lbstate)
> +    lbstate = backtrace_create_state (NULL, __gthread_active_p (),
> +                                     error_callback, NULL);
>
>    if (lbstate == NULL)
>      return;
> --
> 2.17.1
>
>

--
Janne Blomqvist
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, libfortran] PR 88137 Initialize backtrace state once

jerry DeLisle-3
On 11/29/18 11:37 PM, Janne Blomqvist wrote:
> PING!
>

LGTM,

OK and thanks

Jerry

> (for the GCC-7 branch, I'll commit it after the 7.4 release)
>
> On Thu, Nov 22, 2018 at 11:17 AM Janne Blomqvist <[hidden email]>
> wrote:
>
>>  From backtrace.h for backtrace_create_state:
>>
>>     Calling this function allocates resources that can not be freed.
>>     There is no backtrace_free_state function.  The state is used to
>>     cache information that is expensive to recompute.  Programs are
>>     expected to call this function at most once and to save the return
>>     value for all later calls to backtrace functions.
>>
>> So instead of calling backtrace_create_state every time we wish to
>> show a backtrace, do it once and store the result in a static
>> variable.  libbacktrace allows multiple threads to access the state,
>> so no need to use TLS.
>>
>> Regtested on x86_64-pc-linux-gnu, Ok for trunk/8/7?
>>
>> libgfortran/ChangeLog:
>>
>> 2018-11-22  Janne Blomqvist  <[hidden email]>
>>
>>          PR libfortran/88137
>>          * runtime/backtrace.c (show_backtrace): Make lbstate a static
>>          variable, initialize once.
>> ---
>>   libgfortran/runtime/backtrace.c | 12 ++++++++----
>>   1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/libgfortran/runtime/backtrace.c
>> b/libgfortran/runtime/backtrace.c
>> index e0c277044b6..3fc973a5e6d 100644
>> --- a/libgfortran/runtime/backtrace.c
>> +++ b/libgfortran/runtime/backtrace.c
>> @@ -146,11 +146,15 @@ full_callback (void *data, uintptr_t pc, const char
>> *filename,
>>   void
>>   show_backtrace (bool in_signal_handler)
>>   {
>> -  struct backtrace_state *lbstate;
>> +  /* Note that libbacktrace allows the state to be accessed from
>> +     multiple threads, so we don't need to use a TLS variable for the
>> +     state here.  */
>> +  static struct backtrace_state *lbstate;
>>     struct mystate state = { 0, false, in_signal_handler };
>> -
>> -  lbstate = backtrace_create_state (NULL, __gthread_active_p (),
>> -                                   error_callback, NULL);
>> +
>> +  if (!lbstate)
>> +    lbstate = backtrace_create_state (NULL, __gthread_active_p (),
>> +                                     error_callback, NULL);
>>
>>     if (lbstate == NULL)
>>       return;
>> --
>> 2.17.1
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, libfortran] PR 88137 Initialize backtrace state once

Janne Blomqvist-3
In reply to this post by Janne Blomqvist-3
On Fri, Nov 30, 2018 at 9:07 PM Thomas Schwinge <[hidden email]>
wrote:

> Hi!
>
> On Thu, 22 Nov 2018 11:17:24 +0200, Janne Blomqvist <
> [hidden email]> wrote:
> > From backtrace.h for backtrace_create_state:
> >
> >    Calling this function allocates resources that can not be freed.
> >    There is no backtrace_free_state function.  The state is used to
> >    cache information that is expensive to recompute.  Programs are
> >    expected to call this function at most once and to save the return
> >    value for all later calls to backtrace functions.
> >
> > So instead of calling backtrace_create_state every time we wish to
> > show a backtrace, do it once and store the result in a static
> > variable.  libbacktrace allows multiple threads to access the state,
> > so no need to use TLS.
>
> Hmm, OK, but...
>
> > --- a/libgfortran/runtime/backtrace.c
> > +++ b/libgfortran/runtime/backtrace.c
> > @@ -146,11 +146,15 @@ full_callback (void *data, uintptr_t pc, const
> char *filename,
> >  void
> >  show_backtrace (bool in_signal_handler)
> >  {
> > -  struct backtrace_state *lbstate;
> > +  /* Note that libbacktrace allows the state to be accessed from
> > +     multiple threads, so we don't need to use a TLS variable for the
> > +     state here.  */
> > +  static struct backtrace_state *lbstate;
> >    struct mystate state = { 0, false, in_signal_handler };
> > -
> > -  lbstate = backtrace_create_state (NULL, __gthread_active_p (),
> > -                                 error_callback, NULL);
> > +
> > +  if (!lbstate)
> > +    lbstate = backtrace_create_state (NULL, __gthread_active_p (),
> > +                                   error_callback, NULL);
>
> ... don't you still have to make sure that only one thread ever executes
> "backtrace_create_state", and writes into "lbstate" (via locking, or
> atomics, or "pthread_once", or some such)?  Or is that situation (more
> than one thread entering "show_backtrace" with uninitialized "lbstate")
> not possible to happen for other reasons?
>

I thought about that, but I think it probably(?) doesn't matter.

- Two threads could race and run backtrace_create_state() in parallel, and
probably we'd waste some memory then, but that was already possible before.

- As for locking, the function can be called from a signal handler, so
pthread_mutex is out. I guess I could implement a spinlock with atomics,
but, meh, seems overkill.

- And using atomics to access lbstate, it's an aligned pointer anyway, so
while it's a race to access it, it shouldn't be possible to get a situation
with a corrupted value for the pointer, right? I could use
__atomic_load/store to access it, but that doesn't buy much in the end,
does it, since the problem is parallel initialization, and not non-atomic
access to the lbstate pointer? Or does gcc support targets with non-atomic
access to aligned pointers?

Or is there something I'm missing?


--
Janne Blomqvist