diff mbox

Use libbacktrace as libsanitizer's symbolizer

Message ID CAKOQZ8xJa2_ZUVoLnUeLnJAchi3sGiA767GRcW9ZgAaowoUDjA@mail.gmail.com
State New
Headers show

Commit Message

Ian Lance Taylor Nov. 19, 2013, 1:11 a.m. UTC
On Mon, Nov 18, 2013 at 12:14 PM, Jakub Jelinek <jakub@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  <iant@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.

Comments

Alan Modra Nov. 19, 2013, 7:02 a.m. UTC | #1
On Tue, Nov 19, 2013 at 06:17:41AM +0100, Hans-Peter Nilsson wrote:
> In file included from /tmp/x/gcc/libbacktrace/atomic.c:37:
> /tmp/x/gcc/libbacktrace/internal.h:182: error: expected declaration specifiers or '...' before 'off_t'
> make[3]: *** [atomic.lo] Error 1
> 
> brgds, H-P
> PS. Host is Fedora 12, x86_64.

Likewise on powerpc-linux.  Fixed here by #include <sys/types.h> in
atomic.c.
diff mbox

Patch

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;
     }