[cfe-dev] [ubsan] Add -fsanitize-warn-once, only emit runtime error once per check

[Will Dietz]

On Fri, Dec 14, 2012 at 1:53 AM, Alexey Samsonov <samsonov at google.com> wrote:
>
>
> On Thu, Dec 13, 2012 at 6:26 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
>>
>> Have you measured the code size overhead from this extra flag? Did you
>> consider implementing this in the runtime library instead (by
>> suppressing duplicates based on return address or SourceLocation)?
>

Hmm, good point.  I hadn't previously measured this, but I just did
now, see attached.  Sizes are reported for text/data/bss/total as
reported by 'size', and ubsan calls are counted by occurrences of
calls in the actual resulting binary (grep'ing output from objdump
-d).

Summary: average increase across CINT2006 was 1.85% using
-fsanitize=integer with a neatly consistent ~5bytes per check
(including text).

For the ability to scale despite many sites triggering many times,
this does seem like a valid trade-off.

Previously IOC used a short linear scan table (~10-20 elements was
sweet spot IIRC) with fallback to a larger hashtable to manage
duplicates, but that was always a performance issue.  As a useful data
point, a quick spot-check of 403.gcc shows 96 static locations
triggered a total of 3,476,066 times dynamically when just processing
one of the inputs used for the 'ref' input set (166.i).  More on this
below.

>
> +1. ThreadSanitizer has to solve the same problem - we want to report
> each data race (pair of stack traces) exactly once. TSan runtime stores the
> stacks of printed reports (as a sequence of PCs) to do this de-duplication.
>

Great, didn't realize TSan already solved this problem.  That said,
the problem is somewhat different I think:

* TSan supports differentiating based on stack trace (apparently), but
that seems less interesting for ubsan/integer checks, especially since
we don't print that information :).  The byte-per-check approach
doesn't work for stack traces, so that's not really an option for tsan
as-is.
* I would (perhaps erroneously) expect tsan to have many fewer dynamic
invocations than ubsan/integer checks, which might suggest difference
trade-offs in the size vs performance department.  Checking a byte 1
million times vs scanning and managing a vector of ~100 items > 1
million times might make the size increase rather preferable, even if
that's not the right decision for tsan (I have no idea what's right
for tsan :)).

Given this, and in light of the attached data, do you buy that this is
indeed the appropriate approach for ubsan/integer checking?

Until I actually gathered this data I wasn't sure, but the 1-2% seems
very much worth it IMHO.

Thoughts, and thanks for helping ensure we do what's right!

~Will

>>
>> On Thu, Dec 13, 2012 at 11:51 AM, Will Dietz <willdtz at gmail.com> wrote:
>> > On Thu, Dec 13, 2012 at 12:05 PM, Dmitri Gribenko <gribozavr at gmail.com>
>> > wrote:
>> >> On Thu, Dec 13, 2012 at 7:33 PM, Will Dietz <willdtz at gmail.com> wrote:
>> >>> This flag causes clang to emit a byte for each check that is used by
>> >>> the
>> >>> runtime to track whether we've already printed an error for that
>> >>> check.
>> >>>
>> >>>  Often failed checks are triggered many times dynamically, but a user
>> >>>  is only interested in which checks failed (with example dynamic
>> >>> values
>> >>>  to aid in debugging).  This flag lets the user make such runs much
>> >>>  more efficient and generate more manageable output.
>> >>
>> >> Hi Will,
>> >>
>> >> +  if (Checked) {
>> >> +    if (*Checked) return;
>> >> +    *Checked = true;
>> >> +  }
>> >>
>> >> Does it make sense to do the store atomically?  The user's program is
>> >> already buggy, but introducing a possible data race is unfortunate.
>> >>
>> >> Dmitri
>> >>
>> >
>> > Hi Dmitri,
>> >
>> > Glad you brought this up.  I wasn't sure which way to go on this and
>> > erred on simplicity.  Attached is an updated compiler-rt patch using
>> > __sync_val_compare_and_swap, which also simplifies the code a bit.  If
>> > this builtin is sufficiently portable (architectures and compiler
>> > recognition) then I would prefer this for the reasons you mention.
>> >
>> > Thanks!
>> >
>> > _______________________________________________
>> > cfe-dev mailing list
>> > cfe-dev at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>> >
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>
>
>
>
> --
> Alexey Samsonov, MSK
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: size.pdf
Type: application/pdf
Size: 46828 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20121214/661209b7/attachment.pdf>