[compiler-rt] Symbolizer refactoring: Make WinSymbolizer use SymbolizerTool interface

[kubamracek]

Summary

Part of http://reviews.llvm.org/D7827. This converts most of WinSymbolizer into WinSymbolizerTool, which uses the common SymbolizerTool. To make the changes more smooth (and patches more readable), I didn't mess up with POSIXSymbolizer, yet. But eventually, I want to "upgrade" POSIXSymbolizer into a cross-platform Symbolizer, that would use WinSymbolizer just as any other symbolizer tool.

For the transition, I made a small class that does the same as POSIXSymbolizer but for Windows.

One issue with the WinSymbolizer is that is uses DbgHelp even for finding module name and offset from an address. I made it a static function, which also needed the initialization functions to be static and the initialized_ field global. The other option would be to add the FindModuleNameAndOffsetForAddress into the SymbolizerTool interface, which however doesn't sound right to me: On POSIX, all the symbolizer tools use the same way to finding module names, so it looks like it's more of a platform-specific thing than a tool-specific one.

[rnk]

+timurrrr for WinASan

[kubamracek]

About FindModuleNameAndOffsetForAddress: Maybe we could simply implement sanitizer_win.cc's GetListOfModules using Win32 APIs EnumProcessModules, GetModuleBaseName and GetModuleInformation. This way we wouldn't require an initialized DbgHelp.dll.

[timurrrr]

Re: EnumProcessModules -- see also DumpProcessMap() in sanitizer_win.cc.
What's the problem with FindModuleNameAndOffsetForAddress initializing dbghelp?

lib/sanitizer_common/sanitizer_symbolizer_win.cc

29

The implementation is trivial, please make it inline.

156

I think this should be

if (FindModuleNameAndOffsetForAddress(addr, &module_name, &module_offset))
  res->info.FillModuleInfo(module_name, module_offset);

lib/sanitizer_common/sanitizer_symbolizer_win.h

21

Looking at the code in lib/sanitizer_common/sanitizer_symbolizer_internal.h, SymbolizerTool comments assume POSIX. Can you please update the comments there to remove this assumption?

[kubamracek]

Updating patch.

SymbolizerTool comments assume POSIX. Can you please update the comments there to remove this assumption?

I've added a general comment about SymbolizerTool, but currently POSIXSymbolizer is really the only one that implements the chain of symbolizers. After I merge POSIXSymbolizer and WinSymbolizer into just Symbolizer, I will update the comment.

What's the problem with FindModuleNameAndOffsetForAddress initializing dbghelp?

