<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Apr 22, 2015 at 12:37 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">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 right design here -- it doesn't seem to scale very well. Have you considered an attribute like<br>
>> > ><br>
>> > > __attribute__((no_sanitize("list,of,sanitizers"))) void fn() { ... }<br>
>> > ><br>
>> > ><br>
>> > > where the list is parsed as if it were specified as `-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 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 attribute like this, and later deprecate no_sanitize_address, no_sanitize_thread and no_sanitize_memory.<br>
><br>
><br>
> I can put together another patch in the next few days, unless someone else (more experienced) wants to take this?<br>
<br>
</div></div>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).</blockquote><div><br></div><div>Not really. There are two kinds of arguments to -fsanitize= options:</div><div>1) inidividual sanitizers ("address", "thread", "vptr", etc.)</div><div>2) sets of sanitizers ("undefined", "cfi", etc.).</div><div><br></div><div>We can introduce a single no_sanitize("list", "of", "sanitizers") that would be flexible</div><div>enough for your use cases:</div><div>[[clang::no_sanitize("vptr", "shift")]] // disable just "vptr" and "shift" checks in the function</div><div>[[clang::no_sanitize("undefined")]] // disable all checks introduced by UBSan (-fsanitize=undefined)</div><div>[[clang::no_sanitize("undefined", "address", "memory")]] // Disable UBSan, MSan, ASan instrumentation (but use TSan, if the code will be compiled with -fsanitize=thread).</div><div><br></div><div>Then no_sanitize_address will be just a synonym for no_sanitize("address"), etc.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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>
<span class=""><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>
</span>> _______________________________________________<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>
</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>