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

[Alexey Samsonov]

On Sat, Dec 15, 2012 at 4:48 AM, Will Dietz <willdtz at gmail.com> wrote:

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

TSan inserts a call to runtime library for each function entry/exit and
for each memory operation (load/store). Still, data races don't happen that
often, so we have to access hashtable which stores stack traces of
printed reports on slow path only. We haven't observed any performance
problems here (Dmitry may correct me if I'm wrong).

I didn't extensively test ubsan on real-world applications so it's hard for
me
to estimate the number of error reports it prints. But I think we need to
count
not the number of calls to __ubsan_handle (i.e. number of places in code
where
an error _might_ happen), but the number of actual unique reports printed
by ubsan.
If, say, it's at most 10-20, then storing PCs of all the erroneous
instructions and doing
a linear scan before printing another report might be better than bloating
the binary size by 1%.

That said, I think that the de-duplication functionality should definitely
be implemented one
way or another, and it should be "on" by default (and I can't imagine a
reason why a user
may decide to turn it off).



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



-- 
Alexey Samsonov, MSK
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20121217/79714e7f/attachment.html>