There's no problem, if you're fine with moving the methods and field outside of the WinSymbolizer class. I was just thinking that we might use the same implementation of FindModuleNameAndOffsetForAddress that we have in POSIXSymbolizer, which would simplify sanitizer_symbolizer_win.cc even more (maybe I'll try to do it as a separate patch).

[timurrrr]

Do you have a Windows box where you can test the patch or do you need help with that?

lib/sanitizer_common/sanitizer_symbolizer_win.cc

26

please rename to is_dbghelp_initialized;

30–34

Please rename to InitializeDbgHelpIfNeeded and move to the top of the file.
Please add a comment that calls to this function should be synchronized, as well as other calls to dbghelp.

You may also consider using an anonymous namespace instead of static and group other dbghelp-related things in that namespace.

60–100

ditto -- please move to the top of the file

150

nit: please add a CHECK that tool is nonzero.

[kubamracek]

Addressing review comments.

Do you have a Windows box where you can test the patch or do you need help with that?

Yes, I'm testing the patch on a Windows machine.

[timurrrr]

Looks good to me, thanks!

lib/sanitizer_common/sanitizer_symbolizer_win.cc

21

Please make sure this order of includes passes ninja SanitizerLintCheck.
I'm not sure the latter works on Windows, I'm usually doing that on my Linux box before committing.

91

this function should not be placed in the anonymous namespace if is [planned to be] used in other source files.

[kubamracek]

Please make sure this order of includes passes ninja SanitizerLintCheck. I'm not sure the latter works on Windows, I'm usually doing that on my Linux box before committing.

It does pass the lint check.

this function should not be placed in the anonymous namespace

Moved back.

Landed in r231478.

lib/sanitizer_common/sanitizer_symbolizer_internal.h

 // Parsing helpers, 'str' is searched for delimiter(s) and a string or uptr
 // is extracted. When extracting a string, a newly allocated (using
 // InternalAlloc) and null-terminataed buffer is returned. They return a pointer
 // to the next characted after the found delimiter.
 const char *ExtractToken(const char *str, const char *delims, char **result);
 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.
   SymbolizerTool *next;
 
   SymbolizerTool() : next(nullptr) { }

lib/sanitizer_common/sanitizer_symbolizer_win.h

+//===-- sanitizer_symbolizer_win.h ------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// Header file for the Windows symbolizer tool.
+//
+//===----------------------------------------------------------------------===//
+#ifndef SANITIZER_SYMBOLIZER_WIN_H
+#define SANITIZER_SYMBOLIZER_WIN_H
+
+#include "sanitizer_symbolizer_internal.h"
+
+namespace __sanitizer {
+
+class WinSymbolizerTool : public SymbolizerTool {
+ public:
+  bool SymbolizePC(uptr addr, SymbolizedStack *stack) override;
+  bool SymbolizeData(uptr addr, DataInfo *info) override {
+    return false;
+  }
+  const char *Demangle(const char *name) override;
+};
+
+}  // namespace __sanitizer
+
+#endif  // SANITIZER_SYMBOLIZER_WIN_H

lib/sanitizer_common/sanitizer_symbolizer_win.cc

 //===----------------------------------------------------------------------===//
 
 #include "sanitizer_platform.h"
 #if SANITIZER_WINDOWS
 #include <windows.h>
 #include <dbghelp.h>
 #pragma comment(lib, "dbghelp.lib")
 
-#include "sanitizer_symbolizer.h"
+#include "sanitizer_symbolizer_win.h"
+#include "sanitizer_symbolizer_internal.h"
 
 namespace __sanitizer {
 
-class WinSymbolizer : public Symbolizer {
- public:
-  WinSymbolizer() : initialized_(false) {}
-
-  SymbolizedStack *SymbolizePC(uptr addr) override {
-    SymbolizedStack *frame = SymbolizedStack::New(addr);
+namespace {
 
-    BlockingMutexLock l(&dbghelp_mu_);
-    InitializeIfNeeded();
-
-    // See http://msdn.microsoft.com/en-us/library/ms680578(VS.85).aspx
-    char buffer[sizeof(SYMBOL_INFO) + MAX_SYM_NAME * sizeof(CHAR)];
-    PSYMBOL_INFO symbol = (PSYMBOL_INFO)buffer;
-    symbol->SizeOfStruct = sizeof(SYMBOL_INFO);
-    symbol->MaxNameLen = MAX_SYM_NAME;
-    DWORD64 offset = 0;
-    BOOL got_objname = SymFromAddr(GetCurrentProcess(),
-                                   (DWORD64)addr, &offset, symbol);
-    if (!got_objname)
-      return frame;
+bool is_dbghelp_initialized = false;
 
-    DWORD unused;
-    IMAGEHLP_LINE64 line_info;
-    line_info.SizeOfStruct = sizeof(IMAGEHLP_LINE64);
-    BOOL got_fileline = SymGetLineFromAddr64(GetCurrentProcess(), (DWORD64)addr,
-                                             &unused, &line_info);
-    frame->info.function = internal_strdup(symbol->Name);
-    frame->info.function_offset = (uptr)offset;
-    if (got_fileline) {
-      frame->info.file = internal_strdup(line_info.FileName);
-      frame->info.line = line_info.LineNumber;
-    }
-
-    IMAGEHLP_MODULE64 mod_info;
-    internal_memset(&mod_info, 0, sizeof(mod_info));
-    mod_info.SizeOfStruct = sizeof(mod_info);
-    if (SymGetModuleInfo64(GetCurrentProcess(), addr, &mod_info))
-      frame->info.FillModuleInfo(mod_info.ImageName,
-                                 addr - (uptr)mod_info.BaseOfImage);
-    return frame;
-  }
-
-  bool CanReturnFileLineInfo() override {
-    return true;
-  }
-
-  const char *Demangle(const char *name) override {
-    CHECK(initialized_);
-    static char demangle_buffer[1000];
-    if (name[0] == '\01' &&
-        UnDecorateSymbolName(name + 1, demangle_buffer, sizeof(demangle_buffer),
-                             UNDNAME_NAME_ONLY))
-      return demangle_buffer;
-    else
-      return name;
-  }
+bool TrySymInitialize() {
+  SymSetOptions(SYMOPT_DEFERRED_LOADS | SYMOPT_UNDNAME | SYMOPT_LOAD_LINES);
+  return SymInitialize(GetCurrentProcess(), 0, TRUE);
+  // FIXME: We don't call SymCleanup() on exit yet - should we?
 
-  // FIXME: Implement GetModuleNameAndOffsetForPC().
-
- private:
-  void InitializeIfNeeded() {
-    if (initialized_)
-      return;
-    if (!TrySymInitialize()) {
-      // OK, maybe the client app has called SymInitialize already.
-      // That's a bit unfortunate for us as all the DbgHelp functions are
-      // single-threaded and we can't coordinate with the app.
-      // FIXME: Can we stop the other threads at this point?
-      // Anyways, we have to reconfigure stuff to make sure that SymInitialize
-      // has all the appropriate options set.
-      // Cross our fingers and reinitialize DbgHelp.
-      Report("*** WARNING: Failed to initialize DbgHelp!              ***\n");
-      Report("*** Most likely this means that the app is already      ***\n");
-      Report("*** using DbgHelp, possibly with incompatible flags.    ***\n");
-      Report("*** Due to technical reasons, symbolization might crash ***\n");
-      Report("*** or produce wrong results.                           ***\n");
-      SymCleanup(GetCurrentProcess());
-      TrySymInitialize();
-    }
-    initialized_ = true;
+// Initializes DbgHelp library, if it's not yet initialized. Calls to this
+// function should be synchronized with respect to other calls to DbgHelp API
+// (e.g. from WinSymbolizerTool).
+void InitializeDbgHelpIfNeeded() {
+  if (is_dbghelp_initialized)
+  is_dbghelp_initialized = true;
 
-    // When an executable is run from a location different from the one where it
-    // was originally built, we may not see the nearby PDB files.
-    // To work around this, let's append the directory of the main module
-    // to the symbol search path.  All the failures below are not fatal.
-    const size_t kSymPathSize = 2048;
-    static wchar_t path_buffer[kSymPathSize + 1 + MAX_PATH];
-    if (!SymGetSearchPathW(GetCurrentProcess(), path_buffer, kSymPathSize)) {
-      Report("*** WARNING: Failed to SymGetSearchPathW ***\n");
-      return;
-    }
-    size_t sz = wcslen(path_buffer);
-    if (sz) {
-      CHECK_EQ(0, wcscat_s(path_buffer, L";"));
-      sz++;
-    }
-    DWORD res = GetModuleFileNameW(NULL, path_buffer + sz, MAX_PATH);
-    if (res == 0 || res == MAX_PATH) {
-      Report("*** WARNING: Failed to getting the EXE directory ***\n");
-      return;
-    }
-    // Write the zero character in place of the last backslash to get the
-    // directory of the main module at the end of path_buffer.
-    wchar_t *last_bslash = wcsrchr(path_buffer + sz, L'\\');
-    CHECK_NE(last_bslash, 0);
-    *last_bslash = L'\0';
-    if (!SymSetSearchPathW(GetCurrentProcess(), path_buffer)) {
-      Report("*** WARNING: Failed to SymSetSearchPathW\n");
-      return;
-    }
-  }
 
-  bool TrySymInitialize() {
-    SymSetOptions(SYMOPT_DEFERRED_LOADS | SYMOPT_UNDNAME | SYMOPT_LOAD_LINES);
-    return SymInitialize(GetCurrentProcess(), 0, TRUE);
-    // FIXME: We don't call SymCleanup() on exit yet - should we?
+bool FindModuleNameAndOffsetForAddress(uptr addr, const char **module_name,
+                                       uptr *module_offset) {
+  InitializeDbgHelpIfNeeded();
+
+  IMAGEHLP_MODULE64 mod_info;
+  internal_memset(&mod_info, 0, sizeof(mod_info));
+  mod_info.SizeOfStruct = sizeof(mod_info);
+  if (SymGetModuleInfo64(GetCurrentProcess(), addr, &mod_info)) {
+    *module_name = mod_info.ImageName;
+    *module_offset = addr - (uptr)mod_info.BaseOfImage;
+    return true;
+  }
+  return false;
+}
+
+}  // namespace
+
+bool WinSymbolizerTool::SymbolizePC(uptr addr, SymbolizedStack *frame) {
+  InitializeDbgHelpIfNeeded();
+
+  // See http://msdn.microsoft.com/en-us/library/ms680578(VS.85).aspx
+  char buffer[sizeof(SYMBOL_INFO) + MAX_SYM_NAME * sizeof(CHAR)];
+  PSYMBOL_INFO symbol = (PSYMBOL_INFO)buffer;
+  symbol->SizeOfStruct = sizeof(SYMBOL_INFO);
+  symbol->MaxNameLen = MAX_SYM_NAME;
+  DWORD64 offset = 0;
+  BOOL got_objname = SymFromAddr(GetCurrentProcess(),
+                                 (DWORD64)addr, &offset, symbol);
+  if (!got_objname)
+    return false;
+
+  DWORD unused;
+  IMAGEHLP_LINE64 line_info;
+  line_info.SizeOfStruct = sizeof(IMAGEHLP_LINE64);
+  BOOL got_fileline = SymGetLineFromAddr64(GetCurrentProcess(), (DWORD64)addr,
+                                           &unused, &line_info);
+  frame->info.function = internal_strdup(symbol->Name);
+  frame->info.function_offset = (uptr)offset;
+  if (got_fileline) {
+    frame->info.file = internal_strdup(line_info.FileName);
+    frame->info.line = line_info.LineNumber;
+  }
+  return true;
+}
+
+const char *WinSymbolizerTool::Demangle(const char *name) {
+  CHECK(is_dbghelp_initialized);
+  static char demangle_buffer[1000];
+  if (name[0] == '\01' &&
+      UnDecorateSymbolName(name + 1, demangle_buffer, sizeof(demangle_buffer),
+                           UNDNAME_NAME_ONLY))
+    return demangle_buffer;
+  else
+    return name;
+}
+
+// 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);
   }
 
-  // All DbgHelp functions are single threaded, so we should use a mutex to
-  // serialize accesses.
-  BlockingMutex dbghelp_mu_;
-  bool initialized_;
+  BlockingMutex mu_;
+  SymbolizerTool *tool_;
 };
 
 Symbolizer *Symbolizer::PlatformInit() {
   static bool called_once = false;
   CHECK(!called_once && "Shouldn't create more than one symbolizer");
   called_once = true;
-  return new(symbolizer_allocator_) WinSymbolizer();
+  SymbolizerTool *tool = new(symbolizer_allocator_) WinSymbolizerTool();
+  return new(symbolizer_allocator_) WinSymbolizer(tool);
 }
 
 }  // namespace __sanitizer
 
 #endif  // _WIN32