patch: make ubsan report error summaries

[Richard Smith]

On Wed, Oct 23, 2013 at 9:14 AM, Alexey Samsonov <samsonov at google.com>wrote:

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

Definitely. The thing blocking this is that sanitizer_common's backtrace
functionality doesn't work for a ubsan-instrumented binary, because we
don't have any symbol interposition for ubsan (we have avoided this,
because we didn't want to require instrumentation of the whole program) and
the backtrace functionality currently requires us to instrument process and
thread startup.


>
>> 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
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131023/4f831b29/attachment.html>