[PATCH] Overhaul the symbolizer interface.

[Peter Collingbourne]

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer.cc:39
@@ +38,3 @@
+SymbolizerInterface *DisableSymbolizer() {
+  DCHECK_EQ(0, atomic_load(&symbolizer, memory_order_acquire));
+  SymbolizerInterface *dummy_sym =
----------------
Alexey Samsonov wrote:
> This is not thread-safe - if DisableSymbolizer() is called from two threads simultaneously you may create two SymbolizerInterface objects. We use ugly double-checked locking as in current implementation of getSymbolizer() in sanitizer_symbolizer_posix_libcdep.cc
DisableSymbolizer does not need to be thread safe, right?  It's only called from sanitizer initializers which need to be thread safe anyway.  The double checked locking has been moved to GetOrInitSymbolizer which I believe does (well, modulo any decision about the overall thread safety of the Symbolizer interface, I guess). I've added comments in sanitizer_symbolizer.h calling out the non-thread-safe functions.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_internal.h:21
@@ +20,3 @@
+
+extern atomic_uintptr_t symbolizer;
+
----------------
Alexey Samsonov wrote:
> I'd like to avoid extra headers if possible. Suggestion: move all the factory methods (GetSymbolizer, GetSymbolizerOrNull, DisableSymbolizer etc.) into a SymbolizerFactory class. Then you can make PlatformInitSymbolizer() a private method, and make atomic_uintptr_t symbolizer a static private member (it would also clarify that Symbolizer is in fact a singleton).
Sorry, but putting the *Symbolizer functions in a factory class is a little too verbose for my tastes. I'd prefer to keep these here.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer.cc:32
@@ +31,3 @@
+  SymbolizerInterface *sym = GetSymbolizerOrNull();
+  DCHECK(sym);
+  return sym;
----------------
Alexey Samsonov wrote:
> Please don't use DCHECKs - we don't generally build our runtimes in debug mode (I believe COMPILER_RT_DEBUG you're adding should be off by default even in Debug builds). DCHECK's are TSan legacy and are expanded to asserts only if TSAN_DEBUG is defined. CHECK's will be fine here.
Done.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_libcdep.cc:24
@@ +23,3 @@
+CreateAndStoreSymbolizer(const char *path_to_external) {
+  if (!path_to_external || path_to_external[0] == '\0') {
+    path_to_external = GetEnv("LLVM_SYMBOLIZER_PATH");
----------------
Alexey Samsonov wrote:
> This path lookup shouldn't be run on Windows (it *is* a platform-specific code as well).
Done.  (Although I suspect we will need similar logic to run the symbolizer in a separate address space on Windows, let's defer that decision until we get there.)

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_libcdep.cc:25
@@ +24,3 @@
+  if (!path_to_external || path_to_external[0] == '\0') {
+    path_to_external = GetEnv("LLVM_SYMBOLIZER_PATH");
+    if (!path_to_external)
----------------
Alexey Samsonov wrote:
> Why do you need this? Why PATH can't be used for your purposes?
I guess I don't.  Removed.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_libcdep.cc:32
@@ +31,3 @@
+      PlatformInitSymbolizer(path_to_external);
+  if (!platform_symbolizer) {
+    return DisableSymbolizer();
----------------
Alexey Samsonov wrote:
> no need in {} after if (here and below)
Removed.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:377
@@ -374,2 +376,3 @@
   uptr SymbolizeCode(uptr addr, AddressInfo *frames, uptr max_frames) {
+    SpinMutexLock l(&mu_);
     if (max_frames == 0)
----------------
Alexey Samsonov wrote:
> Why do you need thread-safe symbolizer? Sanitizer tools try to not call symbolizer from several threads simultaneously (maybe that contract is broken somewhere, though). In fact, I would prefer to CHECK-fail if symbolizer is used from different threads. The reason for that is: symbolizer is used when we either report an error or print some other stack trace. It is obviously bad to print reports/stack traces from two threads simultaneously - they will mix up. So, we guard report printing in each sanitizer tool with a mutex (I think there was even a common mutex to prevent mixing ASan/UBSan reports).
I guess in theory each report could be buffered and written to the output device atomically in order to prevent interleaving. At any rate, UBSan does use a mutex in its report printer, but it does not protect the symbolizer call in getFunctionLocation.  In my opinion, calls to the symbolizer should be rare enough that it's not worth trying to optimize for them by avoiding double locking.

================
Comment at: lib/asan/asan_rtl.cc:540
@@ +539,3 @@
+  if (common_flags()->symbolize) {
+    InitSymbolizer(common_flags()->external_symbolizer_path);
+  } else {
----------------
Alexey Samsonov wrote:
> In fact, there is a reason for splitting Symbolizer initialization and starting external symbolizer into separate steps - the latter step is not necessary if we have internal symbolizer. If you're merging this into one function (which does make sense), please make starting external symbolizer (including path lookup) optional.
Done.

================
Comment at: lib/msan/msan.cc:333
@@ -332,4 +332,3 @@
   const char *external_symbolizer = common_flags()->external_symbolizer_path;
-  bool symbolizer_started =
-      getSymbolizer()->InitializeExternal(external_symbolizer);
+  bool symbolizer_started = InitSymbolizer(external_symbolizer)->IsAvailable();
   if (external_symbolizer && external_symbolizer[0]) {
----------------
Alexey Samsonov wrote:
> See comment above - if a user explicitly provided external_symbolizer_path flag, we should check
> that external symbolizer was in fact initialized, not that any kind of symbolization is available (same holds for TSan).
Done.


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