[compiler-rt] Symbolizer refactoring: Implement GetListOfModules on Windows

[kubamracek]

Summary

One more step towards unifying the symbolizer classes. This time, I'm implementing GetListOfModules on Windows, which then allows to use the current "POSIX" implementation of FindModuleNameAndOffsetForAddress and FindModuleForAddress on all platforms.

[timurrrr]

Hi Kuba,

Have you seen the "Sanitizer coverage: PC vs modules are not handled efficiently?" email thread on llvm-commits I've started recently?

Basically, I'm up to refactor this very code and I think the current interface and implementation are suboptimal.
How blocked is your progress by this change? If you can wait a week or two, I can improve the code that uses these functions first and try to simplify the interface.
Since I have to do that anyways, I'm not sure it's efficient for us to do the same thing twice in parallel :)

lib/sanitizer_common/sanitizer_symbolizer.cc

164

This line leaks a lot of memory every time a module lookup fails [e.g. a new shared object is dynamically loaded].
I understand you've just moved this code around, but I think we should come up with a better solution if we've decided to refactor this code.

lib/sanitizer_common/sanitizer_win.cc

226

This line looks dangerous to me now.
LoadedModule is not guaranteed to be qsort-friendly.
[ I think "trivially copyable" is what matters here http://www.cplusplus.com/reference/type_traits/is_trivially_copyable/ ]

It already owns a const char *full_name_;, and one day somebody might decide ~LoadedModule dtor should free up some memory, or hold some non-POD member.

[kubamracek]

Hi Timur,

Have you seen the "Sanitizer coverage: PC vs modules are not handled efficiently?" email thread on llvm-commits I've started recently?
Basically, I'm up to refactor this very code and I think the current interface and implementation are suboptimal.
How blocked is your progress by this change? If you can wait a week or two, I can improve the code that uses these functions first and try to simplify the interface.

Please go ahead with your planned changes and don't worry about this patch now, we can revisit it later. Can you cc me on patches/commits?

Kuba

[timurrrr]

Sure, no problem!

[timurrrr]

lib/sanitizer_common/sanitizer_symbolizer.cc

167

Did it work for you on Linux?

GetListOfModules depends on LibC, common symbolizer interface doesn't.

[timurrrr]

Let's continue on http://reviews.llvm.org/D8805

lib/sanitizer_common/sanitizer_symbolizer.h

public:
   // 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();
 
-  virtual bool PlatformFindModuleNameAndOffsetForAddress(
-      uptr address, const char **module_name, uptr *module_offset) {
-    UNIMPLEMENTED();
-  }
+  LoadedModule *FindModuleForAddress(uptr address);
+  bool FindModuleNameAndOffsetForAddress(uptr address, const char **module_name,
+                                         uptr *module_offset);
+  // 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_;
+
   // 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

lib/sanitizer_common/sanitizer_symbolizer.cc

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))
+  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 Symbolizer::SymbolizeData(uptr addr, DataInfo *info) {
   BlockingMutexLock l(&mu_);
   const char *module_name;
   uptr module_offset;
-  if (!PlatformFindModuleNameAndOffsetForAddress(addr, &module_name,
-                                                 &module_offset))
+  if (!FindModuleNameAndOffsetForAddress(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);
+  return FindModuleNameAndOffsetForAddress(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) {
   return PlatformDemangle(name);
 }
 
 void Symbolizer::PrepareForSandboxing() {
   BlockingMutexLock l(&mu_);
   PlatformPrepareForSandboxing();
 }
 
+LoadedModule *Symbolizer::FindModuleForAddress(uptr address) {
+  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);
+    CHECK_LT(n_modules_, kMaxNumberOfModuleContexts);
+    modules_fresh_ = true;
+    modules_were_reloaded = true;
+  }
+  for (uptr i = 0; i < n_modules_; i++) {
+    if (modules_[i].containsAddress(address)) {
+      return &modules_[i];
+    }
+  }
+  // Reload the modules and look up again, if we haven't tried it yet.
+  if (!modules_were_reloaded) {
+    // FIXME: set modules_fresh_ from dlopen()/dlclose() interceptors.
+    // 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 Symbolizer::FindModuleNameAndOffsetForAddress(uptr address,
+                                                   const char **module_name,
+                                                   uptr *module_offset) {
+  LoadedModule *module = FindModuleForAddress(address);
+  if (module == 0)
+    return false;
+  *module_name = module->full_name();
+  *module_offset = address - module->base_address();
+  return true;
+}
+
 }  // namespace __sanitizer

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

