On Wed, Apr 22, 2015 at 1:55 PM, Aaron Ballman <[email protected]> wrote:
> On Wed, Apr 22, 2015 at 4:12 PM, Alexey Samsonov <[email protected]> > wrote: > > > > > > On Wed, Apr 22, 2015 at 12:37 PM, Aaron Ballman <[email protected]> > > wrote: > >> > >> On Mon, Apr 20, 2015 at 9:06 PM, Oliver Chang <[email protected]> > 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. > 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 > >> > [email protected] > >> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > > > > > > > > > > -- > > Alexey Samsonov > > [email protected] > -- Alexey Samsonov [email protected]
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
