patch: sanitizer summaries are redundant, don't emit them

[Nick Lewycky]

On 22 October 2013 22:53, Kostya Serebryany <kcc at google.com> wrote:

> LGTM
>
>
> On Wed, Oct 23, 2013 at 9:25 AM, Nick Lewycky <nlewycky at google.com> wrote:
>
>> On 22 October 2013 22:10, Kostya Serebryany <kcc at google.com> wrote:
>>
>>>
>>> On Wed, Oct 23, 2013 at 8:16 AM, Nick Lewycky <nlewycky at google.com>wrote:
>>>
>>>> __sanitizer_report_error_summary is the replaceable interface for
>>>> reporting an error. This patch changes asan a tiny bit to always emit a
>>>> summary whenever it encounters an error. This is important, otherwise a
>>>> library user of asan will not be aware of errors that were merely printed.
>>>>
>>>> Also, all these summaries are redundant with the non-summarized
>>>> printouts. Make the default weak implementation of report error summary not
>>>> do anything. This means we need to remove the check for SUMMARY lines from
>>>> the tests. This is suboptimal for testing, but printing out these redundant
>>>> lines is just bad UI.
>>>>
>>>
>>> But I've added summaries for a reason: they help to cluster bug reports.
>>>
>>
>> But asan prints one error and exits?
>>
>
> Correct. Summaries help cluster reports from different runs.
>
>
>>
>>
>>> I'd really prefer to keep them by default.
>>>
>>
>> I've attached an updated patch that puts the summaries back. What's left
>> in the patch will fix a bug where a subprocess may catch an asan error and
>> die, but that error would be lost by the parent process.
>>
>
> This happens only if the subprocess does not have a symbolizer, right?
>

Right.

This patch will make asan print summaries with abrakadabra if there is not
> symbolizer.
> Could be totally fine though.
>

With no symbolizer, the results look like:

SUMMARY: AddressSanitizer: stack-buffer-overflow ??:0 ??

which is pretty lousy, but does contain *some* information. We can
certainly change what the summary says in the case where there is no

  The only way to make sure we catch errors in all subprocesses is to make
>> sure they call into sanitizer_report_error_summary if there's any error.
>>
>> And we need ubsan to do this for the same reason. UBSan already has
>> one-line errors. We want to print those, preferably without "SUMMARY:"
>> before them.
>>
>
> [in another patch?] we can move printing SUMMARY: from
> __sanitizer_report_error_summary to ReportErrorSummary
> and add an additional "const char *prefix" parameter to
> ReportErrorSummary. asan/tsan/msan will call it with prefix="SUMMARY: "
>

Okay, that works. But there's one other difference Richard brought up:
ReportErrorSummary expects complete error text, while printing is assumed
to be continually appending. UBSan manages to print out each piece of the
error individually and never allocates any buffers.

I could change UBSan's Printf calls to instead keep appending to an
internal buffer and then feed the buffer to
__sanitizer_report_error_summary when we're done. I just wanted to point
out the difference in these APIs, in case you can think of a way we could
do this without require a buffer somewhere.

btw, why not use phabricator for reviews :) ?
>

I have to search for instructions every time I want to use it. When it
works it's fine, but I had a fit reviewing D1960 where double-clicking the
line number wouldn't let me enter comments, even though I was logged in as
the reviewer. Absolutely maddening.

Anyhow, I'll put the next patch in phab.

Nick
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131022/34468312/attachment.html>