patch: make ubsan report error summaries

[Nick Lewycky]

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.

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.

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131023/3996505f/attachment.html>