[PATCH, libfortran] PR 88137 Initialize backtrace state once

[Janne Blomqvist]

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 <***@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;

[Janne Blomqvist]

PING!

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

Post by Janne Blomqvist
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?
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

[Jerry DeLisle]

Post by Janne Blomqvist
PING!

LGTM,

OK and thanks

Jerry

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

Post by Janne Blomqvist
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?
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]

Hi!

Post by Janne Blomqvist
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...

Post by Janne Blomqvist
--- 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]

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

Post by Janne Blomqvist
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...

Post by Janne Blomqvist
--- a/libgfortran/runtime/backtrace.c
+++ b/libgfortran/runtime/backtrace.c
@@ -146,11 +146,15 @@ full_callback (void *data, uintptr_t pc, const

char *filename,

Post by Janne Blomqvist
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]

Post by Janne Blomqvist

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

Post by Janne Blomqvist
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...

Post by Janne Blomqvist
--- a/libgfortran/runtime/backtrace.c
+++ b/libgfortran/runtime/backtrace.c
@@ -146,11 +146,15 @@ full_callback (void *data, uintptr_t pc, const

char *filename,

Post by Janne Blomqvist
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))
{