private:
   }
 
   void PlatformPrepareForSandboxing() override {
 #if SANITIZER_LINUX && !SANITIZER_ANDROID
     // Cache /proc/self/exe on Linux.
     CacheBinaryName();
 #endif
   }
-
-  LoadedModule *FindModuleForAddress(uptr address) {
-    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);
-      CHECK_LT(n_modules_, kMaxNumberOfModuleContexts);
-      modules_fresh_ = true;
-      modules_were_reloaded = true;
-    }
-    for (uptr i = 0; i < n_modules_; i++) {
-      if (modules_[i].containsAddress(address)) {
-        return &modules_[i];
-      }
-    }
-    // Reload the modules and look up again, if we haven't tried it yet.
-    if (!modules_were_reloaded) {
-      // FIXME: set modules_fresh_ from dlopen()/dlclose() interceptors.
-      // 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 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_;
 };
 
 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

const char *WinSymbolizerTool::Demangle(const char *name) {
   if (name[0] == '\01' &&
       UnDecorateSymbolName(name + 1, demangle_buffer, sizeof(demangle_buffer),
                            UNDNAME_NAME_ONLY))
     return demangle_buffer;
   else
     return name;
 }
 
-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;
-}
-
 // TODO(kuba.brecka): To be merged with POSIXSymbolizer.
 class WinSymbolizer : public Symbolizer {
  public:
   explicit WinSymbolizer(IntrusiveList<SymbolizerTool> tools)
       : Symbolizer(tools) {}
 
  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() {
   IntrusiveList<SymbolizerTool> list;
   list.clear();
   list.push_back(new(symbolizer_allocator_) WinSymbolizerTool());
   return new(symbolizer_allocator_) WinSymbolizer(list);
 }
 
 }  // namespace __sanitizer
 
 #endif  // _WIN32

lib/sanitizer_common/sanitizer_win.cc

