[compiler-rt] Symbolizer refactoring: Unify access to symbolizer tools from POSIXSymbolizer

[kubamracek]

Summary

Part of http://reviews.llvm.org/D7827. The way we select symbolizer tools in sanitizer_symbolizer_posix_libcdep.cc's Symbolizer::PlatformInit currently uses only one (or zero) of them. And since all the POSIX symbolizers now use the commmon interface, we don't need to special-case the three classes anymore.

To maintain the current behavior, I changed the Demangle method in SymbolizerTool interface to return nullptr by default, which means that POSIXSymbolizer will fallback to use DemangleCXXABI. And in SymbolizeData of POSIXSymbolizer, the addition of the module base address (info->start += module->base_address()) has been moved into the individual tools, because not all tool parse back a relative offset (libbacktrace already fills in the full address).

[samsonov]

I'm happy with the direction of this change, although:

Note that this does introduce a change in behavior - in case both libbacktrace *and* external symbolizer are available, we could fallback from former to latter. Is there a strong reason for removing this fallback? I originally thought that it can be useful in symbolizers you were going to add.

[kubamracek]

In D8029#133837, @samsonov wrote:

I'm happy with the direction of this change, although:

Note that this does introduce a change in behavior - in case both libbacktrace *and* external symbolizer are available, we could fallback from former to latter. Is there a strong reason for removing this fallback? I originally thought that it can be useful in symbolizers you were going to add.

The initialization of the external symbolizer (in PlatformInit) is wrapped inside if (!libbacktrace_symbolizer), so it doesn't look to me that we currently have such a fallback. Or am I reading the code wrong?

Anyway, I definitely support having fallbacks, and I plan to submit another patch that implements the chain of symbolizers.

[samsonov]

In D8029#133842, @kubabrecka wrote:

In D8029#133837, @samsonov wrote:

I'm happy with the direction of this change, although:

Note that this does introduce a change in behavior - in case both libbacktrace *and* external symbolizer are available, we could fallback from former to latter. Is there a strong reason for removing this fallback? I originally thought that it can be useful in symbolizers you were going to add.

The initialization of the external symbolizer (in PlatformInit) is wrapped inside if (!libbacktrace_symbolizer), so it doesn't look to me that we currently have such a fallback. Or am I reading the code wrong?

Ah, you are right. Feel free to submit this change first if it's convenient for you.

Anyway, I definitely support having fallbacks, and I plan to submit another patch that implements the chain of symbolizers.

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

482

I would re-write this function using early returns:

static SymbolizerTool *ChooseSymbolizer() {
  if (!common_flags()->symbolize)
    return nullptr;
  if (SymbolizerTool *tool = InternalSymbolizer::get(..)) {
    return tool;
  }
  if (SymbolizerTool *tool = LiibacktraceSymbolizer::get(..)) {
    return tool;
  }
  <...>
}

and then have a single `new(symbolizer_allocator_) POSIXSymbolizer(ChooseSymbolizer());`

[kubamracek]

Updating patch.

[samsonov]

LGTM. Do you plan to inherit WinSymbolizer from SymbolizerTool?

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

511

Restore the comment here (external symbolizer is explicitly disabled).

[kubamracek]

Landed in r231162.

LGTM. Do you plan to inherit WinSymbolizer from SymbolizerTool?

Yes, although it's not as straight-forward as I'd like. I planned to move the implementation of POSIXSymbolizer into just Symbolizer, which would no longer have any subclasses. The ChooseSymbolizer would then simply have a if (SANITIZER_WINDOWS) branch in it, and we wouldn't need PlatformInit at all.

Speaking of Windows, I found out that we currently don't run the testcases from the main TestCases/ directory on Windows (we treat them as if they were in TestCases/Posix/), is that intentional? If yes, what's the point of having a separate Posix subdirectory?

lib/sanitizer_common/sanitizer_symbolizer_internal.h

