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