[compiler-rt] Symbolizer refactoring: Merge common parts of POSIXSymbolizer and WinSymbolizer

[kubamracek]

Summary

Part of http://reviews.llvm.org/D7827. Moves most of the implementation from POSIXSymbolizer to the superclass, Symbolizer, and reuses that in WinSymbolizer. Removes Symbolizer::Disable(), because we're not using it anyway.

Note that the public methods of Symbolizer lock the mu_ mutex, so both the tool's methods *and* the platform-specific virtual methods of Symbolizer (such as FindModuleNameAndOffsetForAddress) are synchronized.

[timurrrr]

Deferring to Alexey.

Some comments inline:

lib/sanitizer_common/sanitizer_symbolizer.cc

98

Interesting: is it possible for a PC to not belong to any module, but still be symbolizable by SymbolizerTools? JITed code maybe?

lib/sanitizer_common/sanitizer_symbolizer.h

127

I encourage you to write a comment explaining that mu_ not only synchronizes calls to Symbolizer, but should actually be used to synchronize all the calls to the underlying symbolizers (e.g. dbghelp on Windows). Not sure where's the best place to put this comment.

Random thought: I wonder if it's time to create a new lib/sanitizer_common/symbolization dir?..

[kubamracek]

is it possible for a PC to not belong to any module, but still be symbolizable by SymbolizerTools

Good question. We certainly don't try to do that in the current POSIXSymbolizer (for any of the POSIX tools). Do you know if DbgHelp can do this?

I encourage you to write a comment explaining that mu_ not only synchronizes calls to Symbolizer, but should actually be used to synchronize all the calls to the underlying symbolizers (e.g. dbghelp on Windows).

I forgot to say that I still plan to get rid of POSIXSymbolizer and WinSymbolizer subclasses completely, moving the platform-specific parts elsewhere. But in the meantime, I agree the comment should be there.

[timurrrr]

lib/sanitizer_common/sanitizer_symbolizer.cc

98

[you might consider replying using the web interface, otherwise the discussion is a bit hard to follow here]

Good question. We certainly don't try to do that in the current POSIXSymbolizer
(for any of the POSIX tools). Do you know if DbgHelp can do this?

I don't know actually, neither do I know if we can use DIA SDK for that... But I can imagine us adding support for plugin-line symbolizers, e.g. for V8-generated code maybe?

Kostya, Zachary, any ideas?

[samsonov]

Looks mostly good.

lib/sanitizer_common/sanitizer_symbolizer.h

88

run clang-format over the changed lines.

96

This function is short, you can just inline it here.

117

This is also Platform-specific function, might make sense to reflect it in the name.

127

Right, just add a comment here.
Timur: I'd rather not add extra directory for that - that would make build rules more complex.

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

357

no need for virtual here.

[kubamracek]

Addressing review comments.

[samsonov]

LGTM. Please watch the bots after commit.

[kubamracek]

Landed in r231680.

lib/sanitizer_common/sanitizer_symbolizer.h

struct DataInfo {
   char *name;
   uptr start;
   uptr size;
 
   DataInfo();
   void Clear();
 };
 
