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.