public:
   // 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.
   virtual const char *Demangle(const char *name) {
-    return 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.
 class SymbolizerProcess {
  public:

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

bool SymbolizePC(uptr addr, SymbolizedStack *stack) override {
     }
     return false;
   }
 
   bool SymbolizeData(uptr addr, DataInfo *info) override {
     if (const char *buf =
             SendCommand(/*is_data*/ true, info->module, info->module_offset)) {
       ParseSymbolizeDataOutput(buf, info);
+      info->start += (addr - info->module_offset);  // Add the base address.
       return true;
     }
     return false;
   }
 
  private:
   const char *SendCommand(bool is_data, const char *module_name,
                           uptr module_offset) {
bool result = __sanitizer_symbolize_code(
         stack->info.module, stack->info.module_offset, buffer_, kBufferSize);
     if (result) ParseSymbolizePCOutput(buffer_, stack);
     return result;
   }
 
   bool SymbolizeData(uptr addr, DataInfo *info) override {
     bool result = __sanitizer_symbolize_data(info->module, info->module_offset,
                                              buffer_, kBufferSize);
-    if (result) ParseSymbolizeDataOutput(buffer_, info);
+    if (result) {
+      ParseSymbolizeDataOutput(buffer_, info);
+      info->start += (addr - info->module_offset);  // Add the base address.
+    }
     return result;
   }
 
   void Flush() override {
     if (__sanitizer_symbolize_flush)
       __sanitizer_symbolize_flush();
   }
 

  public:
   static InternalSymbolizer *get(LowLevelAllocator *alloc) { return 0; }
 };
 
 #endif  // SANITIZER_SUPPORTS_WEAK_HOOKS
 
 class POSIXSymbolizer : public Symbolizer {
  public:
-  POSIXSymbolizer(SymbolizerTool *external_symbolizer,
-                  SymbolizerTool *internal_symbolizer,
-                  LibbacktraceSymbolizer *libbacktrace_symbolizer)
-      : Symbolizer(),
-        external_symbolizer_(external_symbolizer),
-        internal_symbolizer_(internal_symbolizer),
-        libbacktrace_symbolizer_(libbacktrace_symbolizer) {}
+  explicit POSIXSymbolizer(SymbolizerTool *symbolizer_tool)
+      : Symbolizer(), symbolizer_tool_(symbolizer_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))
       return res;
     // Always fill data about module name and offset.
     res->info.FillModuleInfo(module_name, module_offset);
