LGTM On Sat, Nov 3, 2012 at 12:06 AM, Richard Smith <[email protected]>wrote:
> On Fri, Nov 2, 2012 at 6:06 AM, Kostya Serebryany <[email protected]> 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 <[email protected]> > wrote: > >> > >> > >> > >> On Fri, Nov 2, 2012 at 9:41 AM, Alexander Potapenko <[email protected]> > >> 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" <[email protected]> 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 > >> > >> > > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
