[cfe-commits] [PATCH] Add -fsanitize= command line argument to control -fcatch-undefined-behavior, -faddress-sanitizer, and -fthread-sanitizer

[Kostya Serebryany]

LGTM

On Sat, Nov 3, 2012 at 12:06 AM, Richard Smith <richard at metafoo.co.uk>wrote:

> On Fri, Nov 2, 2012 at 6:06 AM, Kostya Serebryany <kcc at google.com> wrote:
> > Assuming the implemented flag naming is ok with everyone, the patch is
> good.
> > Few questions:
> >
> > +  unsigned Kind;
> >    Do we only allow 32 sanitizers? Why not 64?
>
> We currently have only 13. I don't think we'll forget to update this
> if we go past 32 sanitizers: we won't pass the relevant flag on to
> -cc1 (so sanitizers past the first 32 will not work at all and tests
> will fail), and we will also get a compiler diagnostic in self-host,
> at least; we'll probably also get one from g++.
>

Ok!


>
> > Do we want to change -faddress-sanitizer in asan/tsan lit tests?
> >   projects/compiler-rt/lib/asan/lit_tests
> >   projects/compiler-rt/lib/tsan/lit_tests
> >
> > How long do we keep the aliases (-faddress-sanitizer =>
> fsanitizer=address)?
> > (I like the old name more than the new name, but having two names is
> > confusing)
>
> Just so we have something specific to talk about, here are some options:
>
> 1) For the 3.2 release, we remove the existing switches.
> 2) For the 3.2 release, we keep the existing switches, and produce a
> deprecation warning if they are used. At some point between 3.2 and
> 3.3, once you think most of your users have transitioned, we remove
> the aliases.
> 3) We keep the compatibility switches for the time being (and thus
> probably forever).
>
> I think my preference is (2).
>

I don't have a strong opinion here, both (2) and (3) sound reasonable.


>
> > Will this allow to easily add *san-specific subflags?
> > E.g.
> --sanitizer=address,global-init-order,use-after-return,use-after-scope
>
> Yes, this makes perfect sense to me. Additionally, you could break up
> 'address' into subflags too, and make 'address' be an alias for them
> all. The intent is for -fsanitizer= flags to act essentially like -W
> flags in this regard.
>
> > or --sanitizer=memory,memory-origins
>
> I'm hesitant about this one. Following the warning flags analogy, this
> seems analogous to a -fdiagnostic option rather than a -W option.
>

MemorySanitizer will have two major modes: fast mode (similar to
'valgrind') and slow mode
(similar to 'valgrind --track-origins=yes'). We'll need to reflect this in
the flags syntax.
This does not have to be discussed in this review.

--kcc


> > (The first one indicates 3 asan sub passes that are curretly off by
> default,
> > the second one indicates that msan should be run with "track origins"
> > feature)
>
> --
> Richard
>
> > On Fri, Nov 2, 2012 at 10:03 AM, Kostya Serebryany <kcc at google.com>
> wrote:
> >>
> >>
> >>
> >> On Fri, Nov 2, 2012 at 9:41 AM, Alexander Potapenko <glider at google.com>
> >> wrote:
> >>>
> >>> I haven't seen the patch yet, but here are two thoughts:
> >>> - some tools may be incompatible with each other (e.g. ASan and TSan),
> so
> >>> we shouldn't allow to use them together;
> >>> - there are many users of TSan and ASan, thus we can't easily rename
> the
> >>> corresponding command line options. The right thing to do is to make
> >>> -fsanitize fully functional, announce that and then remove the existing
> >>> flags.
> >>> I'm also unsure whether having such an umbrella flag for such different
> >>> tools won't confuse the users.
> >>
> >> As Richard says, the old flags remain (they become synonyms of the new
> >> flags). That sounds fine.
> >> I'll check the patches later today.
> >>
> >> --kcc
> >>
> >>>
> >>> On Nov 2, 2012 4:51 AM, "Richard Smith" <richard at metafoo.co.uk> wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> The attached patch series adds a
> >>>> -fsanitize=sanitizer1,sanitizer2,sanitizer3 command-line option to
> >>>> Clang (and a corresponding -fno-sanitize=...). This works as follows
> >>>> for the driver options:
> >>>>
> >>>>   -faddress-sanitizer becomes a synonym for -fsanitize=address
> >>>>   -fno-address-sanitizer becomes a synonym for -fno-sanitize=address
> >>>>   -fthread-sanitizer becomes a synonym for -fsanitize=thread
> >>>>   -fno-thread-sanitizer becomes a synonym for -fno-sanitize=thread
> >>>>   -fcatch-undefined-behavior becomes a synonym for
> -fsanitize=undefined
> >>>>   -fsanitize=undefined is expanded by the driver into a list of
> >>>> individual undefined behavior sanitizers
> >>>>
> >>>> The -cc1 interface is deliberately very simplistic: it loses support
> >>>> for the existing flags, and instead only supports -fsanitize=..., and
> >>>> only supports naming the individual checks (and not groups like
> >>>> -fsanitize=undefined).
> >>>>
> >>>> Patch 1 just renames the existing ASan and TSan LangOptions to follow
> >>>> the naming convention used by the later patches.
> >>>> Patch 2 adds support to the driver and frontend for parsing
> >>>> -fsanitize, converting the other options to it, and filling in
> >>>> LangOptions.
> >>>> Patch 3 uses the -fsanitize=... values to enable/disable the existing
> >>>> -fcatch-undefined-behavior checks, and removes frontend support for
> >>>> the other options.
> >>>>
> >>>> Please review! I'd like to get this in for the 3.2 release.
> >>>>
> >>>> Thanks!
> >>>> Richard
> >>
> >>
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121103/232780ea/attachment.html>