[libfortran] PR 88137 Initialize backtrace state once

Message ID 20181122091724.5193-1-blomqvist.janne@gmail.com
State New
Headers show
Series
  • [libfortran] PR 88137 Initialize backtrace state once
Related show

Commit Message

Janne Blomqvist Nov. 22, 2018, 9:17 a.m.
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  <jb@gcc.gnu.org>

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

-- 
2.17.1

Comments

Janne Blomqvist Nov. 30, 2018, 7:37 a.m. | #1
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 <blomqvist.janne@gmail.com>
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  <jb@gcc.gnu.org>

>

>         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
Jerry DeLisle Nov. 30, 2018, 2:36 p.m. | #2
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 <blomqvist.janne@gmail.com>

> 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  <jb@gcc.gnu.org>

>>

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

>>

>>

>
Thomas Schwinge Nov. 30, 2018, 7:06 p.m. | #3
Hi!

On Thu, 22 Nov 2018 11:17:24 +0200, Janne Blomqvist <blomqvist.janne@gmail.com> 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?


Grüße
 Thomas
Janne Blomqvist Nov. 30, 2018, 7:29 p.m. | #4
On Fri, Nov 30, 2018 at 9:07 PM Thomas Schwinge <thomas@codesourcery.com>
wrote:

> Hi!

>

> On Thu, 22 Nov 2018 11:17:24 +0200, Janne Blomqvist <

> blomqvist.janne@gmail.com> 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
Janne Blomqvist Nov. 30, 2018, 9:28 p.m. | #5
On Fri, Nov 30, 2018 at 9:29 PM Janne Blomqvist <blomqvist.janne@gmail.com>
wrote:

> On Fri, Nov 30, 2018 at 9:07 PM Thomas Schwinge <thomas@codesourcery.com>

> wrote:

>

>> Hi!

>>

>> On Thu, 22 Nov 2018 11:17:24 +0200, Janne Blomqvist <

>> blomqvist.janne@gmail.com> 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?

>


Using atomics for accessing the static variable can be done as below,
passes regtesting, Ok for trunk/8/7 with a ChangeLog?  If no objections,
I'll commit it as obvious in a few days.

diff --git a/libgfortran/runtime/backtrace.c
b/libgfortran/runtime/backtrace.c
index 3fc973a5e6d..93ea14af19d 100644
--- a/libgfortran/runtime/backtrace.c
+++ b/libgfortran/runtime/backtrace.c
@@ -149,15 +149,20 @@ show_backtrace (bool in_signal_handler)
   /* 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;
+  static struct backtrace_state *lbstate_saved;
+  struct backtrace_state *lbstate;
   struct mystate state = { 0, false, in_signal_handler };

+  lbstate = __atomic_load_n (&lbstate_saved, __ATOMIC_RELAXED);
   if (!lbstate)
-    lbstate = backtrace_create_state (NULL, __gthread_active_p (),
-                                     error_callback, NULL);
-
-  if (lbstate == NULL)
-    return;
+    {
+      lbstate = backtrace_create_state (NULL, __gthread_active_p (),
+                                       error_callback, NULL);
+      if (lbstate)
+       __atomic_store_n (&lbstate_saved, lbstate, __ATOMIC_RELAXED);
+      else
+       return;
+    }

   if (!BACKTRACE_SUPPORTED || (in_signal_handler && BACKTRACE_USES_MALLOC))
     {


-- 
Janne Blomqvist

Patch

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;