[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:
> zaks.anna wrote:
> > 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?
> > 
> I don't like 2 things about this: duplication between SanitizerType and SanitizerToolName, and the fact the SanitizerType list is incomplete and looks arbitrary.
> 
> Maybe extend the list to include, at least, all possible values of SanitizerToolName, and define a global instance of that to replace SanitizerToolName? With a static list of pretty tool names indexed by SanitizerType.
> 
> >> Also, how does this global SanitizerToolName works when we have more than one sanitizer on at the same time?
> 
> There is always a "main sanitizer" which provides the name to be used in reports. In the case of asan+ubsan the main sanitizer is asan. We never really saw this as a problem. After all, the report contains a type string like "use-after-free" which clearly tells the user what it is about.
> 
I agree about the duplication.

> Maybe extend the list to include, at least, all possible values of SanitizerToolName, and define a global instance of that to 
> replace SanitizerToolName? With a static list of pretty tool names indexed by SanitizerType.

In the solution you propose above, we would also replace uses of SanitizerToolName by a call to a function that dispatches on the enum and returns the name. This function would be implemented in sanitizer_common and would have to know about all sanitizers, correct?

I can do this as a separate patch.

> After all, the report contains a type string like "use-after-free" which clearly tells the user what it is about.
I do no think it is true of every user and every error message. Also, this is not friendly to (hypothetical but likely) post processing scripts that check the syslog trying to detect and categorize the failures.

Another issue that is Mac-specific is that the cash report might only have the messages from os_trace, which will not include the rest of the message. Calling out which tool failed there is valuable.


http://reviews.llvm.org/D13452