On Mon, Nov 18, 2013 at 12:14 PM, Jakub Jelinek <ja...@redhat.com> wrote:
>
> Sure, but GCC itself doesn't need the thread-safe stuff, and the target
> libbacktrace is built with gcc and will have __atomic_* support.
> You already have there configure checks for the __sync_* builtins, just
> replacing them with the __atomic_* stuff would DTRT.
>>
>> Of course we could use configure tests and so forth, but the code to
>> implement atomic loads using the sync builtins is simple enough that
>> I'm not sure it's worth it.
>
> It should be (at least on most targets) faster to use
> __atomic_load/__atomic_store, plus you could make it explicit what
> is an acquire, what is release semantics, what needs sequentially consistent
> atomic etc. And it would be more readable.
OK, fair enough. I committed this patch to use the atomic intrinsics
when they are available.
Ian
2013-11-18 Ian Lance Taylor <i...@google.com>
* configure.ac: Check for support of __atomic extensions.
* internal.h: Declare or #define atomic functions for use in
backtrace code.
* atomic.c: New file.
* dwarf.c (dwarf_lookup_pc): Use atomic functions.
(dwarf_fileline, backtrace_dwarf_add): Likewise.
* elf.c (elf_add_syminfo_data, elf_syminfo): Likewise.
(backtrace_initialize): Likewise.
* fileline.c (fileline_initialize): Likewise.
* Makefile.am (libbacktrace_la_SOURCES): Add atomic.c.
* configure, config.h.in, Makefile.in: Rebuild.
Index: atomic.c
===================================================================
--- atomic.c (revision 0)
+++ atomic.c (revision 0)
@@ -0,0 +1,111 @@
+/* atomic.c -- Support for atomic functions if not present.
+ Copyright (C) 2013 Free Software Foundation, Inc.
+ Written by Ian Lance Taylor, Google.
+
+Redistribution and use in source and binary forms, with or without
+modification, are permitted provided that the following conditions are
+met:
+
+ (1) Redistributions of source code must retain the above copyright
+ notice, this list of conditions and the following disclaimer.
+
+ (2) Redistributions in binary form must reproduce the above copyright
+ notice, this list of conditions and the following disclaimer in
+ the documentation and/or other materials provided with the
+ distribution.
+
+ (3) The name of the author may not be used to
+ endorse or promote products derived from this software without
+ specific prior written permission.
+
+THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
+IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+DISCLAIMED. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT,
+INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING
+IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+POSSIBILITY OF SUCH DAMAGE. */
+
+#include "config.h"
+
+#include "backtrace.h"
+#include "backtrace-supported.h"
+#include "internal.h"
+
+/* This file holds implementations of the atomic functions that are
+ used if the host compiler has the sync functions but not the atomic
+ functions, as is true of versions of GCC before 4.7. */
+
+#if !defined (HAVE_ATOMIC_FUNCTIONS) && defined (HAVE_SYNC_FUNCTIONS)
+
+/* Do an atomic load of a pointer. */
+
+void *
+backtrace_atomic_load_pointer (void *arg)
+{
+ void **pp;
+ void *p;
+
+ pp = (void **) arg;
+ p = *pp;
+ while (!__sync_bool_compare_and_swap (pp, p, p))
+ p = *pp;
+ return p;
+}
+
+/* Do an atomic load of an int. */
+
+int
+backtrace_atomic_load_int (int *p)
+{
+ int i;
+
+ i = *p;
+ while (!__sync_bool_compare_and_swap (p, i, i))
+ i = *p;
+ return i;
+}
+
+/* Do an atomic store of a pointer. */
+
+void
+backtrace_atomic_store_pointer (void *arg, void *p)
+{
+ void **pp;
+ void *old;
+
+ pp = (void **) arg;
+ old = *pp;
+ while (!__sync_bool_compare_and_swap (pp, old, p))
+ old = *pp;
+}
+
+/* Do an atomic store of a size_t value. */
+
+void
+backtrace_atomic_store_size_t (size_t *p, size_t v)
+{
+ size_t old;
+
+ old = *p;
+ while (!__sync_bool_compare_and_swap (p, old, v))
+ old = *p;
+}
+
+/* Do an atomic store of a int value. */
+
+void
+backtrace_atomic_store_int (int *p, int v)
+{
+ size_t old;
+
+ old = *p;
+ while (!__sync_bool_compare_and_swap (p, old, v))
+ old = *p;
+}
+
+#endif
Index: internal.h
===================================================================
--- internal.h (revision 204992)
+++ internal.h (working copy)
@@ -65,7 +65,48 @@ POSSIBILITY OF SUCH DAMAGE. */
#define __sync_lock_test_and_set(A, B) (abort(), 0)
#define __sync_lock_release(A) abort()
-#endif /* !defined(HAVE_SYNC_FUNCTIONS) */
+#endif /* !defined (HAVE_SYNC_FUNCTIONS) */
+
+#ifdef HAVE_ATOMIC_FUNCTIONS
+
+/* We have the atomic builtin functions. */
+
+#define backtrace_atomic_load_pointer(p) \
+ __atomic_load_n ((p), __ATOMIC_ACQUIRE)
+#define backtrace_atomic_load_int(p) \
+ __atomic_load_n ((p), __ATOMIC_ACQUIRE)
+#define backtrace_atomic_store_pointer(p, v) \
+ __atomic_store_n ((p), (v), __ATOMIC_RELEASE)
+#define backtrace_atomic_store_size_t(p, v) \
+ __atomic_store_n ((p), (v), __ATOMIC_RELEASE)
+#define backtrace_atomic_store_int(p, v) \
+ __atomic_store_n ((p), (v), __ATOMIC_RELEASE)
+
+#else /* !defined (HAVE_ATOMIC_FUNCTIONS) */
+#ifdef HAVE_SYNC_FUNCTIONS
+
+/* We have the sync functions but not the atomic functions. Define
+ the atomic ones in terms of the sync ones. */
+
+extern void *backtrace_atomic_load_pointer (void *);
+extern int backtrace_atomic_load_int (int *);
+extern void backtrace_atomic_store_pointer (void *, void *);
+extern void backtrace_atomic_store_size_t (size_t *, size_t);
+extern void backtrace_atomic_store_int (int *, int);
+
+#else /* !defined (HAVE_SYNC_FUNCTIONS) */
+
+/* We have neither the sync nor the atomic functions. These will
+ never be called. */
+
+#define backtrace_atomic_load_pointer(p) (abort(), 0)
+#define backtrace_atomic_load_int(p) (abort(), 0)
+#define backtrace_atomic_store_pointer(p, v) abort()
+#define backtrace_atomic_store_size_t(p, v) abort()
+#define backtrace_atomic_store_int(p, v) abort()
+
+#endif /* !defined (HAVE_SYNC_FUNCTIONS) */
+#endif /* !defined (HAVE_ATOMIC_FUNCTIONS) */
/* The type of the function that collects file/line information. This
is like backtrace_pcinfo. */
Index: configure.ac
===================================================================
--- configure.ac (revision 204992)
+++ configure.ac (working copy)
@@ -194,6 +194,24 @@ if test "$libbacktrace_cv_sys_sync" = "y
fi
AC_SUBST(BACKTRACE_SUPPORTS_THREADS)
+# Test for __atomic support.
+AC_CACHE_CHECK([__atomic extensions],
+[libbacktrace_cv_sys_atomic],
+[if test -n "${with_target_subdir}"; then
+ libbacktrace_cv_sys_atomic=yes
+ else
+ AC_LINK_IFELSE(
+ [AC_LANG_PROGRAM([int i;],
+ [__atomic_load_n (&i, __ATOMIC_ACQUIRE);
+ __atomic_store_n (&i, 1, __ATOMIC_RELEASE);])],
+ [libbacktrace_cv_sys_atomic=yes],
+ [libbacktrace_cv_sys_atomic=no])
+ fi])
+if test "$libbacktrace_cv_sys_atomic" = "yes"; then
+ AC_DEFINE([HAVE_ATOMIC_FUNCTIONS], 1,
+ [Define to 1 if you have the __atomic functions])
+fi
+
# The library needs to be able to read the executable itself. Compile
# a file to determine the executable format. The awk script
# filetype.awk prints out the file type.
Index: fileline.c
===================================================================
--- fileline.c (revision 204992)
+++ fileline.c (working copy)
@@ -58,15 +58,10 @@ fileline_initialize (struct backtrace_st
int called_error_callback;
int descriptor;
- failed = state->fileline_initialization_failed;
-
- if (state->threaded)
- {
- /* Use __sync_bool_compare_and_swap to do an atomic load. */
- while (!__sync_bool_compare_and_swap
- (&state->fileline_initialization_failed, failed, failed))
- failed = state->fileline_initialization_failed;
- }
+ if (!state->threaded)
+ failed = state->fileline_initialization_failed;
+ else
+ failed = backtrace_atomic_load_int (&state->fileline_initialization_failed);
if (failed)
{
@@ -74,13 +69,10 @@ fileline_initialize (struct backtrace_st
return 0;
}
- fileline_fn = state->fileline_fn;
- if (state->threaded)
- {
- while (!__sync_bool_compare_and_swap (&state->fileline_fn, fileline_fn,
- fileline_fn))
- fileline_fn = state->fileline_fn;
- }
+ if (!state->threaded)
+ fileline_fn = state->fileline_fn;
+ else
+ fileline_fn = backtrace_atomic_load_pointer (&state->fileline_fn);
if (fileline_fn != NULL)
return 1;
@@ -151,8 +143,7 @@ fileline_initialize (struct backtrace_st
if (!state->threaded)
state->fileline_initialization_failed = 1;
else
- __sync_bool_compare_and_swap (&state->fileline_initialization_failed,
- 0, failed);
+ backtrace_atomic_store_int (&state->fileline_initialization_failed, 1);
return 0;
}
@@ -160,15 +151,10 @@ fileline_initialize (struct backtrace_st
state->fileline_fn = fileline_fn;
else
{
- __sync_bool_compare_and_swap (&state->fileline_fn, NULL, fileline_fn);
+ backtrace_atomic_store_pointer (&state->fileline_fn, fileline_fn);
- /* At this point we know that state->fileline_fn is not NULL.
- Either we stored our value, or some other thread stored its
- value. If some other thread stored its value, we leak the
- one we just initialized. Either way, state->fileline_fn is
- initialized. The compare_and_swap is a full memory barrier,
- so we should have full access to that value even if it was
- created by another thread. */
+ /* Note that if two threads initialize at once, one of the data
+ sets may be leaked. */
}
return 1;
Index: Makefile.am
===================================================================
--- Makefile.am (revision 204992)
+++ Makefile.am (working copy)
@@ -40,6 +40,7 @@ noinst_LTLIBRARIES = libbacktrace.la
libbacktrace_la_SOURCES = \
backtrace.h \
+ atomic.c \
dwarf.c \
fileline.c \
internal.h \
Index: dwarf.c
===================================================================
--- dwarf.c (revision 204992)
+++ dwarf.c (working copy)
@@ -2643,12 +2643,7 @@ dwarf_lookup_pc (struct backtrace_state
&& pc < (entry - 1)->high)
{
if (state->threaded)
- {
- /* Use __sync_bool_compare_and_swap to do a
- load-acquire. */
- while (!__sync_bool_compare_and_swap (&u->lines, lines, lines))
- lines = u->lines;
- }
+ lines = (struct line *) backtrace_atomic_load_pointer (&u->lines);
if (lines != (struct line *) (uintptr_t) -1)
break;
@@ -2659,13 +2654,8 @@ dwarf_lookup_pc (struct backtrace_state
lines = u->lines;
}
- /* Do a load-acquire of u->lines. */
if (state->threaded)
- {
- /* Use __sync_bool_compare_and_swap to do an atomic load. */
- while (!__sync_bool_compare_and_swap (&u->lines, lines, lines))
- lines = u->lines;
- }
+ lines = backtrace_atomic_load_pointer (&u->lines);
new_data = 0;
if (lines == NULL)
@@ -2713,12 +2703,11 @@ dwarf_lookup_pc (struct backtrace_state
}
else
{
- __sync_bool_compare_and_swap (&u->lines_count, 0, count);
- __sync_bool_compare_and_swap (&u->function_addrs, NULL,
- function_addrs);
- __sync_bool_compare_and_swap (&u->function_addrs_count, 0,
- function_addrs_count);
- __sync_bool_compare_and_swap (&u->lines, NULL, lines);
+ backtrace_atomic_store_size_t (&u->lines_count, count);
+ backtrace_atomic_store_pointer (&u->function_addrs, function_addrs);
+ backtrace_atomic_store_size_t (&u->function_addrs_count,
+ function_addrs_count);
+ backtrace_atomic_store_pointer (&u->lines, lines);
}
}
@@ -2849,11 +2838,7 @@ dwarf_fileline (struct backtrace_state *
pp = (struct dwarf_data **) (void *) &state->fileline_data;
while (1)
{
- ddata = *pp;
- /* Atomic load. */
- while (!__sync_bool_compare_and_swap (pp, ddata, ddata))
- ddata = *pp;
-
+ ddata = backtrace_atomic_load_pointer (pp);
if (ddata == NULL)
break;
@@ -2985,10 +2970,7 @@ backtrace_dwarf_add (struct backtrace_st
{
struct dwarf_data *p;
- /* Atomic load. */
- p = *pp;
- while (!__sync_bool_compare_and_swap (pp, p, p))
- p = *pp;
+ p = backtrace_atomic_load_pointer (pp);
if (p == NULL)
break;
Index: elf.c
===================================================================
--- elf.c (revision 204992)
+++ elf.c (working copy)
@@ -442,10 +442,7 @@ elf_add_syminfo_data (struct backtrace_s
{
struct elf_syminfo_data *p;
- /* Atomic load. */
- p = *pp;
- while (!__sync_bool_compare_and_swap (pp, p, p))
- p = *pp;
+ p = backtrace_atomic_load_pointer (pp);
if (p == NULL)
break;
@@ -490,11 +487,7 @@ elf_syminfo (struct backtrace_state *sta
pp = (struct elf_syminfo_data **) (void *) &state->syminfo_data;
while (1)
{
- edata = *pp;
- /* Atomic load. */
- while (!__sync_bool_compare_and_swap (pp, edata, edata))
- edata = *pp;
-
+ edata = backtrace_atomic_load_pointer (pp);
if (edata == NULL)
break;
@@ -902,7 +895,6 @@ backtrace_initialize (struct backtrace_s
{
int found_sym;
int found_dwarf;
- syminfo elf_syminfo_fn;
fileline elf_fileline_fn;
struct phdr_data pd;
@@ -919,18 +911,19 @@ backtrace_initialize (struct backtrace_s
dl_iterate_phdr (phdr_callback, (void *) &pd);
- elf_syminfo_fn = found_sym ? elf_syminfo : elf_nosyms;
if (!state->threaded)
{
- if (state->syminfo_fn == NULL || found_sym)
- state->syminfo_fn = elf_syminfo_fn;
+ if (found_sym)
+ state->syminfo_fn = elf_syminfo;
+ else if (state->syminfo_fn == NULL)
+ state->syminfo_fn = elf_nosyms;
}
else
{
- __sync_bool_compare_and_swap (&state->syminfo_fn, NULL, elf_syminfo_fn);
if (found_sym)
- __sync_bool_compare_and_swap (&state->syminfo_fn, elf_nosyms,
- elf_syminfo_fn);
+ backtrace_atomic_store_pointer (&state->syminfo_fn, elf_syminfo);
+ else
+ __sync_bool_compare_and_swap (&state->syminfo_fn, NULL, elf_nosyms);
}
if (!state->threaded)
@@ -942,11 +935,7 @@ backtrace_initialize (struct backtrace_s
{
fileline current_fn;
- /* Atomic load. */
- current_fn = state->fileline_fn;
- while (!__sync_bool_compare_and_swap (&state->fileline_fn, current_fn,
- current_fn))
- current_fn = state->fileline_fn;
+ current_fn = backtrace_atomic_load_pointer (&state->fileline_fn);
if (current_fn == NULL || current_fn == elf_nodebug)
*fileline_fn = elf_fileline_fn;
}
