This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH, libfortran] PR 88137 Initialize backtrace state once
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