[PATCH] Implement no_sanitize attribute.
Aaron Ballman
aaron at aaronballman.com
Wed May 13 15:38:52 PDT 2015
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
More information about the cfe-commits
mailing list