[PATCH] D13452: [asan] On OS X, log reports to syslog and os_trace (version 2)

[Anna Zaks via llvm-commits]

zaks.anna added inline comments.

================
Comment at: lib/sanitizer_common/sanitizer_common.h:659
@@ +658,3 @@
+  SANITIZER_TYPE_UNKNOWN
+};
+
----------------
eugenis wrote:
> We already have knowledge of the current sanitizer: SanitizerToolName. Could you either dispatch on that, or use it directly?
One of the APIs we use only allows string literals (for privacy reasons).

Dispatching on the string literal instead of the enum feels bad. 

Also, how does this global SanitizerToolName works when we have more than one sanitizer on at the same time? I think UBSan and ASan are not mutually exclusive. It might not matter which tool failed in some cases; however, when a tool reports an error, differentiating between the two might be useful. One way of doing that would be to have the error reporting APIs take the tool name as a parameter.

In most cases, SanitizerToolName is used in calls to Report() when reporting an internal error. It would be useful to log the internal errors to syslog on OS X as well (at least that should be the long term goal).

Any further opinions on this?


================
Comment at: lib/sanitizer_common/sanitizer_common_libcdep.cc:15
@@ -14,2 +14,3 @@
 #include "sanitizer_common.h"
+#include "sanitizer_allocator_internal.h"
 #include "sanitizer_flags.h"
----------------
glider wrote:
> Please keep the includes sorted alphabetically.
They are sorted. sanitizer-common.h is "the module header" for this cc file.

================
Comment at: lib/sanitizer_common/sanitizer_mac.cc:408
@@ +407,3 @@
+  // for GCD to dispatch a new thread, the process will deadlock, because the
+  // pthread_create wrapper needs to acquire the lock as well.
+  if (common_flags()->log_to_syslog)
----------------
filcab wrote:
> Can we verify any/some of these things (e.g: someone was holding the mutex and called us) with some CHECK() calls?
It would be good to check if there is a lock on threadRegestry; however, I don't know if we can access that from a utility in  sanitizer_common without passing it in explicitly.

================
Comment at: test/asan/lit.cfg:39
@@ -37,2 +38,3 @@
   default_asan_opts = 'abort_on_error=0'
+  default_asan_opts += ':log_to_syslog=0'
 if default_asan_opts:
----------------
eugenis wrote:
> This way log_to_syslog is completely untested. How bad is it? Is there a way to tune this down, but still exercise the logging code?
> 
The code will get executed by abort_on_error.cc test that runs with the default options.


http://reviews.llvm.org/D13452