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

[Alexey Samsonov]

Looks mostly good.


================
Comment at: lib/sanitizer_common/sanitizer_symbolizer.h:88
@@ -93,1 +87,3 @@
+  bool GetModuleNameAndOffsetForPC(uptr pc, const char **module_name,
+                                           uptr *module_address);
   const char *GetModuleNameForPc(uptr pc) {
----------------
run clang-format over the changed lines.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer.h:96
@@ -99,5 +95,3 @@
   }
-  virtual bool CanReturnFileLineInfo() {
-    return false;
-  }
+  bool CanReturnFileLineInfo();
   // Release internal caches (if any).
----------------
This function is short, you can just inline it here.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer.h:117
@@ +116,3 @@
+
+  virtual bool FindModuleNameAndOffsetForAddress(uptr address,
+                                                 const char **module_name,
----------------
This is also Platform-specific function, might make sense to reflect it in the name.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer.h:132
@@ -128,1 +131,3 @@
   static StaticSpinMutex init_mu_;
+  BlockingMutex mu_;
+
----------------
timurrrr wrote:
> 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?..
Right, just add a comment here.
Timur: I'd rather not add extra directory for that - that would make build rules more complex.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:357
@@ -422,1 +356,3 @@
+ private:
+  virtual const char *PlatformDemangle(const char *name) override {
     return DemangleCXXABI(name);
----------------
no need for virtual here.

http://reviews.llvm.org/D8105

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/