<div style="font-family: arial, helvetica, sans-serif; font-size: 10pt">LGTM<br><br><div class="gmail_quote">On Sat, Nov 3, 2012 at 12:06 AM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Fri, Nov 2, 2012 at 6:06 AM, Kostya Serebryany <<a href="mailto:kcc@google.com">kcc@google.com</a>> wrote:<br>

> Assuming the implemented flag naming is ok with everyone, the patch is good.<br>
> Few questions:<br>
><br>
> +  unsigned Kind;<br>
>    Do we only allow 32 sanitizers? Why not 64?<br>
<br>
</div>We currently have only 13. I don't think we'll forget to update this<br>
if we go past 32 sanitizers: we won't pass the relevant flag on to<br>
-cc1 (so sanitizers past the first 32 will not work at all and tests<br>
will fail), and we will also get a compiler diagnostic in self-host,<br>
at least; we'll probably also get one from g++.<br></blockquote><div><br></div><div>Ok! </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
> Do we want to change -faddress-sanitizer in asan/tsan lit tests?<br>
>   projects/compiler-rt/lib/asan/lit_tests<br>
>   projects/compiler-rt/lib/tsan/lit_tests<br>
><br>
> How long do we keep the aliases (-faddress-sanitizer => fsanitizer=address)?<br>
> (I like the old name more than the new name, but having two names is<br>
> confusing)<br>
<br>
</div>Just so we have something specific to talk about, here are some options:<br>
<br>
1) For the 3.2 release, we remove the existing switches.<br>
2) For the 3.2 release, we keep the existing switches, and produce a<br>
deprecation warning if they are used. At some point between 3.2 and<br>
3.3, once you think most of your users have transitioned, we remove<br>
the aliases.<br>
3) We keep the compatibility switches for the time being (and thus<br>
probably forever).<br>
<br>
I think my preference is (2).<br></blockquote><div><br></div><div>I don't have a strong opinion here, both (2) and (3) sound reasonable. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div class="im"><br>
> Will this allow to easily add *san-specific subflags?<br>
> E.g. --sanitizer=address,global-init-order,use-after-return,use-after-scope<br>
<br>
</div>Yes, this makes perfect sense to me. Additionally, you could break up<br>
'address' into subflags too, and make 'address' be an alias for them<br>
all. The intent is for -fsanitizer= flags to act essentially like -W<br>
flags in this regard.<br>
<br>
> or --sanitizer=memory,memory-origins<br>
<br>
I'm hesitant about this one. Following the warning flags analogy, this<br>
seems analogous to a -fdiagnostic option rather than a -W option.<br></blockquote><div><br></div><div>MemorySanitizer will have two major modes: fast mode (similar to 'valgrind') and slow mode </div><div>(similar to 'valgrind --track-origins=yes'). We'll need to reflect this in the flags syntax. </div>
<div>This does not have to be discussed in this review. </div><div> </div><div>--kcc </div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im HOEnZb"><br>
> (The first one indicates 3 asan sub passes that are curretly off by default,<br>
> the second one indicates that msan should be run with "track origins"<br>
> feature)<br>
<br>
--<br>
</div><span class="HOEnZb"><font color="#888888">Richard<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
> On Fri, Nov 2, 2012 at 10:03 AM, Kostya Serebryany <<a href="mailto:kcc@google.com">kcc@google.com</a>> wrote:<br>
>><br>
>><br>
>><br>
>> On Fri, Nov 2, 2012 at 9:41 AM, Alexander Potapenko <<a href="mailto:glider@google.com">glider@google.com</a>><br>
>> wrote:<br>
>>><br>
>>> I haven't seen the patch yet, but here are two thoughts:<br>
>>> - some tools may be incompatible with each other (e.g. ASan and TSan), so<br>
>>> we shouldn't allow to use them together;<br>
>>> - there are many users of TSan and ASan, thus we can't easily rename the<br>
>>> corresponding command line options. The right thing to do is to make<br>
>>> -fsanitize fully functional, announce that and then remove the existing<br>
>>> flags.<br>
>>> I'm also unsure whether having such an umbrella flag for such different<br>
>>> tools won't confuse the users.<br>
>><br>
>> As Richard says, the old flags remain (they become synonyms of the new<br>
>> flags). That sounds fine.<br>
>> I'll check the patches later today.<br>
>><br>
>> --kcc<br>
>><br>
>>><br>
>>> On Nov 2, 2012 4:51 AM, "Richard Smith" <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
>>>><br>
>>>> Hi,<br>
>>>><br>
>>>> The attached patch series adds a<br>
>>>> -fsanitize=sanitizer1,sanitizer2,sanitizer3 command-line option to<br>
>>>> Clang (and a corresponding -fno-sanitize=...). This works as follows<br>
>>>> for the driver options:<br>
>>>><br>
>>>>   -faddress-sanitizer becomes a synonym for -fsanitize=address<br>
>>>>   -fno-address-sanitizer becomes a synonym for -fno-sanitize=address<br>
>>>>   -fthread-sanitizer becomes a synonym for -fsanitize=thread<br>
>>>>   -fno-thread-sanitizer becomes a synonym for -fno-sanitize=thread<br>
>>>>   -fcatch-undefined-behavior becomes a synonym for -fsanitize=undefined<br>
>>>>   -fsanitize=undefined is expanded by the driver into a list of<br>
>>>> individual undefined behavior sanitizers<br>
>>>><br>
>>>> The -cc1 interface is deliberately very simplistic: it loses support<br>
>>>> for the existing flags, and instead only supports -fsanitize=..., and<br>
>>>> only supports naming the individual checks (and not groups like<br>
>>>> -fsanitize=undefined).<br>
>>>><br>
>>>> Patch 1 just renames the existing ASan and TSan LangOptions to follow<br>
>>>> the naming convention used by the later patches.<br>
>>>> Patch 2 adds support to the driver and frontend for parsing<br>
>>>> -fsanitize, converting the other options to it, and filling in<br>
>>>> LangOptions.<br>
>>>> Patch 3 uses the -fsanitize=... values to enable/disable the existing<br>
>>>> -fcatch-undefined-behavior checks, and removes frontend support for<br>
>>>> the other options.<br>
>>>><br>
>>>> Please review! I'd like to get this in for the 3.2 release.<br>
>>>><br>
>>>> Thanks!<br>
>>>> Richard<br>
>><br>
>><br>
><br>
</div></div></blockquote></div><br></div>