[PATCH] Add a no_sanitize_vptr function attribute.

[Aaron Ballman]

On Wed, Apr 22, 2015 at 5:28 PM, Alexey Samsonov <vonosmas at gmail.com> wrote:
>
> On Wed, Apr 22, 2015 at 1:55 PM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> On Wed, Apr 22, 2015 at 4:12 PM, Alexey Samsonov <vonosmas at gmail.com>
>> wrote:
>> >
>> >
>> > On Wed, Apr 22, 2015 at 12:37 PM, Aaron Ballman <aaron at aaronballman.com>
>> > wrote:
>> >>
>> >> On Mon, Apr 20, 2015 at 9:06 PM, Oliver Chang <ochang at google.com>
>> >> wrote:
>> >> > In http://reviews.llvm.org/D9059#158733, @samsonov wrote:
>> >> >
>> >> >> In http://reviews.llvm.org/D9059#157697, @ochang wrote:
>> >> >>
>> >> >> > In http://reviews.llvm.org/D9059#157331, @rsmith wrote:
>> >> >> >
>> >> >> > > 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
>> >> >> > >
>> >> >> > >   __attribute__((no_sanitize("list,of,sanitizers"))) void fn() {
>> >> >> > > ... }
>> >> >> > >
>> >> >> > >
>> >> >> > > where the list is parsed as if it were specified as
>> >> >> > > `-fno-sanitize=list,of,sanitizers` on the command line?
>> >> >> >
>> >> >> >
>> >> >> > This does seem like a much better way of doing this. Should I
>> >> >> > change
>> >> >> > this in this patch?
>> >> >> >
>> >> >> > What does everyone else think?
>> >> >>
>> >> >>
>> >> >> 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.
>> >> >
>> >> >
>> >> > I can put together another patch in the next few days, unless someone
>> >> > else (more experienced) wants to take this?
>> >>
>> >> I'd like to make sure I'm on the same page for the design. I was under
>> >> the impression that your original patch was attempting to suppress a
>> >> feature of a sanitizer, not an entire sanitizer itself. I think the
>> >> suggestions have been that we should have a single attribute which
>> >> allows you to turn off individual ubsan sanitizer features, and
>> >> another attribute (someday) to turn of entire sanitizers (which
>> >> replaces no_sanitize_thread, etc).
>> >
>> >
>> > Not really. There are two kinds of arguments to -fsanitize= options:
>> > 1) inidividual sanitizers ("address", "thread", "vptr", etc.)
>> > 2) sets of sanitizers ("undefined", "cfi", etc.).
>> >
>> > We can introduce a single no_sanitize("list", "of", "sanitizers") that
>> > would
>> > be flexible
>> > enough for your use cases:
>> > [[clang::no_sanitize("vptr", "shift")]]  // disable just "vptr" and
>> > "shift"
>> > checks in the function
>> > [[clang::no_sanitize("undefined")]]  // disable all checks introduced by
>> > UBSan (-fsanitize=undefined)
>> > [[clang::no_sanitize("undefined", "address", "memory")]]  // Disable
>> > UBSan,
>> > MSan, ASan instrumentation (but use TSan, if the code will be compiled
>> > with
>> > -fsanitize=thread).
>> >
>> > Then no_sanitize_address will be just a synonym for
>> > no_sanitize("address"),
>> > etc.
>>
>> To me, using the existing attributes makes more sense. For starters,
>> they're self-documenting as to what sanitizer is being affected. This
>> means we don't have to be as careful about adding selective features
>> to the sanitizers (the namespace becomes the sanitizer, and not all
>> sanitizers).
>
>
> The sanitizers are structured in a different way. From Clang point of view,
> it's not the case that there are large
> "sanitizers", each of them having a set of "features". Instead, we have
> small
> "sanitizers" and "groups of sanitizers". They are completely defined by the
> file
> include/clang/Basic/Sanitizers.def. You may say that we have "flat
> namespace".
>
> -fsanitize=cfi is essentially a short way of writing of
> -fsanitize=cfi-cast-strict,cfi-derived-cast,cfi-unrelated-cast,cfi-nvcall,cfi-vcall
>
> As I understand Richard's proposal, it's a new attribute, which takes a list
> of strings
> as its arguments, and has the same semantics as -fsanitize= flag.
>
> ASan, for instance, is a single sanitizer (once again, from Clang's
> driver/codegen point of view). Now, it
> actually has some experimental functionality, but it's protected by runtime
> flags, which is a very different story.

Okay, then I agree that a single no_sanitize attribute would make
sense. Thank you for the clarifications!

~Aaron

>
>
>>
>> Also, how much do we have to worry about feature names across
>> sanitizers? e.g., asan and msan both share a feature that does
>> slightly different things under the same name?
>>
>>
>>
>> For background, eventually I want to move the sanitizer attributes out
>> of clang entirely (by having a plugin system for attributes), and have
>> them under the control of the individual sanitizers. Having all
>> sanitizers listed together makes this goal considerably more
>> challenging. By having each sanitizer segregate its own list of
>> features, it makes a more clear layering.
>>
>> ~Aaron
>>
>> >
>> >> I'm wondering if we can do
>> >> something like this:
>> >>
>> >> 1) Add a new no_sanitize_undefined attribute that turns off all ubsan
>> >> checks for a method, and,
>> >> 2) Have the attribute take an optional, variadic list of sanitizer
>> >> feature strings to fine-tune what gets turned off.
>> >> 3) Someday, if needed, add the optional list functionality to the
>> >> no_sanitize_address, no_sanitize_memory, and no_sanitize_thread
>> >> attributes.
>> >> 4) Someday add [[clang::no_sanitize("list", "of", "sanitizers")]] for
>> >> turning off entire sanitizers.
>> >>
>> >> This allows you to do:
>> >>
>> >> [[clang::no_sanitize_undefined("vptr", "something_else")]] void f() {}
>> >> // Turns off just vptr and something_else ubsan instrumentation
>> >> [[clang::no_sanitize_undefined]] void g() {} // Turns off all ubsan
>> >> instrumentation for this function
>> >>
>> >> // Someday
>> >> [[clang::no_sanitize("ubsan", "asan")]] void h() {} // Turns off all
>> >> ubsan and asan instrumentation, but allows tsan and msan.
>> >>
>> >> I think this provides maximal flexibility with minimal worry about
>> >> name clashes between sanitizer features. It comes with the handy
>> >> benefit of not needing to deprecate the no_sanitize_blah attributes,
>> >> but still scales.
>> >>
>> >> ~Aaron
>> >>
>> >> >
>> >> >
>> >> > http://reviews.llvm.org/D9059
>> >> >
>> >> > EMAIL PREFERENCES
>> >> >   http://reviews.llvm.org/settings/panel/emailpreferences/
>> >> >
>> >> >
>> >> >
>> >> > _______________________________________________
>> >> > cfe-commits mailing list
>> >> > cfe-commits at cs.uiuc.edu
>> >> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>> >
>> >
>> >
>> >
>> > --
>> > Alexey Samsonov
>> > vonosmas at gmail.com
>
>
>
>
> --
> Alexey Samsonov
> vonosmas at gmail.com