I so much like this fast review cycle :) Comments applied.
Adrian
> On Oct 8, 2015, at 11:24 AM, Aaron Ballman <aa...@aaronballman.com> wrote:
>
> On Thu, Oct 8, 2015 at 12:46 PM, Adrian Zgorzalek <a...@fb.com> wrote:
>> Feedback applied, new patch in the attachment.
>
> Thank you for working on this! A few comments:
>
>> From 13f4df6def5f26768f9ea048e013f779bb4a7814 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Adrian=20Zgorza=C5=82ek?= <a...@fb.com>
>> Date: Wed, 7 Oct 2015 15:55:53 -0700
>> Subject: [PATCH] Fix ICE in Clang when dealing with
>> attribute(__no_sanitize_*__)
>>
>> Summary:
>>
>> Both syntaxes: __attribute__((no_sanitize_address)) and
>> __attribute__((__no_sanitize__address__)) are valid, following
>> documentation:
>>
>>> The attribute identifier (but not scope) can also be specified with a
>>> preceding and following __ (double underscore) to avoid interference
>>> from a macro with the same name. For instance, gnu::__const__ can be
>>> used instead of gnu::const.
>>
>> This patch is fixing ICE when __const__ syntax is used.
>>
>> Test Plan:
>>
>> Added unittests in the patch which cover these cases. After applying
>> this patch they don't crash anymore.
>> ---
>> lib/Sema/SemaDeclAttr.cpp | 28 +++++++++++++++++-----------
>> test/SemaCXX/attr-no-sanitize-address.cpp | 2 ++
>> test/SemaCXX/attr-no-sanitize-memory.cpp | 2 ++
>> test/SemaCXX/attr-no-sanitize-thread.cpp | 2 ++
>> 4 files changed, 23 insertions(+), 11 deletions(-)
>
> FWIW, it's usually better to supply svn patches instead of git patches
> (my vcs has troubles applying patches like this).
>
>>
>> diff --git a/lib/Sema/SemaDeclAttr.cpp b/lib/Sema/SemaDeclAttr.cpp
>> index 3cf9567..b2f2cff 100644
>> --- a/lib/Sema/SemaDeclAttr.cpp
>> +++ b/lib/Sema/SemaDeclAttr.cpp
>> @@ -1308,6 +1308,17 @@ void Sema::AddAssumeAlignedAttr(SourceRange
>> AttrRange, Decl *D, Expr *E,
>> AssumeAlignedAttr(AttrRange, Context, E, OE, SpellingListIndex));
>> }
>>
>> +/// Normalize the attribute, __foo__ becomes foo.
>> +/// Returns true if normalization was applied.
>> +static bool normalizeAttrName(StringRef& AttrName) {
>
> I would prefer this be called normalizeName since it doesn't just
> normalize attribute names (for instance, this is used to normalize
> attribute arguments as well).
>
>> + if (AttrName.startswith("__") && AttrName.endswith("__")) {
>> + assert(AttrName.size() > 4 && "Too short attribute name");
>
> "Name too short" instead.
>
>> + AttrName = AttrName.substr(2, AttrName.size() - 4);
>
> I prefer: drop_front() and drop_back() instead.
>
>> + return true;
>> + }
>> + return false;
>> +}
>> +
>> static void handleOwnershipAttr(Sema &S, Decl *D, const AttributeList &AL) {
>> // This attribute must be applied to a function declaration. The first
>> // argument to the attribute must be an identifier, the name of the
>> resource,
>> @@ -1349,11 +1360,8 @@ static void handleOwnershipAttr(Sema &S, Decl *D,
>> const AttributeList &AL) {
>>
>> IdentifierInfo *Module = AL.getArgAsIdent(0)->Ident;
>>
>> - // Normalize the argument, __foo__ becomes foo.
>> StringRef ModuleName = Module->getName();
>> - if (ModuleName.startswith("__") && ModuleName.endswith("__") &&
>> - ModuleName.size() > 4) {
>> - ModuleName = ModuleName.drop_front(2).drop_back(2);
>> + if (normalizeAttrName(ModuleName)) {
>> Module = &S.PP.getIdentifierTable().get(ModuleName);
>> }
>>
>> @@ -2648,9 +2656,7 @@ static void handleFormatAttr(Sema &S, Decl *D, const
>> AttributeList &Attr) {
>> IdentifierInfo *II = Attr.getArgAsIdent(0)->Ident;
>> StringRef Format = II->getName();
>>
>> - // Normalize the argument, __foo__ becomes foo.
>> - if (Format.startswith("__") && Format.endswith("__")) {
>> - Format = Format.substr(2, Format.size() - 4);
>> + if (normalizeAttrName(Format)) {
>> // If we've modified the string name, we need a new identifier for it.
>> II = &S.Context.Idents.get(Format);
>> }
>> @@ -3131,9 +3137,7 @@ static void handleModeAttr(Sema &S, Decl *D, const
>> AttributeList &Attr) {
>> IdentifierInfo *Name = Attr.getArgAsIdent(0)->Ident;
>> StringRef Str = Name->getName();
>>
>> - // Normalize the attribute name, __foo__ becomes foo.
>> - if (Str.startswith("__") && Str.endswith("__"))
>> - Str = Str.substr(2, Str.size() - 4);
>> + static_cast<void>(normalizeAttrName(Str));
>
> No need for the static_cast to void here; we're okay ignoring this
> return value implicitly.
>
>>
>> unsigned DestWidth = 0;
>> bool IntegerMode = true;
>> @@ -4533,8 +4537,10 @@ static void handleNoSanitizeAttr(Sema &S, Decl *D,
>> const AttributeList &Attr) {
>>
>> static void handleNoSanitizeSpecificAttr(Sema &S, Decl *D,
>> const AttributeList &Attr) {
>> + StringRef AttrName = Attr.getName()->getName();
>> + static_cast<void>(normalizeAttrName(AttrName));
>
> No need for the static_cast here either.
>
> ~Aaron
>
>> std::string SanitizerName =
>> - llvm::StringSwitch<std::string>(Attr.getName()->getName())
>> + llvm::StringSwitch<std::string>(AttrName)
>> .Case("no_address_safety_analysis", "address")
>> .Case("no_sanitize_address", "address")
>> .Case("no_sanitize_thread", "thread")
>> diff --git a/test/SemaCXX/attr-no-sanitize-address.cpp
>> b/test/SemaCXX/attr-no-sanitize-address.cpp
>> index 9ca2863..9499742 100644
>> --- a/test/SemaCXX/attr-no-sanitize-address.cpp
>> +++ b/test/SemaCXX/attr-no-sanitize-address.cpp
>> @@ -8,6 +8,8 @@
>>
>> void noanal_fun() NO_SANITIZE_ADDRESS;
>>
>> +void noanal_fun_alt() __attribute__((__no_sanitize_address__));
>> +
>> void noanal_fun_args() __attribute__((no_sanitize_address(1))); // \
>> // expected-error {{'no_sanitize_address' attribute takes no arguments}}
>>
>> diff --git a/test/SemaCXX/attr-no-sanitize-memory.cpp
>> b/test/SemaCXX/attr-no-sanitize-memory.cpp
>> index 9cbcb03..41809a0 100644
>> --- a/test/SemaCXX/attr-no-sanitize-memory.cpp
>> +++ b/test/SemaCXX/attr-no-sanitize-memory.cpp
>> @@ -8,6 +8,8 @@
>>
>> void noanal_fun() NO_SANITIZE_MEMORY;
>>
>> +void noanal_fun_alt() __attribute__((__no_sanitize_memory__));
>> +
>> void noanal_fun_args() __attribute__((no_sanitize_memory(1))); // \
>> // expected-error {{'no_sanitize_memory' attribute takes no arguments}}
>>
>> diff --git a/test/SemaCXX/attr-no-sanitize-thread.cpp
>> b/test/SemaCXX/attr-no-sanitize-thread.cpp
>> index 6cb9c71..d97e050 100644
>> --- a/test/SemaCXX/attr-no-sanitize-thread.cpp
>> +++ b/test/SemaCXX/attr-no-sanitize-thread.cpp
>> @@ -8,6 +8,8 @@
>>
>> void noanal_fun() NO_SANITIZE_THREAD;
>>
>> +void noanal_fun_alt() __attribute__((__no_sanitize_thread__));
>> +
>> void noanal_fun_args() __attribute__((no_sanitize_thread(1))); // \
>> // expected-error {{'no_sanitize_thread' attribute takes no arguments}}
>>
>> --
>> 1.8.0
>>
>>
fix_ice.patch
Description: fix_ice.patch
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
