[PATCH] Overhaul the symbolizer interface.

[Peter Collingbourne]

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_internal.h:19
@@ +18,3 @@
+class SymbolizerInterface;
+struct atomic_uintptr_t;
+
----------------
Alexey Samsonov wrote:
> 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.
OK, done.  (SymbolizerInterface renamed to Symbolizer (and Symbolizer to POSIXSymbolizer); the old name was too much typing).

================
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.
----------------
Alexey Samsonov wrote:
> Please update the comment
Done.

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

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:468
@@ -475,2 +467,3 @@
+
   void Flush() {
     if (internal_symbolizer_)
----------------
Alexey Samsonov wrote:
> Why not lock the mutex in the rest of the functions as well? It does make sense for InternalSymbolizer
Done for Flush, Demangle and PrepareForSandboxing.  Probably not required for Demangle but better safe than sorry I guess.

================
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);
----------------
Alexey Samsonov wrote:
> IsAvailable() is redundant now.
Removed.

================
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()) {
----------------
Alexey Samsonov wrote:
> check that mutex is locked.
Done.

================
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;
----------------
Alexey Samsonov wrote:
> IsAvailable() is redundant.
Removed.

================
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_) {
----------------
Alexey Samsonov wrote:
> Check that mutex is locked.
Done.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:569
@@ -574,3 +568,3 @@
 
   ExternalSymbolizer *external_symbolizer_;  // Leaked.
   InternalSymbolizer *internal_symbolizer_;  // Leaked.
----------------
Alexey Samsonov wrote:
> These are now {External,Internal}Symbolizer const *
Done for internal_symbolizer_. SendCommand may set external_symbolizer_ to 0, so I left it alone.

================
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)];
----------------
Alexey Samsonov wrote:
> 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).
Done.  While I was at it, I decided to write an operator new for LowLevelAllocator, which I committed separately.

================
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();
----------------
Alexey Samsonov wrote:
> s/symbolizer_started/external_symbolizer_started
Done.

================
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();
----------------
Alexey Samsonov wrote:
> s/symbolizer_started/external_symbolizer_started
Done.


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