+class SymbolizerTool;
+
 class Symbolizer {
  public:
   /// Initialize and return platform-specific implementation of symbolizer
   /// (if it wasn't already initialized).
   static Symbolizer *GetOrInit();
   // Returns a list of symbolized frames for a given address (containing
   // all inlined functions, if necessary).
-  virtual SymbolizedStack *SymbolizePC(uptr address) {
-    return SymbolizedStack::New(address);
-  }
-  virtual bool SymbolizeData(uptr address, DataInfo *info) {
-    return false;
-  }
-  virtual bool GetModuleNameAndOffsetForPC(uptr pc, const char **module_name,
-                                           uptr *module_address) {
-    return false;
-  }
+  SymbolizedStack *SymbolizePC(uptr address);
+  bool SymbolizeData(uptr address, DataInfo *info);
+  bool GetModuleNameAndOffsetForPC(uptr pc, const char **module_name,
+                                   uptr *module_address);
   const char *GetModuleNameForPc(uptr pc) {
     const char *module_name = 0;
     uptr unused;
     if (GetModuleNameAndOffsetForPC(pc, &module_name, &unused))
       return module_name;
     return nullptr;
   }
-  virtual bool CanReturnFileLineInfo() {
-    return false;
-  }
+  bool CanReturnFileLineInfo() { return !tools_.empty(); }
   // Release internal caches (if any).
-  virtual void Flush() {}
+  void Flush();
   // Attempts to demangle the provided C++ mangled name.
-  virtual const char *Demangle(const char *name) {
-    return name;
-  }
-  virtual void PrepareForSandboxing() {}
+  const char *Demangle(const char *name);
+  void PrepareForSandboxing();
 
   // Allow user to install hooks that would be called before/after Symbolizer
   // does the actual file/line info fetching. Specific sanitizers may need this
   // to distinguish system library calls made in user code from calls made
   // during in-process symbolization.
   typedef void (*StartSymbolizationHook)();
   typedef void (*EndSymbolizationHook)();
   // May be called at most once.
   void AddHooks(StartSymbolizationHook start_hook,
                 EndSymbolizationHook end_hook);
 
  private:
   /// Platform-specific function for creating a Symbolizer object.
   static Symbolizer *PlatformInit();
-  /// Initialize the symbolizer in a disabled state.  Not thread safe.
-  static Symbolizer *Disable();
+
+  virtual bool PlatformFindModuleNameAndOffsetForAddress(
+      uptr address, const char **module_name, uptr *module_offset) {
+    UNIMPLEMENTED();
+  }
+  // Platform-specific default demangler, must not return nullptr.
+  virtual const char *PlatformDemangle(const char *name) { UNIMPLEMENTED(); }
+  virtual void PlatformPrepareForSandboxing() { UNIMPLEMENTED(); }
 
   static Symbolizer *symbolizer_;
   static StaticSpinMutex init_mu_;
 
+  // Mutex locked from public methods of |Symbolizer|, so that the internals
+  // (including individual symbolizer tools and platform-specific methods) are
+  // always synchronized.
+  BlockingMutex mu_;
+
+  typedef IntrusiveList<SymbolizerTool>::Iterator Iterator;
+  IntrusiveList<SymbolizerTool> tools_;
+
  protected:
-  Symbolizer();
+  explicit Symbolizer(IntrusiveList<SymbolizerTool> tools);
 
   static LowLevelAllocator symbolizer_allocator_;
 
   StartSymbolizationHook start_hook_;
   EndSymbolizationHook end_hook_;
   class SymbolizerScope {
    public:
     explicit SymbolizerScope(const Symbolizer *sym);

lib/sanitizer_common/sanitizer_symbolizer.cc

 // run-time libraries.
 //===----------------------------------------------------------------------===//
 
 #include "sanitizer_allocator_internal.h"
 #include "sanitizer_platform.h"
 #include "sanitizer_internal_defs.h"
 #include "sanitizer_libc.h"
 #include "sanitizer_placement_new.h"
-#include "sanitizer_symbolizer.h"
+#include "sanitizer_symbolizer_internal.h"
 
 namespace __sanitizer {
 
 AddressInfo::AddressInfo() {
   internal_memset(this, 0, sizeof(AddressInfo));
   function_offset = kUnknown;
 }
 
void DataInfo::Clear() {
   InternalFree(name);
   internal_memset(this, 0, sizeof(DataInfo));
 }
 
 Symbolizer *Symbolizer::symbolizer_;
 StaticSpinMutex Symbolizer::init_mu_;
 LowLevelAllocator Symbolizer::symbolizer_allocator_;
 
-Symbolizer *Symbolizer::Disable() {
-  CHECK_EQ(0, symbolizer_);
-  // Initialize a dummy symbolizer.
-  symbolizer_ = new(symbolizer_allocator_) Symbolizer;
-  return symbolizer_;
-}
-
 void Symbolizer::AddHooks(Symbolizer::StartSymbolizationHook start_hook,
                           Symbolizer::EndSymbolizationHook end_hook) {
   CHECK(start_hook_ == 0 && end_hook_ == 0);
   start_hook_ = start_hook;
   end_hook_ = end_hook;
 }
 
-Symbolizer::Symbolizer() : start_hook_(0), end_hook_(0) {}
+Symbolizer::Symbolizer(IntrusiveList<SymbolizerTool> tools)
+    : tools_(tools), start_hook_(0), end_hook_(0) {}
 
 Symbolizer::SymbolizerScope::SymbolizerScope(const Symbolizer *sym)
     : sym_(sym) {
   if (sym_->start_hook_)
     sym_->start_hook_();
 }
 
 Symbolizer::SymbolizerScope::~SymbolizerScope() {
   if (sym_->end_hook_)
     sym_->end_hook_();
 }
 
+SymbolizedStack *Symbolizer::SymbolizePC(uptr addr) {
+  BlockingMutexLock l(&mu_);
+  const char *module_name;
+  uptr module_offset;
+  SymbolizedStack *res = SymbolizedStack::New(addr);
+  if (!PlatformFindModuleNameAndOffsetForAddress(addr, &module_name,
+                                                 &module_offset))
+    return res;
+  // Always fill data about module name and offset.
+  res->info.FillModuleInfo(module_name, module_offset);
+  for (auto iter = Iterator(&tools_); iter.hasNext();) {
+    auto *tool = iter.next();
+    SymbolizerScope sym_scope(this);
+    if (tool->SymbolizePC(addr, res)) {
+      return res;
+    }
+  }
+  return res;
+}
+
+bool Symbolizer::SymbolizeData(uptr addr, DataInfo *info) {
+  BlockingMutexLock l(&mu_);
+  const char *module_name;
+  uptr module_offset;
+  if (!PlatformFindModuleNameAndOffsetForAddress(addr, &module_name,
+                                                 &module_offset))
+    return false;
+  info->Clear();
+  info->module = internal_strdup(module_name);
+  info->module_offset = module_offset;
+  for (auto iter = Iterator(&tools_); iter.hasNext();) {
+    auto *tool = iter.next();
+    SymbolizerScope sym_scope(this);
+    if (tool->SymbolizeData(addr, info)) {
+      return true;
+    }
+  }
+  return true;
+}
+
+bool Symbolizer::GetModuleNameAndOffsetForPC(uptr pc, const char **module_name,
+                                 uptr *module_address) {
+  BlockingMutexLock l(&mu_);
+  return PlatformFindModuleNameAndOffsetForAddress(pc, module_name,
+                                                   module_address);
+}
+
+void Symbolizer::Flush() {
+  BlockingMutexLock l(&mu_);
+  for (auto iter = Iterator(&tools_); iter.hasNext();) {
+    auto *tool = iter.next();
+    SymbolizerScope sym_scope(this);
+    tool->Flush();
+  }
+}
+
+const char *Symbolizer::Demangle(const char *name) {
+  BlockingMutexLock l(&mu_);
+  for (auto iter = Iterator(&tools_); iter.hasNext();) {
+    auto *tool = iter.next();
+    SymbolizerScope sym_scope(this);
+    if (const char *demangled = tool->Demangle(name))
+      return demangled;
+  }
+  return PlatformDemangle(name);
+}
+
+void Symbolizer::PrepareForSandboxing() {
+  BlockingMutexLock l(&mu_);
+  PlatformPrepareForSandboxing();
+}
+
 }  // namespace __sanitizer

lib/sanitizer_common/sanitizer_symbolizer_internal.h

 const char *ExtractInt(const char *str, const char *delims, int *result);
 const char *ExtractUptr(const char *str, const char *delims, uptr *result);
 
 // SymbolizerTool is an interface that is implemented by individual "tools"
 // that can perform symbolication (external llvm-symbolizer, libbacktrace,
 // Windows DbgHelp symbolizer, etc.).
 class SymbolizerTool {
  public:
-  // POSIXSymbolizer implements a "fallback chain" of symbolizer tools. In a
-  // request to symbolize an address, if one tool returns false, the next tool
-  // in the chain will be tried.
+  // The main |Symbolizer| class implements a "fallback chain" of symbolizer
+  // tools. In a request to symbolize an address, if one tool returns false,
+  // the next tool in the chain will be tried.
   SymbolizerTool *next;
 
   SymbolizerTool() : next(nullptr) { }
 
   // Can't declare pure virtual functions in sanitizer runtimes:
   // __cxa_pure_virtual might be unavailable.
 
   // The |stack| parameter is inout. It is pre-filled with the address,
   // module base and module offset values and is to be used to construct
   // other stack frames.
   virtual bool SymbolizePC(uptr addr, SymbolizedStack *stack) {
     UNIMPLEMENTED();
   }
 
   // The |info| parameter is inout. It is pre-filled with the module base
   // and module offset values.
   virtual bool SymbolizeData(uptr addr, DataInfo *info) {
     UNIMPLEMENTED();
   }
 
   virtual void Flush() {}
 
-  // Return nullptr to fallback to the default __cxxabiv1 demangler.
+  // Return nullptr to fallback to the default platform-specific demangler.
   virtual const char *Demangle(const char *name) {
     return nullptr;
   }
 };
 
 // SymbolizerProcess encapsulates communication between the tool and
 // external symbolizer program, running in a different subprocess.
 // SymbolizerProcess may not be used from two threads simultaneously.

lib/sanitizer_common/sanitizer_symbolizer_libcdep.cc

const char *ExtractUptr(const char *str, const char *delims, uptr *result) {
   InternalFree(buff);
   return ret;
 }
 
 Symbolizer *Symbolizer::GetOrInit() {
   SpinMutexLock l(&init_mu_);
   if (symbolizer_)
     return symbolizer_;
-  if ((symbolizer_ = PlatformInit()))
-    return symbolizer_;
-  return Disable();
+  symbolizer_ = PlatformInit();
+  CHECK(symbolizer_);
 }
 
 }  // namespace __sanitizer

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

public:
   static InternalSymbolizer *get(LowLevelAllocator *alloc) { return 0; }
 };
 
 #endif  // SANITIZER_SUPPORTS_WEAK_HOOKS
 
 class POSIXSymbolizer : public Symbolizer {
  public:
   explicit POSIXSymbolizer(IntrusiveList<SymbolizerTool> tools)
-      : Symbolizer(), tools_(tools) {}
+      : Symbolizer(tools) {}
 
-  SymbolizedStack *SymbolizePC(uptr addr) override {
-    BlockingMutexLock l(&mu_);
-    const char *module_name;
-    uptr module_offset;
-    SymbolizedStack *res = SymbolizedStack::New(addr);
-    if (!FindModuleNameAndOffsetForAddress(addr, &module_name, &module_offset))
-      return res;
-    // Always fill data about module name and offset.
-    res->info.FillModuleInfo(module_name, module_offset);
-    for (auto iter = Iterator(&tools_); iter.hasNext();) {
-      auto *tool = iter.next();
-      SymbolizerScope sym_scope(this);
-      if (tool->SymbolizePC(addr, res)) {
-        return res;
-      }
-    }
-    return res;
-  }
-
-  bool SymbolizeData(uptr addr, DataInfo *info) override {
-    BlockingMutexLock l(&mu_);
-    LoadedModule *module = FindModuleForAddress(addr);
-    if (module == 0)
-      return false;
-    const char *module_name = module->full_name();
-    uptr module_offset = addr - module->base_address();
-    info->Clear();
-    info->module = internal_strdup(module_name);
-    info->module_offset = module_offset;
-    for (auto iter = Iterator(&tools_); iter.hasNext();) {
-      auto *tool = iter.next();
-      SymbolizerScope sym_scope(this);
-      if (tool->SymbolizeData(addr, info)) {
-        return true;
-      }
-    }
-    return true;
-  }
-
-  bool GetModuleNameAndOffsetForPC(uptr pc, const char **module_name,
-                                   uptr *module_address) override {
-    BlockingMutexLock l(&mu_);
-    return FindModuleNameAndOffsetForAddress(pc, module_name, module_address);
-  }
-
-  bool CanReturnFileLineInfo() override {
-    return !tools_.empty();
-  }
-
-  void Flush() override {
-    BlockingMutexLock l(&mu_);
-    for (auto iter = Iterator(&tools_); iter.hasNext();) {
-      auto *tool = iter.next();
-      SymbolizerScope sym_scope(this);
-      tool->Flush();
-    }
-  }
-
-  const char *Demangle(const char *name) override {
-    BlockingMutexLock l(&mu_);
-    for (auto iter = Iterator(&tools_); iter.hasNext();) {
-      auto *tool = iter.next();
-      SymbolizerScope sym_scope(this);
-      if (const char *demangled = tool->Demangle(name))
-        return demangled;
-    }
+ private:
+  const char *PlatformDemangle(const char *name) override {
     return DemangleCXXABI(name);
   }
 
-  void PrepareForSandboxing() override {
+  void PlatformPrepareForSandboxing() override {
 #if SANITIZER_LINUX && !SANITIZER_ANDROID
-    BlockingMutexLock l(&mu_);
     // Cache /proc/self/exe on Linux.
     CacheBinaryName();
 #endif
   }
 
- private:
   LoadedModule *FindModuleForAddress(uptr address) {
-    mu_.CheckLocked();
     bool modules_were_reloaded = false;
     if (modules_ == 0 || !modules_fresh_) {
       modules_ = (LoadedModule*)(symbolizer_allocator_.Allocate(
           kMaxNumberOfModuleContexts * sizeof(LoadedModule)));
       CHECK(modules_);
       n_modules_ = GetListOfModules(modules_, kMaxNumberOfModuleContexts,
                                     /* filter */ 0);
       CHECK_GT(n_modules_, 0);
if (!modules_were_reloaded) {
       // It's too aggressive to reload the list of modules each time we fail
       // to find a module for a given address.
       modules_fresh_ = false;
       return FindModuleForAddress(address);
     }
     return 0;
   }
 
-  bool FindModuleNameAndOffsetForAddress(uptr address, const char **module_name,
-                                         uptr *module_offset) {
-    mu_.CheckLocked();
+  bool PlatformFindModuleNameAndOffsetForAddress(uptr address,
+                                                 const char **module_name,
+                                                 uptr *module_offset) override {
     LoadedModule *module = FindModuleForAddress(address);
     if (module == 0)
       return false;
     *module_name = module->full_name();
     *module_offset = address - module->base_address();
     return true;
   }
 
   // 16K loaded modules should be enough for everyone.
   static const uptr kMaxNumberOfModuleContexts = 1 << 14;
   LoadedModule *modules_;  // Array of module descriptions is leaked.
   uptr n_modules_;
   // If stale, need to reload the modules before looking up addresses.
   bool modules_fresh_;
-  BlockingMutex mu_;
-
-  typedef IntrusiveList<SymbolizerTool>::Iterator Iterator;
-  IntrusiveList<SymbolizerTool> tools_;
 };
 
 static SymbolizerTool *ChooseSymbolizer(LowLevelAllocator *allocator) {
   if (!common_flags()->symbolize) {
     VReport(2, "Symbolizer is disabled.\n");
     return nullptr;
   }
   if (SymbolizerTool *tool = InternalSymbolizer::get(allocator)) {

lib/sanitizer_common/sanitizer_symbolizer_win.cc

if (SymGetModuleInfo64(GetCurrentProcess(), addr, &mod_info)) {
     return true;
   }
   return false;
 }
 
 // TODO(kuba.brecka): To be merged with POSIXSymbolizer.
 class WinSymbolizer : public Symbolizer {
  public:
-  explicit WinSymbolizer(SymbolizerTool *tool) : Symbolizer(), tool_(tool) {
-    CHECK(tool);
-  }
-  SymbolizedStack *SymbolizePC(uptr addr) override {
-    BlockingMutexLock l(&mu_);
-    const char *module_name;
-    uptr module_offset;
-    SymbolizedStack *res = SymbolizedStack::New(addr);
-    if (FindModuleNameAndOffsetForAddress(addr, &module_name, &module_offset))
-      res->info.FillModuleInfo(module_name, module_offset);
-    tool_->SymbolizePC(addr, res);
-    return res;
-  }
-  bool CanReturnFileLineInfo() override {
-    return true;
-  }
-  const char *Demangle(const char *name) override {
-    BlockingMutexLock l(&mu_);
-    return tool_->Demangle(name);
-  }
-  bool GetModuleNameAndOffsetForPC(uptr pc, const char **module_name,
-                                   uptr *module_address) override {
-    BlockingMutexLock l(&mu_);
-    return FindModuleNameAndOffsetForAddress(pc, module_name, module_address);
-  }
+  explicit WinSymbolizer(IntrusiveList<SymbolizerTool> tools)
+      : Symbolizer(tools) {}
 
-  BlockingMutex mu_;
-  SymbolizerTool *tool_;
+ private:
+  bool PlatformFindModuleNameAndOffsetForAddress(
+      uptr addr, const char **module_name, uptr *module_offset) override {
+    return ::FindModuleNameAndOffsetForAddress(addr, module_name,
+                                               module_offset);
+  }
+  const char *PlatformDemangle(const char *name) override { return name; }
+  void PlatformPrepareForSandboxing() override { }
 };
 
 Symbolizer *Symbolizer::PlatformInit() {
-  static bool called_once = false;
-  CHECK(!called_once && "Shouldn't create more than one symbolizer");
-  called_once = true;
-  SymbolizerTool *tool = new(symbolizer_allocator_) WinSymbolizerTool();
-  return new(symbolizer_allocator_) WinSymbolizer(tool);
+  IntrusiveList<SymbolizerTool> list;
+  list.clear();
+  list.push_back(new(symbolizer_allocator_) WinSymbolizerTool());
+  return new(symbolizer_allocator_) WinSymbolizer(list);
 }
 
 }  // namespace __sanitizer
 
 #endif  // _WIN32