These constructs were added to clang in r237055 and r237056. Peter
On Wed, May 13, 2015 at 06:38:52PM -0400, Aaron Ballman wrote: > There's actually a greater problem here which I didn't realize when I > did the initial review: your code is relying on constructs which do > not exist in clang. Specifically, SanitizerMask, parseSanitizerValue, > and expandSanitizerGroups. Are these meant to be part of your patch, > or are they part of compiler-rt? If they are part of compiler-rt, then > it complicates the design of this feature because we cannot rely on > compiler-rt being present for a build of clang. > > ~Aaron > > On Mon, May 11, 2015 at 2:23 PM, Aaron Ballman <[email protected]> wrote: > > On Fri, May 8, 2015 at 7:33 PM, Peter Collingbourne <[email protected]> wrote: > >> Hi samsonov, > >> > >> http://reviews.llvm.org/D9631 > >> > >> Files: > >> include/clang/AST/Attr.h > >> include/clang/Basic/Attr.td > >> include/clang/Basic/AttrDocs.td > >> lib/CodeGen/CodeGenFunction.cpp > >> lib/CodeGen/CodeGenModule.cpp > >> lib/Sema/SemaDeclAttr.cpp > >> test/CodeGen/address-safety-attr.cpp > >> test/CodeGen/sanitize-thread-attr.cpp > >> test/CodeGenCXX/cfi-vcall.cpp > >> test/Sema/attr-no-sanitize.c > >> utils/TableGen/ClangAttrEmitter.cpp > > > > I like the concept, but I think the approach isn't quite right yet. > > Some comments below. > > > >> Index: include/clang/AST/Attr.h > >> =================================================================== > >> --- include/clang/AST/Attr.h > >> +++ include/clang/AST/Attr.h > >> @@ -20,6 +20,7 @@ > >> #include "clang/AST/Type.h" > >> #include "clang/Basic/AttrKinds.h" > >> #include "clang/Basic/LLVM.h" > >> +#include "clang/Basic/Sanitizers.h" > >> #include "clang/Basic/SourceLocation.h" > >> #include "clang/Basic/VersionTuple.h" > >> #include "llvm/ADT/SmallVector.h" > >> Index: include/clang/Basic/Attr.td > >> =================================================================== > >> --- include/clang/Basic/Attr.td > >> +++ include/clang/Basic/Attr.td > >> @@ -137,6 +137,7 @@ > >> class BoolArgument<string name, bit opt = 0> : Argument<name, opt>; > >> class IdentifierArgument<string name, bit opt = 0> : Argument<name, opt>; > >> class IntArgument<string name, bit opt = 0> : Argument<name, opt>; > >> +class SanitizerMaskArgument<string name, bit opt = 0> : Argument<name, > >> opt>; > > > > I do not think this is the correct approach. More below. > > > >> class StringArgument<string name, bit opt = 0> : Argument<name, opt>; > >> class ExprArgument<string name, bit opt = 0> : Argument<name, opt>; > >> class FunctionArgument<string name, bit opt = 0> : Argument<name, opt>; > >> @@ -1386,6 +1387,14 @@ > >> let Documentation = [Undocumented]; > >> } > >> > >> +def NoSanitize : InheritableAttr { > >> + let Spellings = [GNU<"no_sanitize">]; > > > > Why not a C++ spelling as well? > > > >> + let Args = [SanitizerMaskArgument<"Mask">]; > > > > This seems like it should be a VariadicEnumArgument instead. If the > > options are not possible to enumerate here, we should consider doing > > some tablegen work to make them available both as frontend options as > > well as here. If that proves to be too complex for some reason, then > > you should create a VariadicStringArgument. The argument as it stands > > is far too special-case for comfort. > > > >> + let Subjects = SubjectList<[Function], ErrorDiag>; > > > > What about other subjects, such as ObjC methods? > > > >> + let HasCustomParsing = 1; > > > > This should not be required; further, it disables a lot of semantic > > checks you've not added yourself (basically all of the common checks). > > > >> + let Documentation = [NoSanitizeDocs]; > >> +} > >> + > >> // Attribute to disable AddressSanitizer (or equivalent) checks. > >> def NoSanitizeAddress : InheritableAttr { > >> let Spellings = [GCC<"no_address_safety_analysis">, > >> Index: include/clang/Basic/AttrDocs.td > >> =================================================================== > >> --- include/clang/Basic/AttrDocs.td > >> +++ include/clang/Basic/AttrDocs.td > >> @@ -920,6 +920,19 @@ > >> }]; > >> } > >> > >> +def NoSanitizeDocs : Documentation { > >> + let Category = DocCatFunction; > >> + let Content = [{ > >> +Use the ``no_sanitize`` attribute on a function declaration to specify > >> +that a particular instrumentation or set of instrumentations should not be > >> +applied to that function. The attribute takes a list of string literals, > >> +which have the same meaning as values accepted by the ``-fno-sanitize=`` > >> +flag. For example, ``__attribute__((no_sanitize("address", "thread")))`` > >> +specifies that AddressSanitizer and ThreadSanitizer should not be applied > >> +to the function. > >> + }]; > > > > Do we also want to deprecate the existing attributes while we're at it? > > > >> +} > >> + > >> def NoSanitizeAddressDocs : Documentation { > >> let Category = DocCatFunction; > >> // This function has multiple distinct spellings, and so it requires a > >> custom > >> Index: lib/CodeGen/CodeGenFunction.cpp > >> =================================================================== > >> --- lib/CodeGen/CodeGenFunction.cpp > >> +++ lib/CodeGen/CodeGenFunction.cpp > >> @@ -608,6 +608,26 @@ > >> if (CGM.isInSanitizerBlacklist(Fn, Loc)) > >> SanOpts.clear(); > >> > >> + if (D) { > > > > Is it intentional that the old code only added these attributes if the > > function was not in the sanitizer black list, and the new code always > > adds them? > > > >> + // Apply the no_sanitize* attributes to SanOpts. > >> + if (auto NoSanAttr = D->getAttr<NoSanitizeAttr>()) > >> + SanOpts.Mask &= ~NoSanAttr->getMask(); > >> + if (D->hasAttr<NoSanitizeAddressAttr>()) > >> + SanOpts.set(SanitizerKind::Address, false); > >> + if (D->hasAttr<NoSanitizeThreadAttr>()) > >> + SanOpts.set(SanitizerKind::Thread, false); > >> + if (D->hasAttr<NoSanitizeMemoryAttr>()) > >> + SanOpts.set(SanitizerKind::Memory, false); > > > > I think that the NoSanitize-foo semantic attributes should be removed > > and specified in terms of NoSanitizeAttr. The attributes should still > > be parsed as alternate spellings of no_sanitize. > > > >> + } > >> + > >> + // Apply sanitizer attributes to the function. > >> + if (SanOpts.has(SanitizerKind::Address)) > >> + Fn->addFnAttr(llvm::Attribute::SanitizeAddress); > >> + if (SanOpts.has(SanitizerKind::Thread)) > >> + Fn->addFnAttr(llvm::Attribute::SanitizeThread); > >> + if (SanOpts.has(SanitizerKind::Memory)) > >> + Fn->addFnAttr(llvm::Attribute::SanitizeMemory); > >> + > >> // Pass inline keyword to optimizer if it appears explicitly on any > >> // declaration. Also, in the case of -fno-inline attach NoInline > >> // attribute to all function that are not marked AlwaysInline. > >> Index: lib/CodeGen/CodeGenModule.cpp > >> =================================================================== > >> --- lib/CodeGen/CodeGenModule.cpp > >> +++ lib/CodeGen/CodeGenModule.cpp > >> @@ -745,23 +745,6 @@ > >> else if (LangOpts.getStackProtector() == LangOptions::SSPReq) > >> B.addAttribute(llvm::Attribute::StackProtectReq); > >> > >> - // Add sanitizer attributes if function is not blacklisted. > >> - if (!isInSanitizerBlacklist(F, D->getLocation())) { > >> - // When AddressSanitizer is enabled, set SanitizeAddress attribute > >> - // unless __attribute__((no_sanitize_address)) is used. > >> - if (LangOpts.Sanitize.has(SanitizerKind::Address) && > >> - !D->hasAttr<NoSanitizeAddressAttr>()) > >> - B.addAttribute(llvm::Attribute::SanitizeAddress); > >> - // Same for ThreadSanitizer and __attribute__((no_sanitize_thread)) > >> - if (LangOpts.Sanitize.has(SanitizerKind::Thread) && > >> - !D->hasAttr<NoSanitizeThreadAttr>()) > >> - B.addAttribute(llvm::Attribute::SanitizeThread); > >> - // Same for MemorySanitizer and __attribute__((no_sanitize_memory)) > >> - if (LangOpts.Sanitize.has(SanitizerKind::Memory) && > >> - !D->hasAttr<NoSanitizeMemoryAttr>()) > >> - B.addAttribute(llvm::Attribute::SanitizeMemory); > >> - } > >> - > >> F->addAttributes(llvm::AttributeSet::FunctionIndex, > >> llvm::AttributeSet::get( > >> F->getContext(), > >> llvm::AttributeSet::FunctionIndex, B)); > >> Index: lib/Sema/SemaDeclAttr.cpp > >> =================================================================== > >> --- lib/Sema/SemaDeclAttr.cpp > >> +++ lib/Sema/SemaDeclAttr.cpp > >> @@ -4354,6 +4354,28 @@ > >> handleAttrWithMessage<DeprecatedAttr>(S, D, Attr); > >> } > >> > >> +static void handleNoSanitizeAttr(Sema &S, Decl *D, const AttributeList > >> &Attr) { > >> + if (!checkAttributeAtLeastNumArgs(S, Attr, 1)) > >> + return; > >> + > >> + SanitizerMask Mask = 0; > >> + > >> + for (unsigned I = 0, E = Attr.getNumArgs(); I != E; ++I) { > >> + StringRef SanitizerName; > >> + SourceLocation LiteralLoc; > >> + > >> + if (!S.checkStringLiteralArgumentAttr(Attr, I, SanitizerName, > >> &LiteralLoc)) > >> + return; > >> + > >> + SanitizerMask ParsedMask = > >> + parseSanitizerValue(SanitizerName, /*AllowGroups=*/true); > >> + Mask |= expandSanitizerGroups(ParsedMask); > >> + } > >> + > >> + D->addAttr(::new (S.Context) NoSanitizeAttr( > >> + Attr.getRange(), S.Context, Mask, > >> Attr.getAttributeSpellingListIndex())); > >> +} > >> + > >> /// Handles semantic checking for features that are common to all > >> attributes, > >> /// such as checking whether a parameter was properly specified, or the > >> correct > >> /// number of arguments were passed, etc. > >> @@ -4822,6 +4844,9 @@ > >> case AttributeList::AT_ScopedLockable: > >> handleSimpleAttribute<ScopedLockableAttr>(S, D, Attr); > >> break; > >> + case AttributeList::AT_NoSanitize: > >> + handleNoSanitizeAttr(S, D, Attr); > >> + break; > >> case AttributeList::AT_NoSanitizeAddress: > >> handleSimpleAttribute<NoSanitizeAddressAttr>(S, D, Attr); > >> break; > >> Index: test/CodeGen/address-safety-attr.cpp > >> =================================================================== > >> --- test/CodeGen/address-safety-attr.cpp > >> +++ test/CodeGen/address-safety-attr.cpp > >> @@ -52,6 +52,21 @@ > >> int NoAddressSafety2(int *a); > >> int NoAddressSafety2(int *a) { return *a; } > >> > >> +// WITHOUT: NoAddressSafety3{{.*}}) [[NOATTR]] > >> +// BLFILE: NoAddressSafety3{{.*}}) [[NOATTR]] > >> +// BLFUNC: NoAddressSafety3{{.*}}) [[NOATTR]] > >> +// ASAN: NoAddressSafety3{{.*}}) [[NOATTR]] > >> +__attribute__((no_sanitize("address"))) > >> +int NoAddressSafety3(int *a) { return *a; } > >> + > >> +// WITHOUT: NoAddressSafety4{{.*}}) [[NOATTR]] > >> +// BLFILE: NoAddressSafety4{{.*}}) [[NOATTR]] > >> +// BLFUNC: NoAddressSafety4{{.*}}) [[NOATTR]] > >> +// ASAN: NoAddressSafety4{{.*}}) [[NOATTR]] > >> +__attribute__((no_sanitize("address"))) > >> +int NoAddressSafety4(int *a); > >> +int NoAddressSafety4(int *a) { return *a; } > >> + > >> // WITHOUT: AddressSafetyOk{{.*}}) [[NOATTR]] > >> // BLFILE: AddressSafetyOk{{.*}}) [[NOATTR]] > >> // BLFUNC: AddressSafetyOk{{.*}}) [[WITH]] > >> @@ -86,16 +101,25 @@ > >> template<int i> > >> int TemplateAddressSafetyOk() { return i; } > >> > >> -// WITHOUT: TemplateNoAddressSafety{{.*}}) [[NOATTR]] > >> -// BLFILE: TemplateNoAddressSafety{{.*}}) [[NOATTR]] > >> -// BLFUNC: TemplateNoAddressSafety{{.*}}) [[NOATTR]] > >> -// ASAN: TemplateNoAddressSafety{{.*}}) [[NOATTR]] > >> +// WITHOUT: TemplateNoAddressSafety1{{.*}}) [[NOATTR]] > >> +// BLFILE: TemplateNoAddressSafety1{{.*}}) [[NOATTR]] > >> +// BLFUNC: TemplateNoAddressSafety1{{.*}}) [[NOATTR]] > >> +// ASAN: TemplateNoAddressSafety1{{.*}}) [[NOATTR]] > >> template<int i> > >> __attribute__((no_sanitize_address)) > >> -int TemplateNoAddressSafety() { return i; } > >> +int TemplateNoAddressSafety1() { return i; } > >> + > >> +// WITHOUT: TemplateNoAddressSafety2{{.*}}) [[NOATTR]] > >> +// BLFILE: TemplateNoAddressSafety2{{.*}}) [[NOATTR]] > >> +// BLFUNC: TemplateNoAddressSafety2{{.*}}) [[NOATTR]] > >> +// ASAN: TemplateNoAddressSafety2{{.*}}) [[NOATTR]] > >> +template<int i> > >> +__attribute__((no_sanitize("address"))) > >> +int TemplateNoAddressSafety2() { return i; } > >> > >> int force_instance = TemplateAddressSafetyOk<42>() > >> - + TemplateNoAddressSafety<42>(); > >> + + TemplateNoAddressSafety1<42>() > >> + + TemplateNoAddressSafety2<42>(); > >> > >> // Check that __cxx_global_var_init* get the sanitize_address attribute. > >> int global1 = 0; > >> Index: test/CodeGen/sanitize-thread-attr.cpp > >> =================================================================== > >> --- test/CodeGen/sanitize-thread-attr.cpp > >> +++ test/CodeGen/sanitize-thread-attr.cpp > >> @@ -22,6 +22,12 @@ > >> int NoTSAN2(int *a); > >> int NoTSAN2(int *a) { return *a; } > >> > >> +// WITHOUT: NoTSAN3{{.*}}) [[NOATTR:#[0-9]+]] > >> +// BL: NoTSAN3{{.*}}) [[NOATTR:#[0-9]+]] > >> +// TSAN: NoTSAN3{{.*}}) [[NOATTR:#[0-9]+]] > >> +__attribute__((no_sanitize("thread"))) > >> +int NoTSAN3(int *a) { return *a; } > >> + > >> // WITHOUT: TSANOk{{.*}}) [[NOATTR]] > >> // BL: TSANOk{{.*}}) [[NOATTR]] > >> // TSAN: TSANOk{{.*}}) [[WITH:#[0-9]+]] > >> Index: test/CodeGenCXX/cfi-vcall.cpp > >> =================================================================== > >> --- test/CodeGenCXX/cfi-vcall.cpp > >> +++ test/CodeGenCXX/cfi-vcall.cpp > >> @@ -47,16 +47,32 @@ > >> a->f(); > >> } > >> > >> -// CHECK: define internal void @_Z2dfPN12_GLOBAL__N_11DE > >> -void df(D *d) { > >> +// CHECK: define internal void @_Z3df1PN12_GLOBAL__N_11DE > >> +void df1(D *d) { > >> // CHECK: {{%[^ ]*}} = call i1 @llvm.bitset.test(i8* {{%[^ ]*}}, > >> metadata !"[{{.*}}cfi-vcall.cpp]N12_GLOBAL__N_11DE") > >> d->f(); > >> } > >> > >> +// CHECK: define internal void @_Z3df2PN12_GLOBAL__N_11DE > >> +__attribute__((no_sanitize("cfi"))) > >> +void df2(D *d) { > >> + // CHECK-NOT: call i1 @llvm.bitset.test > >> + d->f(); > >> +} > >> + > >> +// CHECK: define internal void @_Z3df3PN12_GLOBAL__N_11DE > >> +__attribute__((no_sanitize("address", "cfi-vcall"))) > >> +void df3(D *d) { > >> + // CHECK-NOT: call i1 @llvm.bitset.test > >> + d->f(); > >> +} > >> + > >> D d; > >> > >> void foo() { > >> - df(&d); > >> + df1(&d); > >> + df2(&d); > >> + df3(&d); > >> } > >> > >> // CHECK-DAG: !{!"1A", [3 x i8*]* @_ZTV1A, i64 16} > >> Index: test/Sema/attr-no-sanitize.c > >> =================================================================== > >> --- /dev/null > >> +++ test/Sema/attr-no-sanitize.c > >> @@ -0,0 +1,22 @@ > >> +// RUN: %clang_cc1 -fsyntax-only -verify %s > >> +// RUN: not %clang_cc1 -ast-dump %s 2>&1 | FileCheck %s > >> + > >> +int f1() __attribute__((no_sanitize)); // expected-error{{'no_sanitize' > >> attribute takes at least 1 argument}} > >> + > >> +int f2() __attribute__((no_sanitize(1))); // > >> expected-error{{'no_sanitize' attribute requires a string}} > >> + > >> +// CHECK-LABEL: FunctionDecl {{.*}} f3 > >> +// CHECK: NoSanitizeAttr {{.*}} 1 > >> +int f3() __attribute__((no_sanitize("address"))); > >> + > >> +// CHECK-LABEL: FunctionDecl {{.*}} f4 > >> +// CHECK: NoSanitizeAttr {{.*}} 4 > >> +int f4() __attribute__((no_sanitize("thread"))); > >> + > >> +// CHECK-LABEL: FunctionDecl {{.*}} f5 > >> +// CHECK: NoSanitizeAttr {{.*}} 5 > >> +int f5() __attribute__((no_sanitize("address", "thread"))); > >> + > >> +// CHECK-LABEL: FunctionDecl {{.*}} f6 > >> +// CHECK: NoSanitizeAttr {{.*}} 0 > >> +int f6() __attribute__((no_sanitize("unknown"))); > >> Index: utils/TableGen/ClangAttrEmitter.cpp > >> =================================================================== > >> --- utils/TableGen/ClangAttrEmitter.cpp > >> +++ utils/TableGen/ClangAttrEmitter.cpp > >> @@ -282,7 +282,8 @@ > >> } else if (type == "bool") { > >> OS << " if (SA->get" << getUpperName() << "()) OS << \" " > >> << getUpperName() << "\";\n"; > >> - } else if (type == "int" || type == "unsigned") { > >> + } else if (type == "int" || type == "unsigned" || > >> + type == "SanitizerMask") { > >> OS << " OS << \" \" << SA->get" << getUpperName() << "();\n"; > >> } else { > >> llvm_unreachable("Unknown SimpleArgument type!"); > >> @@ -1036,6 +1037,8 @@ > >> Arg, Attr, "int", Arg.getValueAsInt("Default")); > >> else if (ArgName == "IntArgument") > >> Ptr = llvm::make_unique<SimpleArgument>(Arg, Attr, "int"); > >> + else if (ArgName == "SanitizerMaskArgument") > >> + Ptr = llvm::make_unique<SimpleArgument>(Arg, Attr, "SanitizerMask"); > >> else if (ArgName == "StringArgument") > >> Ptr = llvm::make_unique<StringArgument>(Arg, Attr); > >> else if (ArgName == "TypeArgument") > >> > > > > ~Aaron > -- Peter _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
