[PATCH] Overhaul the symbolizer interface.

[Alexey Samsonov]

  Thanks, for working on this!


================
Comment at: lib/asan/asan_rtl.cc:540
@@ +539,3 @@
+  if (common_flags()->symbolize) {
+    InitSymbolizer(common_flags()->external_symbolizer_path);
+  } else {
----------------
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.

================
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]) {
----------------
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).

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_internal.h:21
@@ +20,3 @@
+
+extern atomic_uintptr_t symbolizer;
+
----------------
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).

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer.cc:32
@@ +31,3 @@
+  SymbolizerInterface *sym = GetSymbolizerOrNull();
+  DCHECK(sym);
+  return sym;
----------------
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.

================
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 =
----------------
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

================
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)
----------------
Why do you need this? Why PATH can't be used for your purposes?

================
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");
----------------
This path lookup shouldn't be run on Windows (it *is* a platform-specific code as well).

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

================
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)
----------------
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).


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