Use libbacktrace as libsanitizer's symbolizer

Hi!

This patch adds libbacktrace symbolizer to libsanitizer, with which we
can avoid spawning and running external an external program (especially when
it is not even provided by gcc itself).

I've kept the possibility to override the symbolizer by magic symbols
(InternalSymbolizer), and as I have no idea how the llvm buildsystem etc.
works and what is the possibility there to add libbacktrace, this just
requires users to compile with SANITIZE_LIBBACKTRACE defined to signal
that backtrace-supported.h and backtrace.h is available and the
sanitizer_symbolizer_posix_libcdep.cc source then decides based on
backtrace-supported.h etc. whether it is usable.

make check RUNTESTFLAGS='asan.exp ubsan.exp' passes with this.

Some pending issues on the libbacktrace side:
1) right now libbacktrace performs dl_iterate_phdr only the first time
   backtrace_pcinfo or backtrace_syminfo is called, if there are some
   dlopens/dlcloses in between that and another querying of the symbolizer,
   it won't notice that.  Perhaps it can be done only when we don't
   find a symbol and/or have some function that tries to dl_iterare_phdr
   again, look at cached st_ino/st_mtime or similar, and for threaded
   version likely just keep old records, just add a flag to them that they
   should be ignored (or say atomically decrease symbol count to zero
   and something similar for debug info).
2) for tsan querying of data symbols, apparently the classes want to see
   not just the symbol name and start value, but also size.  libbacktrace
   has all this info available, just doesn't pass it down to the callback.
   I wonder if we'd need to create yet another libbacktrace entrypoint, or
   if it would be acceptable to do source code incompatible, ABI (at least
   on all sane targets) compatible version of just adding another
   uintptr_t symsize argument to backtrace_syminfo_callback.
3) I wonder if libbacktrace couldn't be updated to use __atomic_* builtins,
   then it could avoid the ugliness to emulate atomic loads and stores.

As for sanitizer, the reason I haven't implemented SendCommand method for
the libbacktrace symbolizer is that the library doesn't provide the info
as text, but as individual values passed to the callback, so printing
that to text and then parsing the text would be very ugly.  libbacktrace
doesn't even need the module names and module offsets, so supposedly
we would need that only if libbacktrace failed to get accurate inline/call
or symbol info.  While the classes have both symbol name/file/line etc.
and module name/offset fields, apparently the latter are unused if the
former is filled in.

2013-11-18  Jakub Jelinek  <jakub@redhat.com>

	PR sanitizer/59136
	* configure.ac: Don't add target-libbacktrace to noconfigdirs
	just because go hasn't been enabled if target-libsanitizer isn't in
	noconfigdirs.
	* Makefile.def: Add configure-target-libsanitizer dependency on
	configure-target-libbacktrace and all-target-libsanitizer dependency
	on configure-target-libsanitizer.
	* configure: Regenerated.
libsanitizer/
	* sanitizer_common/Makefile.am (AM_CPPFLAGS): If
	SANITIZER_LIBBACKTRACE, append -I for libbacktrace headers and
	-DSANITIZER_LIBBACKTRACE.
	* sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc: Add
	libbacktrace symbolizer.
	* tsan/Makefile.am (libtsan_la_LIBADD): Add libbacktrace.la if
	SANITIZER_LIBBACKTRACE.
	* asan/Makefile.am (libasan_la_LIBADD): Likewise.
	* ubsan/Makefile.am (libubsan_la_LIBADD): Likewise.
	* configure.ac (SANITIZER_LIBBACKTRACE): New AM_CONDITIONAL.
	* sanitizer_common/Makefile.in: Regenerated.
	* tsan/Makefile.in: Regenrated.
	* asan/Makefile.in: Regenerated.
	* ubsan/Makefile.in: Regenerated.
	* configure: Regenerated.
gcc/testsuite/
	* c-c++-common/asan/strip-path-prefix-1.c: Allow also the
	filename:line instead of (modulename+offset) form with stripped
	initial / from the filename.


	Jakub

(resending in plain text)

On Mon, Nov 18, 2013 at 7:49 PM, Alexey Samsonov <samsonov@google.com> wrote:
 Hi Jakub,

 Unfortunately, recently there were a few enhancements to
 sanitizer_symbolizer_posix_libcdep.cc (and friends),
 in LLVM trunk, and now it looks different from gcc version (apparently, the
 changes were committed
 after the merge to gcc happened, I should have pointed this out earlier,
 sorry).

 Kostya (or Jakub), is it possible to somehow pick up the changes? Otherwise
 this patch can't go in ASan runtime
 in gcc - the code will significantly diverge.

