patch: make ubsan report error summaries

[Alexey Samsonov]

On Wed, Oct 23, 2013 at 2:21 PM, Nick Lewycky <nlewycky at google.com> wrote:

> On 23 October 2013 03:05, Alexey Samsonov <samsonov at google.com> wrote:
>
>> Hi!
>>
>> +  if (Level == DL_Error) {
>> +    InternalScopedBuffer<char> summary(1024);
>> +    internal_snprintf(summary.data(), summary.size(),
>> +                      "UndefinedBehaviorSanitizer: %s", Flag);
>> +    switch (Loc.getKind()) {
>> +      case Location::LK_Null:
>> +        break;
>> +      case Location::LK_Source: {
>> +        SourceLocation SLoc = Loc.getSourceLocation();
>> +        internal_snprintf(summary.data(), summary.size(), "%s %s:%u:%u",
>> +                          summary.data(),
>> +                          SLoc.getFilename(), SLoc.getLine(),
>> SLoc.getColumn());
>> +        break;
>> +      }
>> +      case Location::LK_Module: {
>> +        ModuleLocation MLoc = Loc.getModuleLocation();
>> +        internal_snprintf(
>> +            summary.data(), summary.size(), "%s (%s+0x%zx)",
>> summary.data(),
>> +            StripPathPrefix(MLoc.getModuleName(),
>> +
>>  __sanitizer::common_flags()->strip_path_prefix),
>> +            MLoc.getOffset());
>> +        break;
>>  +      }
>> +      case Location::LK_Memory:
>> +        internal_snprintf(summary.data(), summary.size(),
>> +                          " %p", summary.data(),
>> Loc.getMemoryLocation());
>> +        break;
>> +    }
>> +    __sanitizer_report_error_summary(summary.data());
>> +  }
>>
>> This code looks too similar to renderLocation() in ubsan_diag.cc.
>> Probably they both
>> should use PrintSourceLocation() and PrintModuleAndOffset() from
>> sanitizer_common.cc, and the
>> latter two functions should be generalized to work with a buffer instead
>> of simply printing the error message.
>>
>
> It's not just renderLocation, the code in this if-statement is a second
> copy of the function up to before the renderMemorySnippet call.
>
> Richard doesn't want this code to use any buffer at all (and indeed, up
> until this patch ubsan doesn't). Is there any way we can preserve that?
> Making PrintSourceLocation and PrintModuleAndOffset use intermediate
> buffers is going in the wrong direction -- it'd be best if we could avoid
> needing a buffer, because when this code runs the program is failing and we
> might not be able to get a buffer.
>

It's a pretty strict limitation, this makes error reporting a lot harder.
Usually we think that we can allocate (or at least mmap) a buffer. In fact,
if we're using an internal symbolizer, it does all sorts of complex system
calls, memory (de)allocation etc, which happen during the report printing.
By the way, I've investigated a number of UBSan failures today and think
that we should really move toward better error reports in UBSan - at least
we should print the stack traces when error happens (if
-fno-sanitize-recover is provided).


> Could we break up __sanitizer_report_error_summary into two functions, one
> that takes text as a stream, and a second one which flushes operation? I
> think the only problem is that it wouldn't be thread safe.
>

I think CommonSanitizerReportMutex should protect us here - we use it to
make sure two reports (from different or same sanitizer) are not intermixed.


>
> Nick
>
> On Wed, Oct 23, 2013 at 1:21 PM, Nick Lewycky <nlewycky at google.com> wrote:
>>
>>> On 22 October 2013 22:07, Nick Lewycky <nlewycky at google.com> wrote:
>>>
>>>> On 22 October 2013 21:18, Nick Lewycky <nlewycky at google.com> wrote:
>>>>
>>>>> The attached patch makes ubsan emit summaries of errors it encounters.
>>>>> The format of these summaries is:
>>>>>   UndefinedBehaviourSanitizer: signed-integer-overflow file:49:7
>>>>> where the string is the flag name. Most of the patch is adding the
>>>>> flag names to all the reports all over.
>>>>>
>>>>
>>>> I've noticed a small bug, for load-invalid-value we always pick "enum"
>>>> and never "bool". I would guess that's because
>>>> ASTContext::getTypeSize(BoolTy) returns 8 instead of 1?
>>>>
>>>> Richard, thoughts?
>>>>
>>>
>>> Updated patch attached. It now detects bool sanitizer by looking at the
>>> Type as a string, and is otherwise updated for the changes in
>>> sanitizer-common.
>>>
>>> Nick
>>>
>>>
>>>>
>>>> Nick
>>>>
>>>>  This patch is stacked on top of
>>>>> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20131021/091535.html ,
>>>>> or else ubsan's tests will fail.
>>>>>
>>>>> Please review!
>>>>>
>>>>> Nick
>>>>>
>>>>
>>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>>
>>
>>
>> --
>> Alexey Samsonov, MSK
>>
>
>


-- 
Alexey Samsonov, MSK
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131023/4000967e/attachment.html>