[PATCH] Implement no_sanitize attribute.
Peter Collingbourne
peter at pcc.me.uk
Wed May 13 15:40:40 PDT 2015
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 <aaron at aaronballman.com> wrote:
> > On Fri, May 8, 2015 at 7:33 PM, Peter Collingbourne <peter at pcc.me.uk> 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
More information about the cfe-commits
mailing list