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