[PATCH] Implement no_sanitize attribute.

[Aaron Ballman]

On Thu, May 14, 2015 at 4:36 PM, Peter Collingbourne <peter at pcc.me.uk> wrote:
> On Mon, May 11, 2015 at 02:23:46PM -0400, Aaron Ballman wrote:
>> > +def NoSanitize : InheritableAttr {
>> > +  let Spellings = [GNU<"no_sanitize">];
>>
>> Why not a C++ spelling as well?
>
> Makes sense, done.
>
>> > +  let Subjects = SubjectList<[Function], ErrorDiag>;
>>
>> What about other subjects, such as ObjC methods?
>
> Added ObjC methods.
>
>> Do we also want to deprecate the existing attributes while we're at it?
>
> Probably not. We'll still need them for GCC compatibility.
>
>> > 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?
>
> See above -- we clear SanOpts if the function is in the blacklist.

Ah, and the below code can only clear flags, not set them. Okay, that
makes more sense then.

>
>> > +    // 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.
>
> It isn't clear that they can be parsed as alternate spellings because they
> take different numbers of arguments. It seems simplest to keep the semantic
> attributes, given that we don't plan to introduce any more of them.

I was unclear, I apologize. I was thinking along the lines of:


def NoSanitizeAttrs : InheritableAttr {
  let Spellings = [GNU<"no_sanitize_thread">, GNU<"no_sanitize_memory">, etc];
  let ASTNode = 0; // No AST representations; creates a NoSanitizeAttr
object instead
  let Subjects = SubjectList<[Function], ErrorDiag>;
  let Documentation = [Undocumented];
}

Then in SemaDeclAttr.cpp, the handler for all these attributes can be
implemented as instead attaching a no_sanitize attribute with the
proper mask. The downside to this is that it could lose source
fidelity when pretty printing (it would be equivalent, but not
identical attributes in that case). The upside is that we're not
carrying around AST nodes for these things, and we can remove some of
the clutter in Attr.td.

~Aaron