[compiler-rt] Symbolizer refactoring: Make LibbacktraceSymbolizer adopt the SymbolizerTool interface

[kubamracek]

Summary

Part of http://reviews.llvm.org/D7827. This converts LibbacktraceSymbolizer to adopt the new SymbolizerTool interface that all symbolizers will (eventually) use.

[samsonov]

On a related note, let me know if you want to replace AddressInfo::FillAddressAndModuleInfo with AddressInfo::FillModuleInfo, or I should do it myself.

lib/sanitizer_common/sanitizer_symbolizer_libbacktrace.cc

94

last can't be nullptr now, perhaps you can add a CHECK. Or, better, as this function is only used once, you just inline it in get_new_frame

104

I'd prefer to have

CHECK(last);
if (frames_symbolized > 0) {
    SymbolizedStack *cur = SymbolizedStack::New(addr);
    /.../
    last->next = cur;
    last = cur;
}
CHECK_EQ(addr, last->info.address);
return &last->info;

179

Shouldn't you return false if data.frames_symbolized is zero?

205

Make it a static function.

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

570

Looks like you can rewrite it now:

SymbolizeStack *res = SymbolizedStack::New(addr);
if (!FindModuleNameAndOffset(...))
  return res;
res->info.FillAddressAndModuleInfo(addr, module_name, module_offset);
<...>

[kubamracek]

Updating patch.

On a related note, let me know if you want to replace AddressInfo::FillAddressAndModuleInfo with AddressInfo::FillModuleInfo, or I should do it myself.

I'll do it in a subsequent patch.

[samsonov]

LGTM

On a related note, let me know if you want to replace AddressInfo::FillAddressAndModuleInfo with AddressInfo::FillModuleInfo, or I should do it myself.

I'll do it in a subsequent patch.

Thanks!

lib/sanitizer_common/sanitizer_symbolizer_libbacktrace.cc

101

Should this be first->info.address?

[kubamracek]

CHECK_EQ(addr, last->info.address);
Should this be first->info.address?

For llvm-symbolizer, we always have the same addr in all frames coming from one SymbolizePC(addr) request. I am assuming the same holds for libbacktrace symbolizer. If that's really the case, I'd even vote for having both checks (addr == first->info.address == last->info.address).

Btw. do we have any buildbots that is at least building with SANITIZER_LIBBACKTRACE=1?

[samsonov]

In D7971#132931, @kubabrecka wrote:

CHECK_EQ(addr, last->info.address);
Should this be first->info.address?

For llvm-symbolizer, we always have the same addr in all frames coming from one SymbolizePC(addr) request. I am assuming the same holds for libbacktrace symbolizer. If that's really the case, I'd even vote for having both checks (addr == first->info.address == last->info.address).

Makes sense, let's have both checks.

Btw. do we have any buildbots that is at least building with SANITIZER_LIBBACKTRACE=1?

No, currently it's only used in GCC. Did you build it with it this definition?

[kubamracek]

Btw. do we have any buildbots that is at least building with SANITIZER_LIBBACKTRACE=1?

No, currently it's only used in GCC. Did you build it with it this definition?

Yes, I did. But it seems to me it doesn't build out-of-the-box with dynamic runtime – it failed to link and I needed to modify CMake definitions to add -lbacktrace into the .so link phase. Are there official steps how to build and use libbacktrace symbolizer?

[kubamracek]

Landed in r231032.