-    // First, try to use libbacktrace symbolizer (if it's available).
-    if (libbacktrace_symbolizer_ != 0) {
-      mu_.CheckLocked();
-      if (libbacktrace_symbolizer_->SymbolizePC(addr, res))
-        return res;
-    }
-    if (SymbolizerTool *tool = GetSymbolizerTool()) {
+    if (symbolizer_tool_) {
       SymbolizerScope sym_scope(this);
-      tool->SymbolizePC(addr, res);
+      symbolizer_tool_->SymbolizePC(addr, 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;
-    // First, try to use libbacktrace symbolizer (if it's available).
-    if (libbacktrace_symbolizer_ != 0) {
-      mu_.CheckLocked();
-      if (libbacktrace_symbolizer_->SymbolizeData(addr, info))
-        return true;
-    }
-    if (SymbolizerTool *tool = GetSymbolizerTool()) {
+    if (symbolizer_tool_) {
       SymbolizerScope sym_scope(this);
-      tool->SymbolizeData(addr, info);
+      symbolizer_tool_->SymbolizeData(addr, info);
     }
-    info->start += module->base_address();
     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 internal_symbolizer_ != 0 || external_symbolizer_ != 0 ||
-           libbacktrace_symbolizer_ != 0;
+    return symbolizer_tool_ != nullptr;
   }
 
   void Flush() override {
     BlockingMutexLock l(&mu_);
-    if (internal_symbolizer_ != 0) {
+    if (symbolizer_tool_) {
       SymbolizerScope sym_scope(this);
-      internal_symbolizer_->Flush();
+      symbolizer_tool_->Flush();
     }
   }
 
   const char *Demangle(const char *name) override {
     BlockingMutexLock l(&mu_);
-    // Run hooks even if we don't use internal symbolizer, as cxxabi
-    // demangle may call system functions.
+    if (symbolizer_tool_) {
       SymbolizerScope sym_scope(this);
-    // Try to use libbacktrace demangler (if available).
-    if (libbacktrace_symbolizer_ != 0) {
-      if (const char *demangled = libbacktrace_symbolizer_->Demangle(name))
+      if (const char *demangled = symbolizer_tool_->Demangle(name))
         return demangled;
     }
-    if (internal_symbolizer_ != 0)
-      return internal_symbolizer_->Demangle(name);
     return DemangleCXXABI(name);
   }
 
   void PrepareForSandboxing() override {
 #if SANITIZER_LINUX && !SANITIZER_ANDROID
     BlockingMutexLock l(&mu_);
     // Cache /proc/self/exe on Linux.
     CacheBinaryName();
 #endif
   }
 
  private:
-  SymbolizerTool *GetSymbolizerTool() {
-    mu_.CheckLocked();
-    if (internal_symbolizer_) return internal_symbolizer_;
-    if (external_symbolizer_) return external_symbolizer_;
-    return nullptr;
-  }
-
   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,
private:
   // 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_;
 
-  SymbolizerTool *const external_symbolizer_;         // Leaked.
-  SymbolizerTool *const internal_symbolizer_;         // Leaked.
-  LibbacktraceSymbolizer *libbacktrace_symbolizer_;   // Leaked.
+  SymbolizerTool *const symbolizer_tool_;         // Leaked.
 };
 
 Symbolizer *Symbolizer::PlatformInit() {
   if (!common_flags()->symbolize) {
-    return new(symbolizer_allocator_) POSIXSymbolizer(0, 0, 0);
+    return new(symbolizer_allocator_) POSIXSymbolizer(nullptr);
   }
-  InternalSymbolizer* internal_symbolizer =
-      InternalSymbolizer::get(&symbolizer_allocator_);
-  SymbolizerTool *external_symbolizer = 0;
-  LibbacktraceSymbolizer *libbacktrace_symbolizer = 0;
-
-  if (!internal_symbolizer) {
-    libbacktrace_symbolizer =
-        LibbacktraceSymbolizer::get(&symbolizer_allocator_);
-    if (!libbacktrace_symbolizer) {
+  SymbolizerTool *tool = InternalSymbolizer::get(&symbolizer_allocator_);
+  if (!tool) {
+    tool = LibbacktraceSymbolizer::get(&symbolizer_allocator_);
+    if (!tool) {
       const char *path_to_external = common_flags()->external_symbolizer_path;
       if (path_to_external && path_to_external[0] == '\0') {
         // External symbolizer is explicitly disabled. Do nothing.
       } else {
         // Find path to llvm-symbolizer if it's not provided.
         if (!path_to_external)
           path_to_external = FindPathToBinary("llvm-symbolizer");
         if (path_to_external) {
-          external_symbolizer = new(symbolizer_allocator_)
+          tool = new(symbolizer_allocator_)
               LLVMSymbolizer(path_to_external, &symbolizer_allocator_);
         } else if (common_flags()->allow_addr2line) {
           // If llvm-symbolizer is not found, try to use addr2line.
           if (const char *addr2line_path = FindPathToBinary("addr2line")) {
-            external_symbolizer = new(symbolizer_allocator_)
+            tool = new(symbolizer_allocator_)
                 Addr2LinePool(addr2line_path, &symbolizer_allocator_);
           }
         }
       }
     }
   }
 
-  return new(symbolizer_allocator_) POSIXSymbolizer(
-      external_symbolizer, internal_symbolizer, libbacktrace_symbolizer);
+  return new(symbolizer_allocator_) POSIXSymbolizer(tool);
 }
 
 }  // namespace __sanitizer
 
 #endif  // SANITIZER_POSIX