Re: [PATCH RFC] do not take mutex in _Unwind_Find_registered_FDE if there is no registered objects

[Gleb Natapov <gleb at scylladb dot com>]

Thank you for prompt review.

On Sun, Jul 24, 2016 at 11:00:53AM -0700, Andrew Pinski wrote:
> On Sun, Jul 24, 2016 at 8:01 AM, Gleb Natapov <gleb@scylladb.com> wrote:
> > _Unwind_Find_FDE calls _Unwind_Find_registered_FDE and it takes lock even
> > when there is no registered objects. As far as I see only statically
> > linked applications call __register_frame_info* functions, so for
> > dynamically linked executables taking the lock to check unseen_objects
> > and seen_objects is a pessimization. Since the function is called on
> > each thrown exception this is a lot of unneeded locking.  This patch
> > checks unseen_objects and seen_objects outside of the lock and returns
> > earlier if both are NULL.
> 
> There are problems with this patch.
> First the stores to unseen_objects and seen_objects are not atomic stores.
They are not atomic yes, but they are in locked section so there is a
release after the store which should order writes to both of them. I can
use __atomic_store_n for storing but I do not see (yet) why it is needed.

> The second issue is not all targets support atomics because some are
> single threaded.
__atomic builtins should compile to regular stores/loads on those, no? If
__atomic builtins cannot be used on those platforms can this be detected
with preprocessor macros somehow?

> Another issue is CONSUME memory model should almost never be used; it
> just decays into acquire anyways.
> 
> 
Hmm, yes, not sure why I used CONSUME here instead of ACQUIRE.

> >
> > diff --git a/libgcc/unwind-dw2-fde.c b/libgcc/unwind-dw2-fde.c
> > index 5b16a1f..9af558d 100644
> > --- a/libgcc/unwind-dw2-fde.c
> > +++ b/libgcc/unwind-dw2-fde.c
> > @@ -1001,6 +1001,10 @@ _Unwind_Find_FDE (void *pc, struct dwarf_eh_bases *bases)
> >    struct object *ob;
> >    const fde *f = NULL;
> >
> > +  if (!__atomic_load_n(&unseen_objects, __ATOMIC_CONSUME) && !__atomic_load_n(&seen_objects, __ATOMIC_CONSUME)) {
> > +      return NULL;
> > +  }
> 
> There is formatting issues too.
>   if (!__atomic_load_n(&unseen_objects, __ATOMIC_CONSUME)
>      && !__atomic_load_n(&seen_objects, __ATOMIC_CONSUME))
>     {
>       return NULL;
>     }
> 
> is the correct formating.
> 
OK.

> > +
> >    init_object_mutex_once ();
> >    __gthread_mutex_lock (&object_mutex);
> >
> > @@ -1020,8 +1024,8 @@ _Unwind_Find_FDE (void *pc, struct dwarf_eh_bases *bases)
> >    while ((ob = unseen_objects))
> >      {
> >        struct object **p;
> > +      struct object *next = ob->next;
> >
> > -      unseen_objects = ob->next;
> >        f = search_object (ob, pc);
> >
> >        /* Insert the object into the classified list.  */
> > @@ -1031,6 +1035,8 @@ _Unwind_Find_FDE (void *pc, struct dwarf_eh_bases *bases)
> >        ob->next = *p;
> >        *p = ob;
> >
> > +      unseen_objects = next;
> 
> This store should be an atomic store to be paired with the loads.
> 
This store is inside mutex, so unlock will have RELEASE.

> Thanks,
> Andrew
> 
> > +
> >        if (f)
> >         goto fini;
> >      }
> > --
> >                         Gleb.

--
			Gleb.