[PATCH] Overhaul the symbolizer interface.

[Alexey Samsonov]

================
Comment at: lib/msan/msan.cc:333
@@ -332,2 +332,3 @@
   const char *external_symbolizer = common_flags()->external_symbolizer_path;
   bool symbolizer_started =
+      InitSymbolizer(external_symbolizer)->IsExternalAvailable();
----------------
s/symbolizer_started/external_symbolizer_started

================
Comment at: lib/tsan/rtl/tsan_rtl.cc:241
@@ -240,2 +240,3 @@
   const char *external_symbolizer = flags()->external_symbolizer_path;
   bool symbolizer_started =
+      InitSymbolizer(external_symbolizer)->IsExternalAvailable();
----------------
s/symbolizer_started/external_symbolizer_started

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:465
@@ +464,3 @@
+  bool IsExternalAvailable() {
+    return external_symbolizer_;
+  }
----------------
while you're here, please use form "return external_symbolizer_ != 0;" here and in IsAvailable()

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:476
@@ -482,3 +475,3 @@
   const char *Demangle(const char *name) {
     if (IsAvailable() && internal_symbolizer_ != 0)
       return internal_symbolizer_->Demangle(name);
----------------
IsAvailable() is redundant now.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:491
@@ -497,3 +490,3 @@
     // First, try to use internal symbolizer.
     if (!IsAvailable()) {
       return 0;
----------------
IsAvailable() is redundant.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:573
@@ -578,2 +572,3 @@
 
+static ALIGNED(64) char ext_symbolizer_placeholder[sizeof(ExternalSymbolizer)];
 static ALIGNED(64) char symbolizer_placeholder[sizeof(Symbolizer)];
----------------
I think we should either consistently allocate InternalSymbolizer, ExternalSymbolizer and Symbolizer singletones with symbolizer_allocator, or use this placeholder hack for all three of them (The former should work fine for current use case).

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:522
@@ -528,3 +521,3 @@
   LoadedModule *FindModuleForAddress(uptr address) {
     bool modules_were_reloaded = false;
     if (modules_ == 0 || !modules_fresh_) {
----------------
Check that mutex is locked.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:490
@@ -496,3 +489,3 @@
   char *SendCommand(bool is_data, const char *module_name, uptr module_offset) {
     // First, try to use internal symbolizer.
     if (!IsAvailable()) {
----------------
check that mutex is locked.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer.h:115
@@ +114,3 @@
+// symbolizer.  The path argument is only required for legacy reasons as this
+// function will check $LLVM_SYMBOLIZER_PATH and $PATH for an external
+// symbolizer.  Not thread safe.
----------------
Please update the comment

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:468
@@ -475,2 +467,3 @@
+
   void Flush() {
     if (internal_symbolizer_)
----------------
Why not lock the mutex in the rest of the functions as well? It does make sense for InternalSymbolizer

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:569
@@ -574,3 +568,3 @@
 
   ExternalSymbolizer *external_symbolizer_;  // Leaked.
   InternalSymbolizer *internal_symbolizer_;  // Leaked.
----------------
These are now {External,Internal}Symbolizer const *

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_internal.h:19
@@ +18,3 @@
+class SymbolizerInterface;
+struct atomic_uintptr_t;
+
----------------
I am still not convinced that you need one more header. I think static private pointer in SymbolizerInterface, pointing to actual platform-specific implementation of this interface would make sense.


http://llvm-reviews.chandlerc.com/D1985