On Thu, Oct 8, 2015 at 2:50 PM, Adrian Zgorzalek <a...@fb.com> wrote:
> Run clang-format, diff is in SVN format.
>
> Please, can you commit on behalf of me unless you are willing to grant me
> write permission to the repo ;-)
Thanks! I've commit in r249721. As for repo permissions, if you are
intending to contribute with some frequency, you can talk to Chris
Lattner about the process of getting an svn login.
~Aaron
>
> Adrian
>
>
>> On Oct 8, 2015, at 11:40 AM, Aaron Ballman <aa...@aaronballman.com> wrote:
>>
>> On Thu, Oct 8, 2015 at 2:33 PM, Adrian Zgorzalek <a...@fb.com> wrote:
>>> I so much like this fast review cycle :)
>>
>> I aim to please. ;-)
>>
>>>
>>> Comments applied.
>>
>> LGTM with one nit:
>>
>> +static bool normalizeName(StringRef& AttrName) {
>>
>> Should be (StringRef &AttrName) per style guidelines. A good idea is
>> to run clang-format over the patch or modified code, that fixes these
>> sort of things handily.
>>
>> Thanks!
>>
>> ~Aaron
>>
>>>
>>> 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
>>>>>
>>>>>
>>>
>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