lib/sanitizer_common/sanitizer_symbolizer_libbacktrace.h

 #endif
 
 #ifndef SANITIZER_CP_DEMANGLE
 # define SANITIZER_CP_DEMANGLE 0
 #endif
 
 namespace __sanitizer {
 
-class LibbacktraceSymbolizer {
+class LibbacktraceSymbolizer : public SymbolizerTool {
  public:
   static LibbacktraceSymbolizer *get(LowLevelAllocator *alloc);
 
-  SymbolizedStack *SymbolizeCode(uptr addr, const char *module_name,
-                                 uptr module_offset);
+  bool SymbolizePC(uptr addr, SymbolizedStack *stack) override;
 
-  bool SymbolizeData(uptr addr, DataInfo *info);
+  bool SymbolizeData(uptr addr, DataInfo *info) override;
 
   // May return NULL if demangling failed.
-  static char *Demangle(const char *name, bool always_alloc = false);
+  const char *Demangle(const char *name) override;
 
  private:
   explicit LibbacktraceSymbolizer(void *state) : state_(state) {}
 
   void *state_;  // Leaked.
 };
 
 }  // namespace __sanitizer
 #endif  // SANITIZER_SYMBOLIZER_LIBBACKTRACE_H

lib/sanitizer_common/sanitizer_symbolizer_libbacktrace.cc

 #  endif
 # else
 #  define SANITIZER_LIBBACKTRACE 0
 # endif
 #endif
 
 namespace __sanitizer {
 
+char *DemangleAlloc(const char *name, bool always_alloc);
+
 #if SANITIZER_LIBBACKTRACE
 
 namespace {
 
 # if SANITIZER_CP_DEMANGLE
 struct CplusV3DemangleData {
   char *buf;
   uptr size, allocated;
if (data.buf)
     InternalFree(data.buf);
   return 0;
 }
 # endif  // SANITIZER_CP_DEMANGLE
 
 struct SymbolizeCodeCallbackArg {
   SymbolizedStack *first;
   SymbolizedStack *last;
-  const char *module_name;
-  uptr module_offset;
+  uptr frames_symbolized;
 
   void append(SymbolizedStack *f) {
     if (last != nullptr) {
       last->next = f;
       last = f;
     } else {
       first = f;
       last = f;
     }
   }
+
+  AddressInfo *get_new_frame(uintptr_t addr) {
+    SymbolizedStack *cur = last;
+    if (frames_symbolized > 0) {
+      cur = SymbolizedStack::New(addr);
+      AddressInfo *info = &cur->info;
+      info->FillAddressAndModuleInfo(addr, first->info.module,
+                                     first->info.module_offset);
+      append(cur);
+    } else {
+      CHECK_EQ(addr, cur->info.address);
+    }
+    return &cur->info;
+  }
 };
 
 extern "C" {
 static int SymbolizeCodePCInfoCallback(void *vdata, uintptr_t addr,
                                        const char *filename, int lineno,
                                        const char *function) {
   SymbolizeCodeCallbackArg *cdata = (SymbolizeCodeCallbackArg *)vdata;
   if (function) {
-    SymbolizedStack *cur = SymbolizedStack::New(addr);
-    cdata->append(cur);
-    AddressInfo *info = &cur->info;
-    info->FillAddressAndModuleInfo(addr, cdata->module_name,
-                                   cdata->module_offset);
-    info->function = LibbacktraceSymbolizer::Demangle(function, true);
+    AddressInfo *info = cdata->get_new_frame(addr);
+    info->function = DemangleAlloc(function, /*always_alloc*/ true);
     if (filename)
       info->file = internal_strdup(filename);
     info->line = lineno;
+    cdata->frames_symbolized++;
   }
   return 0;
 }
 
 static void SymbolizeCodeCallback(void *vdata, uintptr_t addr,
                                   const char *symname, uintptr_t, uintptr_t) {
   SymbolizeCodeCallbackArg *cdata = (SymbolizeCodeCallbackArg *)vdata;
   if (symname) {
-    SymbolizedStack *cur = SymbolizedStack::New(addr);
-    cdata->append(cur);
-    AddressInfo *info = &cur->info;
-    info->FillAddressAndModuleInfo(addr, cdata->module_name,
-                                   cdata->module_offset);
-    info->function = LibbacktraceSymbolizer::Demangle(symname, true);
+    AddressInfo *info = cdata->get_new_frame(addr);
+    info->function = DemangleAlloc(symname, /*always_alloc*/ true);
+    cdata->frames_symbolized++;
   }
 }
 
 static void SymbolizeDataCallback(void *vdata, uintptr_t, const char *symname,
                                   uintptr_t symval, uintptr_t symsize) {
   DataInfo *info = (DataInfo *)vdata;
   if (symname && symval) {
-    info->name = LibbacktraceSymbolizer::Demangle(symname, true);
+    info->name = DemangleAlloc(symname, /*always_alloc*/ true);
     info->start = symval;
     info->size = symsize;
   }
 }
 
 static void ErrorCallback(void *, const char *, int) {}
 }  // extern "C"
 
 }  // namespace
 
 LibbacktraceSymbolizer *LibbacktraceSymbolizer::get(LowLevelAllocator *alloc) {
   // State created in backtrace_create_state is leaked.
   void *state = (void *)(backtrace_create_state("/proc/self/exe", 0,
                                                 ErrorCallback, NULL));
   if (!state)
     return 0;
   return new(*alloc) LibbacktraceSymbolizer(state);
 }
 
-SymbolizedStack *LibbacktraceSymbolizer::SymbolizeCode(uptr addr,
-                                                       const char *module_name,
-                                                       uptr module_offset) {
+bool LibbacktraceSymbolizer::SymbolizePC(uptr addr, SymbolizedStack *stack) {
   SymbolizeCodeCallbackArg data;
-  data.first = nullptr;
-  data.last = nullptr;
-  data.module_name = module_name;
-  data.module_offset = module_offset;
+  data.first = stack;
+  data.last = stack;
+  data.frames_symbolized = 0;
   backtrace_pcinfo((backtrace_state *)state_, addr, SymbolizeCodePCInfoCallback,
                    ErrorCallback, &data);
-  if (data.first)
-    return data.first;
+  if (data.frames_symbolized > 0)
+    return true;
   backtrace_syminfo((backtrace_state *)state_, addr, SymbolizeCodeCallback,
                     ErrorCallback, &data);
-  return data.first;
+  return true;
 }
 
 bool LibbacktraceSymbolizer::SymbolizeData(uptr addr, DataInfo *info) {
   backtrace_syminfo((backtrace_state *)state_, addr, SymbolizeDataCallback,
                     ErrorCallback, info);
   return true;
 }
 
 #else  // SANITIZER_LIBBACKTRACE
 
 LibbacktraceSymbolizer *LibbacktraceSymbolizer::get(LowLevelAllocator *alloc) {
   return 0;
 }
 
-SymbolizedStack *LibbacktraceSymbolizer::SymbolizeCode(uptr addr,
-                                                       const char *module_name,
-                                                       uptr module_offset) {
+bool LibbacktraceSymbolizer::SymbolizePC(uptr addr, SymbolizedStack *stack) {
   (void)state_;
-  return nullptr;
+  return false;
 }
 
 bool LibbacktraceSymbolizer::SymbolizeData(uptr addr, DataInfo *info) {
   return false;
 }
 
 #endif  // SANITIZER_LIBBACKTRACE
 
-char *LibbacktraceSymbolizer::Demangle(const char *name, bool always_alloc) {
+char *DemangleAlloc(const char *name, bool always_alloc) {
 #if SANITIZER_LIBBACKTRACE && SANITIZER_CP_DEMANGLE
   if (char *demangled = CplusV3Demangle(name))
     return demangled;
 #endif
   if (always_alloc)
     return internal_strdup(name);
   return 0;
 }
 
+const char *LibbacktraceSymbolizer::Demangle(const char *name) {
+  return DemangleAlloc(name, /*always_alloc*/ false);
+}
+
 }  // namespace __sanitizer

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

POSIXSymbolizer(SymbolizerTool *external_symbolizer,
         external_symbolizer_(external_symbolizer),
         internal_symbolizer_(internal_symbolizer),
         libbacktrace_symbolizer_(libbacktrace_symbolizer) {}
 
   SymbolizedStack *SymbolizePC(uptr addr) override {
     BlockingMutexLock l(&mu_);
     const char *module_name;
     uptr module_offset;
     if (!FindModuleNameAndOffsetForAddress(addr, &module_name, &module_offset))
       return SymbolizedStack::New(addr);
+    // Always fill data about module name and offset.
+    SymbolizedStack *res = SymbolizedStack::New(addr);
+    res->info.FillAddressAndModuleInfo(addr, module_name, module_offset);
     // First, try to use libbacktrace symbolizer (if it's available).
     if (libbacktrace_symbolizer_ != 0) {
       mu_.CheckLocked();
-      if (SymbolizedStack *res = libbacktrace_symbolizer_->SymbolizeCode(
-              addr, module_name, module_offset))
+      if (libbacktrace_symbolizer_->SymbolizePC(addr, res))
         return res;
     }
-    // Always fill data about module name and offset.
-    SymbolizedStack *res = SymbolizedStack::New(addr);
-    res->info.FillAddressAndModuleInfo(addr, module_name, module_offset);
     if (SymbolizerTool *tool = GetSymbolizerTool()) {
       SymbolizerScope sym_scope(this);
       tool->SymbolizePC(addr, res);
     }
     return res;
   }
 
   bool SymbolizeData(uptr addr, DataInfo *info) override {