> On Mon, Nov 18, 2013 at 5:39 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>>
>> Hi!
>>
>> This patch adds libbacktrace symbolizer to libsanitizer, with which we
>> can avoid spawning and running external an external program (especially
>> when
>> it is not even provided by gcc itself).
>>
>> I've kept the possibility to override the symbolizer by magic symbols
>> (InternalSymbolizer), and as I have no idea how the llvm buildsystem etc.
>> works and what is the possibility there to add libbacktrace, this just
>> requires users to compile with SANITIZE_LIBBACKTRACE defined to signal
>> that backtrace-supported.h and backtrace.h is available and the
>> sanitizer_symbolizer_posix_libcdep.cc source then decides based on
>> backtrace-supported.h etc. whether it is usable.
>>
>> make check RUNTESTFLAGS='asan.exp ubsan.exp' passes with this.
>>
>> Some pending issues on the libbacktrace side:
>> 1) right now libbacktrace performs dl_iterate_phdr only the first time
>>    backtrace_pcinfo or backtrace_syminfo is called, if there are some
>>    dlopens/dlcloses in between that and another querying of the
>> symbolizer,
>>    it won't notice that.  Perhaps it can be done only when we don't
>>    find a symbol and/or have some function that tries to dl_iterare_phdr
>>    again, look at cached st_ino/st_mtime or similar, and for threaded
>>    version likely just keep old records, just add a flag to them that they
>>    should be ignored (or say atomically decrease symbol count to zero
>>    and something similar for debug info).
>> 2) for tsan querying of data symbols, apparently the classes want to see
>>    not just the symbol name and start value, but also size.  libbacktrace
>>    has all this info available, just doesn't pass it down to the callback.
>>    I wonder if we'd need to create yet another libbacktrace entrypoint, or
>>    if it would be acceptable to do source code incompatible, ABI (at least
>>    on all sane targets) compatible version of just adding another
>>    uintptr_t symsize argument to backtrace_syminfo_callback.
>> 3) I wonder if libbacktrace couldn't be updated to use __atomic_*
>> builtins,
>>    then it could avoid the ugliness to emulate atomic loads and stores.
>>
>> As for sanitizer, the reason I haven't implemented SendCommand method for
>> the libbacktrace symbolizer is that the library doesn't provide the info
>> as text, but as individual values passed to the callback, so printing
>> that to text and then parsing the text would be very ugly.  libbacktrace
>> doesn't even need the module names and module offsets,
>
>
If libbacktrace does its own call of dl_iterate_phdr, then it doesn't need
many
pieces of existing Symbolizer (or POSIXSymbolizer, in new version of source
code)
complexity - like FindModuleForAddress() function. Consider creating a
separate
wrapper class for libbacktrace functionality and returning it from
Symbolizer::PlatformInit
factory (in the new version of code) instead of POSIXSymbolizer.
>
>>
>> so supposedly
>> we would need that only if libbacktrace failed to get accurate inline/call
>> or symbol info.  While the classes have both symbol name/file/line etc.
>> and module name/offset fields, apparently the latter are unused if the
>> former is filled in.
>>
>> 2013-11-18  Jakub Jelinek  <jakub@redhat.com>
>>
>>         PR sanitizer/59136
>>         * configure.ac: Don't add target-libbacktrace to noconfigdirs
>>         just because go hasn't been enabled if target-libsanitizer isn't
>> in
>>         noconfigdirs.
>>         * Makefile.def: Add configure-target-libsanitizer dependency on
>>         configure-target-libbacktrace and all-target-libsanitizer
>> dependency
>>         on configure-target-libsanitizer.
>>         * configure: Regenerated.
>> libsanitizer/
>>         * sanitizer_common/Makefile.am (AM_CPPFLAGS): If
>>         SANITIZER_LIBBACKTRACE, append -I for libbacktrace headers and
>>         -DSANITIZER_LIBBACKTRACE.
>>         * sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc: Add
>>         libbacktrace symbolizer.
>>         * tsan/Makefile.am (libtsan_la_LIBADD): Add libbacktrace.la if
>>         SANITIZER_LIBBACKTRACE.
>>         * asan/Makefile.am (libasan_la_LIBADD): Likewise.
>>         * ubsan/Makefile.am (libubsan_la_LIBADD): Likewise.
>>         * configure.ac (SANITIZER_LIBBACKTRACE): New AM_CONDITIONAL.
>>         * sanitizer_common/Makefile.in: Regenerated.
>>         * tsan/Makefile.in: Regenrated.
>>         * asan/Makefile.in: Regenerated.
>>         * ubsan/Makefile.in: Regenerated.
>>         * configure: Regenerated.
>> gcc/testsuite/
>>         * c-c++-common/asan/strip-path-prefix-1.c: Allow also the
>>         filename:line instead of (modulename+offset) form with stripped
>>         initial / from the filename.
>>
>> --- libsanitizer/sanitizer_common/Makefile.am.jj        2013-11-18
>> 09:59:04.012244260 +0100
>> +++ libsanitizer/sanitizer_common/Makefile.am   2013-11-18
>> 11:04:54.241360825 +0100
>> @@ -1,5 +1,8 @@
>> -AM_CPPFLAGS = -I $(top_srcdir)/include
>> -
>> +AM_CPPFLAGS = -I $(top_srcdir)/include
>> +if SANITIZER_LIBBACKTRACE
>> +AM_CPPFLAGS += -I $(top_srcdir)/../libbacktrace -I
>> $(top_builddir)/../libbacktrace -DSANITIZER_LIBBACKTRACE
>> +endif
>> +
>>  # May be used by toolexeclibdir.
>>  gcc_version := $(shell cat $(top_srcdir)/../gcc/BASE-VER)
>>
>> --- libsanitizer/sanitizer_common/Makefile.in.jj        2013-11-18
>> 09:59:04.012244260 +0100
>> +++ libsanitizer/sanitizer_common/Makefile.in   2013-11-18
>> 12:55:52.709310820 +0100
>> @@ -35,6 +35,7 @@ POST_UNINSTALL = :
>>  build_triplet = @build@
>>  host_triplet = @host@
>>  target_triplet = @target@
>> +@SANITIZER_LIBBACKTRACE_TRUE@am__append_1 = -I
>> $(top_srcdir)/../libbacktrace -I $(top_builddir)/../libbacktrace
>> -DSANITIZER_LIBBACKTRACE
>>  subdir = sanitizer_common
>>  DIST_COMMON = $(srcdir)/Makefile.in $(srcdir)/Makefile.am
>>  ACLOCAL_M4 = $(top_srcdir)/aclocal.m4
>> @@ -217,7 +218,7 @@ toolexeclibdir = @toolexeclibdir@
>>  top_build_prefix = @top_build_prefix@
>>  top_builddir = @top_builddir@
>>  top_srcdir = @top_srcdir@
>> -AM_CPPFLAGS = -I $(top_srcdir)/include
>> +AM_CPPFLAGS = -I $(top_srcdir)/include $(am__append_1)
>>
>>  # May be used by toolexeclibdir.
>>  gcc_version := $(shell cat $(top_srcdir)/../gcc/BASE-VER)
>> --- libsanitizer/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc.jj
>> 2013-11-18 09:58:51.938333272 +0100
>> +++ libsanitizer/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc
>> 2013-11-18 13:17:40.105620024 +0100
>> @@ -25,6 +25,16 @@
>>  #include <sys/wait.h>
>>  #include <unistd.h>
>>
>> +#ifdef SANITIZER_LIBBACKTRACE
>> +#include "backtrace-supported.h"
>> +#if SANITIZER_LINUX && BACKTRACE_SUPPORTED \
>> +    && !BACKTRACE_USES_MALLOC && BACKTRACE_SUPPORTS_THREADS
>> +#include "backtrace.h"
>> +#else
>> +#undef SANITIZER_LIBBACKTRACE
>> +#endif
>> +#endif
>> +
>>  // C++ demangling function, as required by Itanium C++ ABI. This is weak,
>>  // because we do not require a C++ ABI library to be linked to a program
>>  // using sanitizers; if it's not present, we'll just use the mangled
>> name.
>> @@ -365,6 +375,117 @@ class InternalSymbolizer {
>>
>>  #endif  // SANITIZER_SUPPORTS_WEAK_HOOKS
>>
>> +#if SANITIZER_LIBBACKTRACE
>> +namespace {
>> +
>> +struct SymbolizeCodeData {
>> +  AddressInfo *frames;
>> +  uptr n_frames;
>> +  uptr max_frames;
>> +  const char *module_name;
>> +  uptr module_offset;
>> +};
>> +
>> +extern "C" {
>> +
>> +static int SymbolizeCodePCInfoCallback(void *vdata, uintptr_t addr,
>> +                                      const char *filename, int lineno,
>> +                                      const char *function) {
>> +  SymbolizeCodeData *cdata = (SymbolizeCodeData *)vdata;
>> +  if (function) {
>> +    AddressInfo *info = &cdata->frames[cdata->n_frames++];
>> +    info->Clear();
>> +    info->FillAddressAndModuleInfo(addr, cdata->module_name,
>> cdata->module_offset);
>> +    info->function = internal_strdup(function);
>> +    if (filename)
>> +      info->file = internal_strdup(filename);
>> +    info->line = lineno;
>> +  }
>> +  return 0;
>> +}
>> +
>> +static void SymbolizeCodeCallback(void *vdata, uintptr_t addr,
>> +                                 const char *symname, uintptr_t) {
>> +  SymbolizeCodeData *cdata = (SymbolizeCodeData *)vdata;
>> +  if (symname) {
>> +    AddressInfo *info = &cdata->frames[0];
>> +    info->Clear();
>> +    info->FillAddressAndModuleInfo(addr, cdata->module_name,
>> cdata->module_offset);
>> +    info->function = internal_strdup(symname);
>> +    cdata->n_frames = 1;
>> +  }
>> +}
>> +
>> +static void SymbolizeDataCallback(void *vdata, uintptr_t,
>> +                                 const char *symname, uintptr_t symval) {
>> +  DataInfo *info = (DataInfo *)vdata;
>> +  if (symname && symval) {
>> +    info->name = internal_strdup(symname);
>> +    info->start = symval;
>> +  }
>> +}
>> +
>> +static void ErrorCallback(void *, const char *, int) {
>> +}
>> +
>> +}
>> +
>> +}
>> +
>> +class LibbacktraceSymbolizer {
>> + public:
>> +  static LibbacktraceSymbolizer *get() {
>> +    backtrace_state *state
>> +      = backtrace_create_state("/proc/self/exe", 1, ErrorCallback, NULL);
>> +    if (state == NULL)
>> +      return NULL;
>> +    void *mem =
>> symbolizer_allocator.Allocate(sizeof(LibbacktraceSymbolizer));
>> +    return new(mem) LibbacktraceSymbolizer(state);
>> +  }
>> +
>> +  uptr SymbolizeCode(uptr addr, AddressInfo *frames, uptr max_frames,
>> +                    const char *module_name, uptr module_offset) {
>> +    SymbolizeCodeData data;
>> +    data.frames = frames;
>> +    data.n_frames = 0;
>> +    data.max_frames = max_frames;
>> +    data.module_name = module_name;
>> +    data.module_offset = module_offset;
>> +    backtrace_pcinfo(state_, addr, SymbolizeCodePCInfoCallback,
>> ErrorCallback,
>> +                    &data);
>> +    if (data.n_frames)
>> +      return data.n_frames;
>> +    backtrace_syminfo(state_, addr, SymbolizeCodeCallback, ErrorCallback,
>> &data);
>> +    return data.n_frames;
>> +  }
>> +
>> +  void SymbolizeData(DataInfo *info) {
>> +    backtrace_syminfo(state_, info->address, SymbolizeDataCallback,
>> +                     ErrorCallback, info);
>> +  }
>> +
>> + private:
>> +  LibbacktraceSymbolizer(backtrace_state *state) : state_(state) { }
>> +
>> +  backtrace_state *state_;     // Leaked.
>> +};
>> +#else
>> +class LibbacktraceSymbolizer {
>> + public:
>> +  static LibbacktraceSymbolizer *get() {
>> +    return NULL;
>> +  }
>> +
>> +  uptr SymbolizeCode(uptr addr, AddressInfo *frames, uptr max_frames,
>> +                    const char *module_name, uptr module_offset) {
>> +    return 0;
>> +  }
>> +
>> +  void SymbolizeData(DataInfo *info) {
>> +  }
>> +};
>> +#endif
>> +
>>  class Symbolizer : public SymbolizerInterface {
>>    // This class has no constructor, as global constructors are forbidden
>> in
>>    // sanitizer_common. It should be linker initialized instead.
>> @@ -377,9 +498,28 @@ class Symbolizer : public SymbolizerInte
>>        return 0;
>>      const char *module_name = module->full_name();
>>      uptr module_offset = addr - module->base_address();
>> -    const char *str = SendCommand(false, module_name, module_offset);
>> +    if (!IsAvailable()) {
>> +      // External symbolizer was not initialized. Fill only data
>> +      // about module name and offset.
>> +      AddressInfo *info = &frames[0];
>> +      info->Clear();
>> +      info->FillAddressAndModuleInfo(addr, module_name, module_offset);
>> +      return 1;
>> +    }
>> +    const char *str = NULL;
>> +    if (libbacktrace_symbolizer_)
>> +      {
>> +       uptr ret
>> +         = libbacktrace_symbolizer_->SymbolizeCode(addr, frames,
>> max_frames,
>> +                                                   module_name,
>> +                                                   module_offset);
>> +       if (ret)
>> +         return ret;
>> +      }
>> +    else
>> +      str = SendCommand(false, module_name, module_offset);
>>      if (str == 0) {
>> -      // External symbolizer was not initialized or failed. Fill only
>> data
>> +      // External symbolizer failed. Fill only data
>>        // about module name and offset.
>>        AddressInfo *info = &frames[0];
>>        info->Clear();
>> @@ -439,6 +579,12 @@ class Symbolizer : public SymbolizerInte
>>      info->address = addr;
>>      info->module = internal_strdup(module_name);
>>      info->module_offset = module_offset;
>> +    if (!IsAvailable ())
>> +      return true;
>> +    if (libbacktrace_symbolizer_) {
>> +      libbacktrace_symbolizer_->SymbolizeData(info);
>> +      return true;
>> +    }
>>      const char *str = SendCommand(true, module_name, module_offset);
>>      if (str == 0)
>>        return true;
>> @@ -449,7 +595,17 @@ class Symbolizer : public SymbolizerInte
>>      return true;
>>    }
>>
>> +  void InitializeInternal() {
>> +    internal_symbolizer_ = InternalSymbolizer::get();
>> +#ifdef SANITIZER_LIBBACKTRACE
>> +    if (internal_symbolizer_ == NULL)
>> +      libbacktrace_symbolizer_ = LibbacktraceSymbolizer::get();
>> +#endif
>> +  }
>> +
>>    bool InitializeExternal(const char *path_to_symbolizer) {
>> +    if (internal_symbolizer_ || libbacktrace_symbolizer_)
>> +      return true;
>>      if (!path_to_symbolizer || path_to_symbolizer[0] == '\0') {
>>        path_to_symbolizer = FindPathToBinary("llvm-symbolizer");
>>        if (!path_to_symbolizer)
>> @@ -465,9 +621,17 @@ class Symbolizer : public SymbolizerInte
>>    }
>>
>>    bool IsAvailable() {
>> -    if (internal_symbolizer_ == 0)
>> -      internal_symbolizer_ = InternalSymbolizer::get();
>> -    return internal_symbolizer_ || external_symbolizer_;
>> +    static atomic_uint8_t initialized;
>> +    static StaticSpinMutex init_mu;
>> +    if (atomic_load(&initialized, memory_order_acquire) == 0) {
>> +      SpinMutexLock l(&init_mu);
>> +      if (atomic_load(&initialized, memory_order_relaxed) == 0) {
>> +       atomic_store(&initialized, 1, memory_order_release);
>> +      }
>> +    }
>
>

What is this piece for?

>
>>
>> +    return internal_symbolizer_ ||
>> +          libbacktrace_symbolizer_ ||
>> +          external_symbolizer_;
>>    }
>>
>>    void Flush() {
>> @@ -492,10 +656,6 @@ class Symbolizer : public SymbolizerInte
>>
>>   private:
>>    char *SendCommand(bool is_data, const char *module_name, uptr
>> module_offset) {
>> -    // First, try to use internal symbolizer.
>> -    if (!IsAvailable()) {
>> -      return 0;
>> -    }
>>      if (internal_symbolizer_) {
>>        return internal_symbolizer_->SendCommand(is_data, module_name,
>>                                                 module_offset);
>> @@ -572,6 +732,7 @@ class Symbolizer : public SymbolizerInte
>>
>>    ExternalSymbolizer *external_symbolizer_;  // Leaked.
>>    InternalSymbolizer *internal_symbolizer_;  // Leaked.
>> +  LibbacktraceSymbolizer *libbacktrace_symbolizer_;  // Leaked.
>>  };
>>
>>  static ALIGNED(64) char symbolizer_placeholder[sizeof(Symbolizer)];
>> @@ -584,6 +745,7 @@ SymbolizerInterface *getSymbolizer() {
>>      SpinMutexLock l(&init_mu);
>>      if (atomic_load(&initialized, memory_order_relaxed) == 0) {
>>        symbolizer = new(symbolizer_placeholder) Symbolizer();
>> +      symbolizer->InitializeInternal ();
>>        atomic_store(&initialized, 1, memory_order_release);
>>      }
>>    }
>> --- libsanitizer/tsan/Makefile.am.jj    2013-11-18 09:59:04.558240293
>> +0100
>> +++ libsanitizer/tsan/Makefile.am       2013-11-18 11:04:19.655537046
>> +0100
>> @@ -38,7 +38,11 @@ tsan_files = \
>>          tsan_rtl_amd64.S
>>
>>  libtsan_la_SOURCES = $(tsan_files)
>> -libtsan_la_LIBADD =
>> $(top_builddir)/sanitizer_common/libsanitizer_common.la
>> $(top_builddir)/interception/libinterception.la $(LIBSTDCXX_RAW_CXX_LDFLAGS)
>> +libtsan_la_LIBADD =
>> $(top_builddir)/sanitizer_common/libsanitizer_common.la
>> $(top_builddir)/interception/libinterception.la
>> +if SANITIZER_LIBBACKTRACE
>> +libtsan_la_LIBADD += $(top_builddir)/../libbacktrace/libbacktrace.la
>> +endif
>> +libtsan_la_LIBADD += $(LIBSTDCXX_RAW_CXX_LDFLAGS)
>>  libtsan_la_LDFLAGS = -version-info `grep -v '^\#'
>> $(srcdir)/libtool-version` -lpthread -ldl
>>
>>  # Work around what appears to be a GNU make bug handling MAKEFLAGS
>> --- libsanitizer/tsan/Makefile.in.jj    2013-11-18 09:59:04.557240301
>> +0100
>> +++ libsanitizer/tsan/Makefile.in       2013-11-18 12:55:52.779310461
>> +0100
>> @@ -35,6 +35,7 @@ POST_UNINSTALL = :
>>  build_triplet = @build@
>>  host_triplet = @host@
>>  target_triplet = @target@
>> +@SANITIZER_LIBBACKTRACE_TRUE@am__append_1 =
>> $(top_builddir)/../libbacktrace/libbacktrace.la
>>  subdir = tsan
>>  DIST_COMMON = $(srcdir)/Makefile.in $(srcdir)/Makefile.am
>>  ACLOCAL_M4 = $(top_srcdir)/aclocal.m4
>> @@ -80,7 +81,7 @@ am__DEPENDENCIES_1 =
>>  libtsan_la_DEPENDENCIES =  \
>>         $(top_builddir)/sanitizer_common/libsanitizer_common.la \
>>         $(top_builddir)/interception/libinterception.la \
>> -       $(am__DEPENDENCIES_1)
>> +       $(am__append_1) $(am__DEPENDENCIES_1)
>>  am__objects_1 = tsan_clock.lo tsan_interface_atomic.lo tsan_mutex.lo \
>>         tsan_report.lo tsan_rtl_thread.lo tsan_symbolize.lo \
>>         tsan_flags.lo tsan_interface.lo tsan_platform_linux.lo \
>> @@ -294,7 +295,10 @@ tsan_files = \
>>          tsan_rtl_amd64.S
>>
>>  libtsan_la_SOURCES = $(tsan_files)
>> -libtsan_la_LIBADD =
>> $(top_builddir)/sanitizer_common/libsanitizer_common.la
>> $(top_builddir)/interception/libinterception.la $(LIBSTDCXX_RAW_CXX_LDFLAGS)
>> +libtsan_la_LIBADD =  \
>> +       $(top_builddir)/sanitizer_common/libsanitizer_common.la \
>> +       $(top_builddir)/interception/libinterception.la \
>> +       $(am__append_1) $(LIBSTDCXX_RAW_CXX_LDFLAGS)
>>  libtsan_la_LDFLAGS = -version-info `grep -v '^\#'
>> $(srcdir)/libtool-version` -lpthread -ldl
>>
>>  # Work around what appears to be a GNU make bug handling MAKEFLAGS
>> --- libsanitizer/ubsan/Makefile.am.jj   2013-11-18 09:58:51.000000000
>> +0100
>> +++ libsanitizer/ubsan/Makefile.am      2013-11-18 12:55:36.664382032
>> +0100
>> @@ -22,6 +22,9 @@ libubsan_la_LIBADD = $(top_builddir)/san
>>  if !USING_MAC_INTERPOSE
>>  libubsan_la_LIBADD += $(top_builddir)/interception/libinterception.la
>>  endif
>> +if SANITIZER_LIBBACKTRACE
>> +libubsan_la_LIBADD += $(top_builddir)/../libbacktrace/libbacktrace.la
>> +endif
>>  libubsan_la_LIBADD += $(LIBSTDCXX_RAW_CXX_LDFLAGS)
>>  libubsan_la_LDFLAGS = -version-info `grep -v '^\#'
>> $(srcdir)/libtool-version` -lpthread -ldl
>>
>> --- libsanitizer/ubsan/Makefile.in.jj   2013-11-18 09:58:51.000000000
>> +0100
>> +++ libsanitizer/ubsan/Makefile.in      2013-11-18 12:55:52.840310148
>> +0100
>> @@ -36,6 +36,7 @@ build_triplet = @build@
>>  host_triplet = @host@
>>  target_triplet = @target@
>>  @USING_MAC_INTERPOSE_FALSE@am__append_1 =
>> $(top_builddir)/interception/libinterception.la
>> +@SANITIZER_LIBBACKTRACE_TRUE@am__append_2 =
>> $(top_builddir)/../libbacktrace/libbacktrace.la
>>  subdir = ubsan
>>  DIST_COMMON = $(srcdir)/Makefile.in $(srcdir)/Makefile.am
>>  ACLOCAL_M4 = $(top_srcdir)/aclocal.m4
>> @@ -80,7 +81,7 @@ LTLIBRARIES = $(toolexeclib_LTLIBRARIES)
>>  am__DEPENDENCIES_1 =
>>  libubsan_la_DEPENDENCIES =  \
>>         $(top_builddir)/sanitizer_common/libsanitizer_common.la \
>> -       $(am__append_1) $(am__DEPENDENCIES_1)
>> +       $(am__append_1) $(am__append_2) $(am__DEPENDENCIES_1)
>>  am__objects_1 = ubsan_diag.lo ubsan_handlers.lo ubsan_handlers_cxx.lo \
>>         ubsan_type_hash.lo ubsan_value.lo
>>  am_libubsan_la_OBJECTS = $(am__objects_1)
>> @@ -255,7 +256,7 @@ ubsan_files = \
>>  libubsan_la_SOURCES = $(ubsan_files)
>>  libubsan_la_LIBADD =  \
>>         $(top_builddir)/sanitizer_common/libsanitizer_common.la \
>> -       $(am__append_1) $(LIBSTDCXX_RAW_CXX_LDFLAGS)
>> +       $(am__append_1) $(am__append_2) $(LIBSTDCXX_RAW_CXX_LDFLAGS)
>>  libubsan_la_LDFLAGS = -version-info `grep -v '^\#'
>> $(srcdir)/libtool-version` -lpthread -ldl
>>
>>  # Work around what appears to be a GNU make bug handling MAKEFLAGS
>> --- libsanitizer/configure.ac.jj        2013-11-12 11:31:01.000000000
>> +0100
>> +++ libsanitizer/configure.ac   2013-11-18 11:08:56.650153864 +0100
>> @@ -87,6 +87,12 @@ case "$host" in
>>  esac
>>  AM_CONDITIONAL(USING_MAC_INTERPOSE, $MAC_INTERPOSE)
>>
>> +SANITIZER_LIBBACKTRACE=no
>> +if [[ -d ../libbacktrace -a -f ../libbacktrace/backtrace-supported.h -a
>> "$MAC_INTERPOSE" = false ]]; then
>> +  SANITIZER_LIBBACKTRACE=yes
>> +fi
>> +AM_CONDITIONAL(SANITIZER_LIBBACKTRACE, [test "x$SANITIZER_LIBBACKTRACE" =
>> "xyes"])
>> +
>>  AC_CONFIG_FILES([Makefile])
>>
>>  AC_CONFIG_FILES(AC_FOREACH([DIR], [interception sanitizer_common lsan
>> asan ubsan], [DIR/Makefile ]),
>> --- libsanitizer/configure.jj   2013-11-12 11:31:00.000000000 +0100
>> +++ libsanitizer/configure      2013-11-18 11:09:37.491963995 +0100
>> @@ -604,6 +604,8 @@ ac_subst_vars='am__EXEEXT_FALSE
>>  am__EXEEXT_TRUE
>>  LTLIBOBJS
>>  LIBOBJS
>> +SANITIZER_LIBBACKTRACE_FALSE
>> +SANITIZER_LIBBACKTRACE_TRUE
>>  USING_MAC_INTERPOSE_FALSE
>>  USING_MAC_INTERPOSE_TRUE
>>  TSAN_SUPPORTED_FALSE
>> @@ -11117,7 +11119,7 @@ else
>>    lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
>>    lt_status=$lt_dlunknown
>>    cat > conftest.$ac_ext <<_LT_EOF
>> -#line 11120 "configure"
>> +#line 11122 "configure"
>>  #include "confdefs.h"
>>
>>  #if HAVE_DLFCN_H
>> @@ -11223,7 +11225,7 @@ else
>>    lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
>>    lt_status=$lt_dlunknown
>>    cat > conftest.$ac_ext <<_LT_EOF
>> -#line 11226 "configure"
>> +#line 11228 "configure"
>>  #include "confdefs.h"
>>
>>  #if HAVE_DLFCN_H
>> @@ -14546,6 +14548,19 @@ else
>>  fi
>>
>>
>> +SANITIZER_LIBBACKTRACE=no
>> +if [ -d ../libbacktrace -a -f ../libbacktrace/backtrace-supported.h -a
>> "$MAC_INTERPOSE" = false ]; then
>> +  SANITIZER_LIBBACKTRACE=yes
>> +fi
>> + if test "x$SANITIZER_LIBBACKTRACE" = "xyes"; then
>> +  SANITIZER_LIBBACKTRACE_TRUE=
>> +  SANITIZER_LIBBACKTRACE_FALSE='#'
>> +else
>> +  SANITIZER_LIBBACKTRACE_TRUE='#'
>> +  SANITIZER_LIBBACKTRACE_FALSE=
>> +fi
>> +
>> +
>>  ac_config_files="$ac_config_files Makefile"
>>
>>
>> @@ -14726,6 +14741,10 @@ if test -z "${USING_MAC_INTERPOSE_TRUE}"
>>    as_fn_error "conditional \"USING_MAC_INTERPOSE\" was never defined.
>>  Usually this means the macro was only invoked conditionally." "$LINENO" 5
>>  fi
>> +if test -z "${SANITIZER_LIBBACKTRACE_TRUE}" && test -z
>> "${SANITIZER_LIBBACKTRACE_FALSE}"; then
>> +  as_fn_error "conditional \"SANITIZER_LIBBACKTRACE\" was never defined.
>> +Usually this means the macro was only invoked conditionally." "$LINENO" 5
>> +fi
>>
>>  : ${CONFIG_STATUS=./config.status}
>>  ac_write_fail=0
>> --- libsanitizer/asan/Makefile.am.jj    2013-11-18 09:59:04.216242778
>> +0100
>> +++ libsanitizer/asan/Makefile.am       2013-11-18 11:03:18.183863902
>> +0100
>> @@ -41,6 +41,9 @@ libasan_la_LIBADD = $(top_builddir)/sani
>>  else
>>  libasan_la_LIBADD =
>> $(top_builddir)/sanitizer_common/libsanitizer_common.la
>> $(top_builddir)/lsan/libsanitizer_lsan.la
>> $(top_builddir)/interception/libinterception.la
>>  endif
>> +if SANITIZER_LIBBACKTRACE
>> +libasan_la_LIBADD += $(top_builddir)/../libbacktrace/libbacktrace.la
>> +endif
>>  libasan_la_LIBADD += $(LIBSTDCXX_RAW_CXX_LDFLAGS)
>>
>>  libasan_la_LDFLAGS = -version-info `grep -v '^\#'
>> $(srcdir)/libtool-version` -lpthread -ldl
>> --- libsanitizer/asan/Makefile.in.jj    2013-11-18 09:59:04.214242792
>> +0100
>> +++ libsanitizer/asan/Makefile.in       2013-11-18 12:55:52.538311697
>> +0100
>> @@ -37,6 +37,7 @@ build_triplet = @build@
>>  host_triplet = @host@
>>  target_triplet = @target@
>>  @USING_MAC_INTERPOSE_TRUE@am__append_1 = -DMAC_INTERPOSE_FUNCTIONS
>> -DMISSING_BLOCKS_SUPPORT
>> +@SANITIZER_LIBBACKTRACE_TRUE@am__append_2 =
>> $(top_builddir)/../libbacktrace/libbacktrace.la
>>  subdir = asan
>>  DIST_COMMON = $(srcdir)/Makefile.in $(srcdir)/Makefile.am
>>  ACLOCAL_M4 = $(top_srcdir)/aclocal.m4
>> @@ -83,9 +84,11 @@ am__DEPENDENCIES_1 =
>>  @USING_MAC_INTERPOSE_FALSE@libasan_la_DEPENDENCIES =
>> $(top_builddir)/sanitizer_common/libsanitizer_common.la \
>>  @USING_MAC_INTERPOSE_FALSE@    $(top_builddir)/lsan/libsanitizer_lsan.la
>> \
>>  @USING_MAC_INTERPOSE_FALSE@
>> $(top_builddir)/interception/libinterception.la \
>> +@USING_MAC_INTERPOSE_FALSE@    $(am__append_2) \
>>  @USING_MAC_INTERPOSE_FALSE@    $(am__DEPENDENCIES_1)
>>  @USING_MAC_INTERPOSE_TRUE@libasan_la_DEPENDENCIES =
>> $(top_builddir)/sanitizer_common/libsanitizer_common.la \
>>  @USING_MAC_INTERPOSE_TRUE@     $(top_builddir)/lsan/libsanitizer_lsan.la
>> \
>> +@USING_MAC_INTERPOSE_TRUE@     $(am__append_2) \
>>  @USING_MAC_INTERPOSE_TRUE@     $(am__DEPENDENCIES_1)
>>  am__objects_1 = asan_allocator2.lo asan_dll_thunk.lo \
>>         asan_fake_stack.lo asan_globals.lo asan_interceptors.lo \
>> @@ -285,9 +288,11 @@ libasan_la_SOURCES = $(asan_files)
>>  @USING_MAC_INTERPOSE_FALSE@libasan_la_LIBADD =
>> $(top_builddir)/sanitizer_common/libsanitizer_common.la \
>>  @USING_MAC_INTERPOSE_FALSE@    $(top_builddir)/lsan/libsanitizer_lsan.la
>> \
>>  @USING_MAC_INTERPOSE_FALSE@
>> $(top_builddir)/interception/libinterception.la \
>> +@USING_MAC_INTERPOSE_FALSE@    $(am__append_2) \
>>  @USING_MAC_INTERPOSE_FALSE@    $(LIBSTDCXX_RAW_CXX_LDFLAGS)
>>  @USING_MAC_INTERPOSE_TRUE@libasan_la_LIBADD =
>> $(top_builddir)/sanitizer_common/libsanitizer_common.la \
>>  @USING_MAC_INTERPOSE_TRUE@     $(top_builddir)/lsan/libsanitizer_lsan.la
>> \
>> +@USING_MAC_INTERPOSE_TRUE@     $(am__append_2) \
>>  @USING_MAC_INTERPOSE_TRUE@     $(LIBSTDCXX_RAW_CXX_LDFLAGS)
>>  libasan_la_LDFLAGS = -version-info `grep -v '^\#'
>> $(srcdir)/libtool-version` -lpthread -ldl
>>
>> --- gcc/testsuite/c-c++-common/asan/strip-path-prefix-1.c.jj    2012-12-13
>> 00:02:50.000000000 +0100
>> +++ gcc/testsuite/c-c++-common/asan/strip-path-prefix-1.c       2013-11-18
>> 13:11:16.538581367 +0100
>> @@ -12,4 +12,4 @@ int main() {
>>  }
>>
>>  /* { dg-output "heap-use-after-free.*(\n|\r\n|\r)" } */
>> -/* { dg-output "    #0 0x\[0-9a-f\]+ \[(\]\[^/\]\[^\n\r]*(\n|\r\n|\r)" }
>> */
>> +/* { dg-output "    #0 0x\[0-9a-f\]+ \[(\]?\[^/\]\[^\n\r]*(\n|\r\n|\r)" }
>> */
>> --- configure.ac.jj6    2013-11-12 11:31:05.000000000 +0100
>> +++ configure.ac        2013-11-18 11:12:45.258014225 +0100
>> @@ -1966,7 +1966,17 @@ if test -d ${srcdir}/gcc; then
>>      for dir in $disabled_target_libs; do
>>        case " $enabled_target_libs " in
>>        *" ${dir} "*) ;;
>> -      *) noconfigdirs="$noconfigdirs $dir" ;;
>> +      *) if [[ "${dir}" = target-libbacktrace ]]; then
>> +          # target-libbacktrace is needed by target-libsanitizer.
>> +          # Thus if target-libsanitizer is supported, don't disable
>> +          # target-libbacktrace.
>> +          case " $noconfigdirs " in
>> +            *" target-libsanitizer "*)
>> +              noconfigdirs="$noconfigdirs $dir";;
>> +          esac
>> +        else
>> +          noconfigdirs="$noconfigdirs $dir"
>> +        fi ;;
>>        esac
>>      done
>>    fi
>> --- configure.jj6       2013-11-12 11:31:06.000000000 +0100
>> +++ configure   2013-11-18 11:12:58.028955632 +0100
>> @@ -6535,7 +6535,17 @@ if test -d ${srcdir}/gcc; then
>>      for dir in $disabled_target_libs; do
>>        case " $enabled_target_libs " in
>>        *" ${dir} "*) ;;
>> -      *) noconfigdirs="$noconfigdirs $dir" ;;
>> +      *) if [ "${dir}" = target-libbacktrace ]; then
>> +          # target-libbacktrace is needed by target-libsanitizer.
>> +          # Thus if target-libsanitizer is supported, don't disable
>> +          # target-libbacktrace.
>> +          case " $noconfigdirs " in
>> +            *" target-libsanitizer "*)
>> +              noconfigdirs="$noconfigdirs $dir";;
>> +          esac
>> +        else
>> +          noconfigdirs="$noconfigdirs $dir"
>> +        fi ;;
>>        esac
>>      done
>>    fi
>> --- Makefile.def.jj6    2013-11-12 11:31:45.000000000 +0100
>> +++ Makefile.def        2013-11-18 10:57:12.870803393 +0100
>> @@ -510,6 +510,8 @@ dependencies = { module=configure-target
>>  dependencies = { module=all-target-libgo; on=all-target-libbacktrace; };
>>  dependencies = { module=all-target-libgo; on=all-target-libffi; };
>>  dependencies = { module=all-target-libgo; on=all-target-libatomic; };
>> +dependencies = { module=configure-target-libsanitizer;
>> on=configure-target-libbacktrace; };
>> +dependencies = { module=all-target-libsanitizer;
>> on=all-target-libbacktrace; };
>>  dependencies = { module=configure-target-libjava;
>> on=configure-target-zlib; };
>>  dependencies = { module=configure-target-libjava;
>> on=configure-target-boehm-gc; };
>>  dependencies = { module=configure-target-libjava;
>> on=configure-target-libffi; };
>> --- Makefile.in.jj6     2013-11-12 11:30:59.000000000 +0100
>> +++ Makefile.in 2013-11-18 10:57:24.000000000 +0100
>> @@ -47170,6 +47170,8 @@ configure-target-libgo: maybe-all-target
>>  all-target-libgo: maybe-all-target-libbacktrace
>>  all-target-libgo: maybe-all-target-libffi
>>  all-target-libgo: maybe-all-target-libatomic
>> +configure-target-libsanitizer: maybe-configure-target-libbacktrace
>> +all-target-libsanitizer: maybe-all-target-libbacktrace
>>  configure-target-libjava: maybe-configure-target-zlib
>>  configure-target-libjava: maybe-configure-target-boehm-gc
>>  configure-target-libjava: maybe-configure-target-libffi
>>
>>         Jakub
>
>
>
>
> --
> Alexey Samsonov, MSK

On Mon, Nov 18, 2013 at 07:49:03PM +0400, Alexey Samsonov wrote:
> > As for sanitizer, the reason I haven't implemented SendCommand method for
> > the libbacktrace symbolizer is that the library doesn't provide the info
> > as text, but as individual values passed to the callback, so printing
> > that to text and then parsing the text would be very ugly.  libbacktrace
> > doesn't even need the module names and module offsets,
> 
> 
> If libbacktrace does its own call of dl_iterate_phdr, then it doesn't need
> many

It does.

> pieces of existing Symbolizer (or POSIXSymbolizer, in new version of source
> code)
> complexity - like FindModuleForAddress() function. Consider creating a
> separate

Well, as fallback it still needs that.  libbacktrace APIs won't tell you:
"ok, see, I don't know exactly what this symbol is, but it is in DSO
libfoobar.so plus offset 0x123456".  It tells you either
function name and file/line (for backtrace_pcinfo callback, possibly several
times to iterate through inline frames) or symbol name and symbol value
(hopefully in the future also symbol size) for backtrace_syminfo callback.
If the address isn't found in debug info resp. symbol tables, you just get
NULL function name of symbol value, NULL filename and 0 line number or
symbol value.  Initially I had the code as a different toplevel symbolizer,
but for the case where libbacktrace isn't there, or where it isn't usable
(due to host limitations e.g. not thread safe, or using malloc, etc.)
there needs to be a fallback to the POSIXSymbolizer.

	Jakub

On Mon, Nov 18, 2013 at 5:39 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>
> I've kept the possibility to override the symbolizer by magic symbols
> (InternalSymbolizer), and as I have no idea how the llvm buildsystem etc.
> works and what is the possibility there to add libbacktrace, this just
> requires users to compile with SANITIZE_LIBBACKTRACE defined to signal
> that backtrace-supported.h and backtrace.h is available and the
> sanitizer_symbolizer_posix_libcdep.cc source then decides based on
> backtrace-supported.h etc. whether it is usable.

Just FYI for those who don't know, libbacktrace is under a BSD license
so there should be no licensing objection to using it in LLVM.


> 1) right now libbacktrace performs dl_iterate_phdr only the first time
>    backtrace_pcinfo or backtrace_syminfo is called, if there are some
>    dlopens/dlcloses in between that and another querying of the symbolizer,
>    it won't notice that.  Perhaps it can be done only when we don't
>    find a symbol and/or have some function that tries to dl_iterare_phdr
>    again, look at cached st_ino/st_mtime or similar, and for threaded
>    version likely just keep old records, just add a flag to them that they
>    should be ignored (or say atomically decrease symbol count to zero
>    and something similar for debug info).

