<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Oct 8, 2015 at 1:21 PM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Thu, Oct 8, 2015 at 4:16 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
> On Thu, Oct 8, 2015 at 12:24 PM, Aaron Ballman via cfe-commits<br>
> <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> wrote:<br>
>><br>
>> Author: aaronballman<br>
>> Date: Thu Oct 8 14:24:08 2015<br>
>> New Revision: 249721<br>
>><br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=249721&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=249721&view=rev</a><br>
>> Log:<br>
>> When mapping no_sanitize_* attributes to no_sanitize attributes, handle<br>
>> GNU-style formatting that involves prefix and suffix underscores. Cleans up<br>
>> other usages of similar functionality.<br>
>><br>
>> Patch by Adrian Zgorzalek!<br>
>><br>
>> Modified:<br>
>> cfe/trunk/lib/Sema/SemaDeclAttr.cpp<br>
>> cfe/trunk/test/SemaCXX/attr-no-sanitize-address.cpp<br>
>> cfe/trunk/test/SemaCXX/attr-no-sanitize-memory.cpp<br>
>> cfe/trunk/test/SemaCXX/attr-no-sanitize-thread.cpp<br>
>><br>
>> Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=249721&r1=249720&r2=249721&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=249721&r1=249720&r2=249721&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original)<br>
>> +++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Thu Oct 8 14:24:08 2015<br>
>> @@ -1308,6 +1308,17 @@ void Sema::AddAssumeAlignedAttr(SourceRa<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 normalizeName(StringRef &AttrName) {<br>
>> + if (AttrName.startswith("__") && AttrName.endswith("__")) {<br>
>> + assert(AttrName.size() > 4 && "Name too short");<br>
><br>
><br>
> This assert will fire on the strings __, ___, and ____, which are valid in<br>
> some of the below cases.<br>
<br>
</div></div>That assert won't fire on anything but ____ because it's &&, not ||.</blockquote><div><br></div><div>I disagree. __ starts with __ and ends with __. The right thing to do here is remove the assert and put back the AttrName.size() > 4 check that the callers used to have.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I<br>
don't think these names were intended to be valid in their uses.<br>
However, you are correct that this will trigger assertions instead of<br>
diagnostics. Adrian, can you investigate?<br>
<div><div class="h5"><br>
><br>
>><br>
>> + AttrName = AttrName.drop_front(2).drop_back(2);<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,<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 (normalizeName(ModuleName)) {<br>
>> Module = &S.PP.getIdentifierTable().get(ModuleName);<br>
>> }<br>
>><br>
>> @@ -2648,9 +2656,7 @@ static void handleFormatAttr(Sema &S, De<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 (normalizeName(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<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>
>> + normalizeName(Str);<br>
>><br>
>> unsigned DestWidth = 0;<br>
>> bool IntegerMode = true;<br>
>> @@ -4533,8 +4537,10 @@ static void handleNoSanitizeAttr(Sema &S<br>
>><br>
>> static void handleNoSanitizeSpecificAttr(Sema &S, Decl *D,<br>
>> const AttributeList &Attr) {<br>
>> + StringRef AttrName = Attr.getName()->getName();<br>
>> + normalizeName(AttrName);<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>
><br>
><br>
> Is there any way we could use the spelling list index in this case rather<br>
> than repeating the attribute names and __-stripping here?<br>
<br>
</div></div>The spelling list index isn't exposed in a meaningful way (and I think<br>
that would be an abuse of it; I want to someday remove that<br>
implementation detail to something far more private).<br>
<br>
I was hoping there would be a way to use the semantic spelling, but<br>
the issue is that this particular attribute doesn't have semantic<br>
spellings. The NoSanitizeSpecificAttr would have one, but it has no<br>
AST node to hang those off of. Since we explicitly document that we do<br>
not want any additional names added to this list (see Attr.td line<br>
1500), I think this is a reasonable solution as-is.<br>
<span class="HOEnZb"><font color="#888888"><br>
~Aaron<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
>> Modified: cfe/trunk/test/SemaCXX/attr-no-sanitize-address.cpp<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/attr-no-sanitize-address.cpp?rev=249721&r1=249720&r2=249721&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/attr-no-sanitize-address.cpp?rev=249721&r1=249720&r2=249721&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- cfe/trunk/test/SemaCXX/attr-no-sanitize-address.cpp (original)<br>
>> +++ cfe/trunk/test/SemaCXX/attr-no-sanitize-address.cpp Thu Oct 8<br>
>> 14:24:08 2015<br>
>> @@ -5,12 +5,14 @@<br>
>> #if !__has_attribute(no_sanitize_address)<br>
>> #error "Should support no_sanitize_address"<br>
>> #endif<br>
>> -<br>
>> -void noanal_fun() 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>
>> +<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>
>> int noanal_testfn(int y) NO_SANITIZE_ADDRESS;<br>
>><br>
>> int noanal_testfn(int y) {<br>
>><br>
>> Modified: cfe/trunk/test/SemaCXX/attr-no-sanitize-memory.cpp<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/attr-no-sanitize-memory.cpp?rev=249721&r1=249720&r2=249721&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/attr-no-sanitize-memory.cpp?rev=249721&r1=249720&r2=249721&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- cfe/trunk/test/SemaCXX/attr-no-sanitize-memory.cpp (original)<br>
>> +++ cfe/trunk/test/SemaCXX/attr-no-sanitize-memory.cpp Thu Oct 8 14:24:08<br>
>> 2015<br>
>> @@ -5,12 +5,14 @@<br>
>> #if !__has_attribute(no_sanitize_memory)<br>
>> #error "Should support no_sanitize_memory"<br>
>> #endif<br>
>> -<br>
>> -void noanal_fun() 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>
>> +<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>
>> int noanal_testfn(int y) NO_SANITIZE_MEMORY;<br>
>><br>
>> int noanal_testfn(int y) {<br>
>><br>
>> Modified: cfe/trunk/test/SemaCXX/attr-no-sanitize-thread.cpp<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/attr-no-sanitize-thread.cpp?rev=249721&r1=249720&r2=249721&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/attr-no-sanitize-thread.cpp?rev=249721&r1=249720&r2=249721&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- cfe/trunk/test/SemaCXX/attr-no-sanitize-thread.cpp (original)<br>
>> +++ cfe/trunk/test/SemaCXX/attr-no-sanitize-thread.cpp Thu Oct 8 14:24:08<br>
>> 2015<br>
>> @@ -5,12 +5,14 @@<br>
>> #if !__has_attribute(no_sanitize_thread)<br>
>> #error "Should support no_sanitize_thread"<br>
>> #endif<br>
>> -<br>
>> -void noanal_fun() 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>
>> +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>
>> int noanal_testfn(int y) NO_SANITIZE_THREAD;<br>
>><br>
>> int noanal_testfn(int y) {<br>
>><br>
>><br>
>> _______________________________________________<br>
>> cfe-commits mailing list<br>
>> <a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>