[PATCH] Implement no_sanitize attribute.
Aaron Ballman
aaron at aaronballman.com
Thu May 14 07:42:50 PDT 2015
On Wed, May 13, 2015 at 6:40 PM, Peter Collingbourne <peter at pcc.me.uk> wrote:
> These constructs were added to clang in r237055 and r237056.
Thank you; I was just slightly too out of date to have them. Having
fetched, I'm back on the same page.
I don't think a variadic enumeration argument will be worth the effort
given the structure of Sanitizers.def. However, the current approach
is still incorrect. For instance, the attribute will not round-trip a
pretty printing, and it fails to diagnose illogical values that wind
up being ignored. The semantic attribute needs to carry around the
valid original information the user provided, but I can see why you'd
also want it to carry it around in mask format.
I think the correct approach for this is:
1) Make a VariadicStringArgument tablegen type (model it after
VariadicExprArgument, roughly). (Attr.td, ClangAttrEmitter.cpp)
2) Implement no_sanitize in terms of a VariadicStringArgument (Attr.td)
3) Add an additional public member to hold the SanitizerMask value (Attr.td)
4) Issue diagnostics for invalid members when creating the sanitizer
mask value (SemaDeclAttr.cpp)
5) Create the attribute, then attach the flattened SanitizerMask
value, then attach it to the declaration (SemaDeclAttr.cpp)
If you prefer to stick with a custom argument like you have, you could
add most of this logic as part of the tablegen (e.g., the constructor
can take the variadic strings, but expose a SanitizerMask getter from
the semantic attribute, automate testing of the input values, etc).
However, since that would only be useful for this one attribute, it
seems like it might be overkill (though clean overkill, if you wanted
to go that route).
~Aaron
>
> 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