<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Apr 22, 2015 at 1:55 PM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5">On Wed, Apr 22, 2015 at 4:12 PM, Alexey Samsonov <<a href="mailto:vonosmas@gmail.com">vonosmas@gmail.com</a>> wrote:<br>
><br>
><br>
> On Wed, Apr 22, 2015 at 12:37 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
> wrote:<br>
>><br>
>> On Mon, Apr 20, 2015 at 9:06 PM, Oliver Chang <<a href="mailto:ochang@google.com">ochang@google.com</a>> wrote:<br>
>> > In <a href="http://reviews.llvm.org/D9059#158733" target="_blank">http://reviews.llvm.org/D9059#158733</a>, @samsonov wrote:<br>
>> ><br>
>> >> In <a href="http://reviews.llvm.org/D9059#157697" target="_blank">http://reviews.llvm.org/D9059#157697</a>, @ochang wrote:<br>
>> >><br>
>> >> > In <a href="http://reviews.llvm.org/D9059#157331" target="_blank">http://reviews.llvm.org/D9059#157331</a>, @rsmith wrote:<br>
>> >> ><br>
>> >> > > I'm not convinced that adding one attribute per sanitizer is the<br>
>> >> > > right design here -- it doesn't seem to scale very well. Have you considered<br>
>> >> > > an attribute like<br>
>> >> > ><br>
>> >> > >   __attribute__((no_sanitize("list,of,sanitizers"))) void fn() {<br>
>> >> > > ... }<br>
>> >> > ><br>
>> >> > ><br>
>> >> > > where the list is parsed as if it were specified as<br>
>> >> > > `-fno-sanitize=list,of,sanitizers` on the command line?<br>
>> >> ><br>
>> >> ><br>
>> >> > This does seem like a much better way of doing this. Should I change<br>
>> >> > this in this patch?<br>
>> >> ><br>
>> >> > What does everyone else think?<br>
>> >><br>
>> >><br>
>> >> I agree with this suggestion. It would be cool to have a single<br>
>> >> attribute like this, and later deprecate no_sanitize_address,<br>
>> >> no_sanitize_thread and no_sanitize_memory.<br>
>> ><br>
>> ><br>
>> > I can put together another patch in the next few days, unless someone<br>
>> > else (more experienced) wants to take this?<br>
>><br>
>> I'd like to make sure I'm on the same page for the design. I was under<br>
>> the impression that your original patch was attempting to suppress a<br>
>> feature of a sanitizer, not an entire sanitizer itself. I think the<br>
>> suggestions have been that we should have a single attribute which<br>
>> allows you to turn off individual ubsan sanitizer features, and<br>
>> another attribute (someday) to turn of entire sanitizers (which<br>
>> replaces no_sanitize_thread, etc).<br>
><br>
><br>
> Not really. There are two kinds of arguments to -fsanitize= options:<br>
> 1) inidividual sanitizers ("address", "thread", "vptr", etc.)<br>
> 2) sets of sanitizers ("undefined", "cfi", etc.).<br>
><br>
> We can introduce a single no_sanitize("list", "of", "sanitizers") that would<br>
> be flexible<br>
> enough for your use cases:<br>
> [[clang::no_sanitize("vptr", "shift")]]  // disable just "vptr" and "shift"<br>
> checks in the function<br>
> [[clang::no_sanitize("undefined")]]  // disable all checks introduced by<br>
> UBSan (-fsanitize=undefined)<br>
> [[clang::no_sanitize("undefined", "address", "memory")]]  // Disable UBSan,<br>
> MSan, ASan instrumentation (but use TSan, if the code will be compiled with<br>
> -fsanitize=thread).<br>
><br>
> Then no_sanitize_address will be just a synonym for no_sanitize("address"),<br>
> etc.<br>
<br>
</div></div>To me, using the existing attributes makes more sense. For starters,<br>
they're self-documenting as to what sanitizer is being affected. This<br>
means we don't have to be as careful about adding selective features<br>
to the sanitizers (the namespace becomes the sanitizer, and not all<br>
sanitizers).<br></blockquote><div><br></div><div>The sanitizers are structured in a different way. From Clang point of view, it's not the case that there are large</div><div>"sanitizers", each of them having a set of "features". Instead, we have small</div><div>"sanitizers" and "groups of sanitizers". They are completely defined by the file</div><div>include/clang/Basic/Sanitizers.def. You may say that we have "flat namespace".</div><div><br></div><div>-fsanitize=cfi is essentially a short way of writing of -fsanitize=cfi-cast-strict,cfi-derived-cast,cfi-unrelated-cast,cfi-nvcall,cfi-vcall</div><div><br></div><div>As I understand Richard's proposal, it's a new attribute, which takes a list of strings</div><div>as its arguments, and has the same semantics as -fsanitize= flag.</div><div><br></div><div>ASan, for instance, is a single sanitizer (once again, from Clang's driver/codegen point of view). Now, it</div><div>actually has some experimental functionality, but it's protected by runtime flags, which is a very different story.</div><div><br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Also, how much do we have to worry about feature names across<br>
sanitizers? e.g., asan and msan both share a feature that does<br>
slightly different things under the same name? </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
For background, eventually I want to move the sanitizer attributes out<br>
of clang entirely (by having a plugin system for attributes), and have<br>
them under the control of the individual sanitizers. Having all<br>
sanitizers listed together makes this goal considerably more<br>
challenging. By having each sanitizer segregate its own list of<br>
features, it makes a more clear layering.<br>
<span class=""><font color="#888888"><br>
~Aaron<br>
</font></span><div class=""><div class="h5"><br>
><br>
>> I'm wondering if we can do<br>
>> something like this:<br>
>><br>
>> 1) Add a new no_sanitize_undefined attribute that turns off all ubsan<br>
>> checks for a method, and,<br>
>> 2) Have the attribute take an optional, variadic list of sanitizer<br>
>> feature strings to fine-tune what gets turned off.<br>
>> 3) Someday, if needed, add the optional list functionality to the<br>
>> no_sanitize_address, no_sanitize_memory, and no_sanitize_thread<br>
>> attributes.<br>
>> 4) Someday add [[clang::no_sanitize("list", "of", "sanitizers")]] for<br>
>> turning off entire sanitizers.<br>
>><br>
>> This allows you to do:<br>
>><br>
>> [[clang::no_sanitize_undefined("vptr", "something_else")]] void f() {}<br>
>> // Turns off just vptr and something_else ubsan instrumentation<br>
>> [[clang::no_sanitize_undefined]] void g() {} // Turns off all ubsan<br>
>> instrumentation for this function<br>
>><br>
>> // Someday<br>
>> [[clang::no_sanitize("ubsan", "asan")]] void h() {} // Turns off all<br>
>> ubsan and asan instrumentation, but allows tsan and msan.<br>
>><br>
>> I think this provides maximal flexibility with minimal worry about<br>
>> name clashes between sanitizer features. It comes with the handy<br>
>> benefit of not needing to deprecate the no_sanitize_blah attributes,<br>
>> but still scales.<br>
>><br>
>> ~Aaron<br>
>><br>
>> ><br>
>> ><br>
>> > <a href="http://reviews.llvm.org/D9059" target="_blank">http://reviews.llvm.org/D9059</a><br>
>> ><br>
>> > EMAIL PREFERENCES<br>
>> >   <a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
>> ><br>
>> ><br>
>> ><br>
>> > _______________________________________________<br>
>> > cfe-commits mailing list<br>
>> > <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
>> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
><br>
><br>
><br>
><br>
> --<br>
> Alexey Samsonov<br>
> <a href="mailto:vonosmas@gmail.com">vonosmas@gmail.com</a><br>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature"><div dir="ltr">Alexey Samsonov<br><a href="mailto:vonosmas@gmail.com" target="_blank">vonosmas@gmail.com</a></div></div>
</div></div>