Yes, this is a FIXME in dwarf_fileline in libbacktrace/dwarf.c.


> 2) for tsan querying of data symbols, apparently the classes want to see
>    not just the symbol name and start value, but also size.  libbacktrace
>    has all this info available, just doesn't pass it down to the callback.
>    I wonder if we'd need to create yet another libbacktrace entrypoint, or
>    if it would be acceptable to do source code incompatible, ABI (at least
>    on all sane targets) compatible version of just adding another
>    uintptr_t symsize argument to backtrace_syminfo_callback.

I think it would be fine to change the callback.  I doubt that
libbacktrace is so widely used that we need to worry about backward
compatibility at this stage.  In particular I imagine that any users
of libbacktrace are simply copying the source code, since there is no
installable package.


> 3) I wonder if libbacktrace couldn't be updated to use __atomic_* builtins,
>    then it could avoid the ugliness to emulate atomic loads and stores.

I think that would be fine.  I can't remember why I didn't use the
__atomic builtins.  Maybe they weren't quite ready at the time.

Ian

On Mon, Nov 18, 2013 at 9:09 AM, Ian Lance Taylor <iant@google.com> wrote:
> On Mon, Nov 18, 2013 at 5:39 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>
>> 3) I wonder if libbacktrace couldn't be updated to use __atomic_* builtins,
>>    then it could avoid the ugliness to emulate atomic loads and stores.
>
> I think that would be fine.  I can't remember why I didn't use the
> __atomic builtins.  Maybe they weren't quite ready at the time.

Wait, I remember the reason.  It's because we compile libbacktrace
with the host compiler so that it can be linked with GCC itself, and
it seems premature to assume that the host compiler supports the
__atomic builtins.

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.

Ian

On Mon, Nov 18, 2013 at 10:26:58AM -0800, Ian Lance Taylor wrote:
> On Mon, Nov 18, 2013 at 9:09 AM, Ian Lance Taylor <iant@google.com> wrote:
> > On Mon, Nov 18, 2013 at 5:39 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> >
> >> 3) I wonder if libbacktrace couldn't be updated to use __atomic_* builtins,
> >>    then it could avoid the ugliness to emulate atomic loads and stores.
> >
> > I think that would be fine.  I can't remember why I didn't use the
> > __atomic builtins.  Maybe they weren't quite ready at the time.
> 
> Wait, I remember the reason.  It's because we compile libbacktrace
> with the host compiler so that it can be linked with GCC itself, and
> it seems premature to assume that the host compiler supports the
> __atomic builtins.

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.

Anyway, seems that in the latest libsanitizer code it uses locking around
the symbolizer anyway, so from libsanitizer POV there won't be a need to use
the thread safe variant.

	Jakub