On Thu, 2016-09-15 at 06:05 -0700, Ian Lance Taylor wrote: > Jakub Jelinek <ja...@redhat.com> writes: > > > 2016-09-15 Jakub Jelinek <ja...@redhat.com> > > > > PR libgcc/71744 > > * unwind-dw2-fde.c (ATOMIC_FDE_FAST_PATH): Define if __register_frame* > > is not the primary registry and atomics are available. > > (any_objects_registered): New variable. > > (__register_frame_info_bases, __register_frame_info_table_bases): > > Atomically store 1 to any_objects_registered after registering first > > unwind info. > > (_Unwind_Find_FDE): Return early if any_objects_registered is 0. > > This is OK.
In glibc, we have a rule that we add sufficient code comments for glibc. I'm not in the position to set rules for GCC, but I think this would help here too. Trying to explain in comments why a certain memory order is used would have at least brought up the issue you mention below. > > +#ifdef ATOMIC_FDE_FAST_PATH > > + /* For targets where unwind info is usually not registered through these > > + APIs anymore, avoid taking a global lock. */ > > + if (__builtin_expect (!__atomic_load_n (&any_objects_registered, > > + __ATOMIC_ACQUIRE), 1)) > > + return NULL; > > +#endif > > + > > init_object_mutex_once (); > > __gthread_mutex_lock (&object_mutex); > > I doubt it matters, but I don't think you need to use __ATOMIC_ACQUIRE > in the atomic_load_n. You could use __ATOMIC_RELAXED. Acquiring the > mutex is going to enforce cross-thread sequential consistency anyhow. But then the release MO wouldn't be required either. The __gthread_mutex_lock call has no synchronizes-with relation with the release MO stores the patch adds, if that was what you were assuming. Note that in the C11 / C++11 memory model, lock acquisitions are not considered to automatically also be acquire MO fences. This may be the case on many archs, but it's not something the memory model guarantees.