const char *GetPwd() {
   UNIMPLEMENTED();
 }
 
 u32 GetUid() {
   UNIMPLEMENTED();
 }
 
 namespace {
-struct ModuleInfo {
-  HMODULE handle;
-  uptr base_address;
-  uptr end_address;
-};
-
 int CompareModulesBase(const void *pl, const void *pr) {
-  const ModuleInfo &l = *(ModuleInfo *)pl, &r = *(ModuleInfo *)pr;
-  if (l.base_address < r.base_address)
+  const LoadedModule *l = (LoadedModule *)pl, *r = (LoadedModule *)pr;
+  if (l->base_address() < r->base_address())
     return -1;
-  return l.base_address > r.base_address;
+  return l->base_address() > r->base_address();
 }
 }  // namespace
 
 #ifndef SANITIZER_GO
 void DumpProcessMap() {
   Report("Dumping process modules:\n");
-  HANDLE cur_process = GetCurrentProcess();
 
-  // Query the list of modules.  Start by assuming there are no more than 256
-  // modules and retry if that's not sufficient.
-  ModuleInfo *modules;
-  size_t num_modules;
-  {
-    HMODULE *hmodules = 0;
-    uptr modules_buffer_size = sizeof(HMODULE) * 256;
-    DWORD bytes_required;
-    while (!hmodules) {
-      hmodules = (HMODULE *)MmapOrDie(modules_buffer_size, __FUNCTION__);
-      CHECK(EnumProcessModules(cur_process, hmodules, modules_buffer_size,
-                               &bytes_required));
-      if (bytes_required > modules_buffer_size) {
-        // Either there turned out to be more than 256 hmodules, or new hmodules
-        // could have loaded since the last try.  Retry.
-        UnmapOrDie(hmodules, modules_buffer_size);
-        hmodules = 0;
-        modules_buffer_size = bytes_required;
-      }
-    }
+  static const uptr kMaxNumberOfModules = 1 << 14;
+  InternalScopedBuffer<LoadedModule> modules(kMaxNumberOfModules);
+  CHECK(modules.data());
+  int num_modules = GetListOfModules(modules.data(), kMaxNumberOfModules,
+                                     /* filter */ 0);
 
-    num_modules = bytes_required / sizeof(HMODULE);
-    modules =
-        (ModuleInfo *)MmapOrDie(num_modules * sizeof(ModuleInfo), __FUNCTION__);
-    for (size_t i = 0; i < num_modules; ++i) {
-      modules[i].handle = hmodules[i];
-      MODULEINFO mi;
-      if (!GetModuleInformation(cur_process, hmodules[i], &mi, sizeof(mi)))
-        continue;
-      modules[i].base_address = (uptr)mi.lpBaseOfDll;
-      modules[i].end_address = (uptr)mi.lpBaseOfDll + mi.SizeOfImage;
-    }
-    UnmapOrDie(hmodules, modules_buffer_size);
-  }
-
-  qsort(modules, num_modules, sizeof(ModuleInfo), CompareModulesBase);
+  qsort(modules.data(), num_modules, sizeof(LoadedModule), CompareModulesBase);
 
   for (size_t i = 0; i < num_modules; ++i) {
-    const ModuleInfo &mi = modules[i];
-    char module_name[MAX_PATH];
-    bool got_module_name = GetModuleFileNameA(
-        mi.handle, module_name, sizeof(module_name));
-    if (mi.end_address != 0) {
-      Printf("\t%p-%p %s\n", mi.base_address, mi.end_address,
-             got_module_name ? module_name : "[no name]");
-    } else if (got_module_name) {
-      Printf("\t??\?-??? %s\n", module_name);
-    } else {
-      Printf("\t???\n");
-    }
+    const char *module_name = modules[i].full_name();
+    // On Windows, the module contains a single address range describing the
+    // whole module. 
+    uptr beg = modules[i].ranges().next()->beg;
+    uptr end = modules[i].ranges().next()->end;
+    Printf("\t%p-%p %s\n", beg, end, module_name);
+    modules[i].clear();
   }
-  UnmapOrDie(modules, num_modules * sizeof(ModuleInfo));
 }
 #endif
 
 void DisableCoreDumperIfNecessary() {
   // Do nothing.
 }
 
 void ReExec() {

 void Abort() {
   if (::IsDebuggerPresent())
     __debugbreak();
   internal__exit(3);
 }
 
 uptr GetListOfModules(LoadedModule *modules, uptr max_modules,
                       string_predicate_t filter) {
-  UNIMPLEMENTED();
+  HANDLE cur_process = GetCurrentProcess();
+
+  // Query the list of modules.  Start by assuming there are no more than 256
+  // modules and retry if that's not sufficient.
+  HMODULE *hmodules = 0;
+  uptr modules_buffer_size = sizeof(HMODULE) * 256;
+  DWORD bytes_required;
+  while (!hmodules) {
+    hmodules = (HMODULE *)MmapOrDie(modules_buffer_size, __FUNCTION__);
+    CHECK(EnumProcessModules(cur_process, hmodules, modules_buffer_size,
+                             &bytes_required));
+    if (bytes_required > modules_buffer_size) {
+      // Either there turned out to be more than 256 hmodules, or new hmodules
+      // could have loaded since the last try.  Retry.
+      UnmapOrDie(hmodules, modules_buffer_size);
+      hmodules = 0;
+      modules_buffer_size = bytes_required;
+    }
+  }
+
+  size_t n = bytes_required / sizeof(HMODULE);
+  size_t num_modules = 0;
+  for (size_t i = 0; i < n && num_modules < max_modules; ++i) {
+    HMODULE handle = hmodules[i];
+    MODULEINFO mi;
+    if (!GetModuleInformation(cur_process, handle, &mi, sizeof(mi)))
+      continue;
+    uptr base_address = (uptr)mi.lpBaseOfDll;
+    uptr end_address = (uptr)mi.lpBaseOfDll + mi.SizeOfImage;
+
+    char module_name[MAX_PATH];
+    bool got_module_name =
+        GetModuleFileNameA(handle, module_name, sizeof(module_name));
+    if (!got_module_name)
+      module_name[0] = '\0';
+
+    if (filter && !filter(module_name))
+      continue;
+
+    void *mem = &modules[num_modules];
+    LoadedModule *cur_module = new(mem) LoadedModule(module_name, base_address);
+    // We add the whole module as one single address range.
+    cur_module->addAddressRange(base_address, end_address, /*executable*/ true);
+    num_modules++;
+  }
+  UnmapOrDie(hmodules, modules_buffer_size);
+
+  return num_modules;
 };
 
 #ifndef SANITIZER_GO
 int Atexit(void (*function)(void)) {
   return atexit(function);
 }
 #endif