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.
