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

[Andrew Pinski]

On Sun, Jul 24, 2016 at 8:01 AM, Gleb Natapov <g...@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.
The second issue is not all targets support atomics because some are
single threaded.
Another issue is CONSUME memory model should almost never be used; it
just decays into acquire anyways.


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

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

Thanks,
Andrew

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