<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 7 April 2017 at 12:06, 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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-">On Fri, Apr 7, 2017 at 2:53 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
> On 7 Apr 2017 11:49 am, "David Majnemer via cfe-commits"<br>
> <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> wrote:<br>
><br>
><br>
><br>
> On Fri, Apr 7, 2017 at 8:30 AM, Aaron Ballman via cfe-commits<br>
> <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> wrote:<br>
>><br>
>> On Fri, Apr 7, 2017 at 11:13 AM, Saleem Abdulrasool via cfe-commits<br>
>> <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> wrote:<br>
>> > Author: compnerd<br>
>> > Date: Fri Apr 7 10:13:47 2017<br>
>> > New Revision: 299774<br>
>> ><br>
>> > URL: <a href="http://llvm.org/viewvc/llvm-project?rev=299774&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=299774&view=rev</a><br>
>> > Log:<br>
>> > Sema: prevent __declspec(naked) use on x64<br>
>> ><br>
>> > MSDN (<a href="https://msdn.microsoft.com/en-us/library/h5w10wxs.aspx" rel="noreferrer" target="_blank">https://msdn.microsoft.com/<wbr>en-us/library/h5w10wxs.aspx</a>) indicates<br>
>> > that `__declspec(naked)` is only permitted on x86 and ARM targets.<br>
>> > Testing with cl does confirm this behaviour. Provide a warning for use<br>
>> > of `__declspec(naked)` on x64.<br>
>><br>
>> Your patch is not providing a warning, it's providing an error, which<br>
>> seems inappropriate (to me). Why is this attribute not silently<br>
>> ignored there instead?<br>
><br>
><br>
> FWIW, MSVC diagnoses this with an error: <a href="https://godbolt.org/g/T5sQr5" rel="noreferrer" target="_blank">https://godbolt.org/g/T5sQr5</a><br>
><br>
><br>
> Is there a reason we can't support this? Was our existing support broken in<br>
> some way? We generally allow our features to be combined in arbitrary ways<br>
> unless there's a reason not to.<br>
<br>
</span>For the __declspec spelling of the attribute, we shouldn't really do<br>
*more* than what Microsoft allows, should we (even if we can)?<br></blockquote><div><br></div><div>Why not? What is the purpose / benefit of adding an arbitrary restriction here, if the functionality naturally extends to other targets? (Also note that we support __declspec for non-MS targets.) Of course, if this *doesn't* work on other targets for some reason, then that might be a good justification for not supporting it there.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
As for error vs warning, I was mostly pointing out that (1) the commit<br>
comment doesn't match the behavior of the patch, which is always a<br>
concern, and (2) most of our attributes warn rather than err, unless<br>
there's a solid reason to err.</blockquote><div><br></div><div>+1</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">If LLVM gets a naked attribute for a<br>
function and the architecture doesn't support the concept, what<br>
happens? I thought it ignored it (which suggests a warning rather than<br>
an error, to a certain extent), but I could be wrong.<br>
<span class="gmail-HOEnZb"><font color="#888888"><br>
~Aaron<br>
</font></span><div class="gmail-HOEnZb"><div class="gmail-h5"><br>
><br>
>> (Btw, I did not see a review thread for this, did I miss one?) More<br>
>> comments below.<br>
>><br>
>> ><br>
>> > Added:<br>
>> > cfe/trunk/test/Sema/declspec-<wbr>naked.c<br>
>> > Modified:<br>
>> > cfe/trunk/include/clang/Basic/<wbr>DiagnosticSemaKinds.td<br>
>> > cfe/trunk/lib/Sema/<wbr>SemaDeclAttr.cpp<br>
>> > cfe/trunk/test/Sema/ms-inline-<wbr>asm.c<br>
>> ><br>
>> > Modified: cfe/trunk/include/clang/Basic/<wbr>DiagnosticSemaKinds.td<br>
>> > URL:<br>
>> > <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=299774&r1=299773&r2=299774&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/include/<wbr>clang/Basic/<wbr>DiagnosticSemaKinds.td?rev=<wbr>299774&r1=299773&r2=299774&<wbr>view=diff</a><br>
>> ><br>
>> > ==============================<wbr>==============================<wbr>==================<br>
>> > --- cfe/trunk/include/clang/Basic/<wbr>DiagnosticSemaKinds.td (original)<br>
>> > +++ cfe/trunk/include/clang/Basic/<wbr>DiagnosticSemaKinds.td Fri Apr 7<br>
>> > 10:13:47 2017<br>
>> > @@ -3231,7 +3231,8 @@ def err_attribute_regparm_invalid_<wbr>number<br>
>> > "'regparm' parameter must be between 0 and %0 inclusive">;<br>
>> > def err_attribute_not_supported_<wbr>in_lang : Error<<br>
>> > "%0 attribute is not supported in %select{C|C++|Objective-C}1">;<br>
>> > -<br>
>> > +def err_attribute_not_supported_<wbr>on_arch<br>
>> > + : Error<"%0 attribute is not supported on '%1'">;<br>
>> ><br>
>> > // Clang-Specific Attributes<br>
>> > def warn_attribute_iboutlet : Warning<<br>
>> ><br>
>> > Modified: cfe/trunk/lib/Sema/<wbr>SemaDeclAttr.cpp<br>
>> > URL:<br>
>> > <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=299774&r1=299773&r2=299774&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/lib/Sema/<wbr>SemaDeclAttr.cpp?rev=299774&<wbr>r1=299773&r2=299774&view=diff</a><br>
>> ><br>
>> > ==============================<wbr>==============================<wbr>==================<br>
>> > --- cfe/trunk/lib/Sema/<wbr>SemaDeclAttr.cpp (original)<br>
>> > +++ cfe/trunk/lib/Sema/<wbr>SemaDeclAttr.cpp Fri Apr 7 10:13:47 2017<br>
>> > @@ -1923,6 +1923,17 @@ static void handleNakedAttr(Sema &S, Dec<br>
>> > Attr.getName()))<br>
>> > return;<br>
>> ><br>
>> > + if (Attr.isDeclspecAttribute()) {<br>
>> > + const auto &Triple = S.getASTContext().<wbr>getTargetInfo().getTriple();<br>
>><br>
>> No need to have the Triple variable (which is a rather poor name given<br>
>> that it's also the type name).<br>
>> > + const auto &Arch = Triple.getArch();<br>
>><br>
>> Do not use auto for either of these.<br>
>>, prior to which <br>
>> ~Aaron<br>
>><br>
>> > + if (Arch != llvm::Triple::x86 &&<br>
>> > + (Arch != llvm::Triple::arm && Arch != llvm::Triple::thumb)) {<br>
>> > + S.Diag(Attr.getLoc(), diag::err_attribute_not_<wbr>supported_on_arch)<br>
>> > + << Attr.getName() << Triple.getArchName();<br>
>> > + return;<br>
>> > + }<br>
>> > + }<br>
>> > +<br>
>> > D->addAttr(::new (S.Context) NakedAttr(Attr.getRange(), S.Context,<br>
>> ><br>
>> > Attr.<wbr>getAttributeSpellingListIndex(<wbr>)));<br>
>> > }<br>
>> ><br>
>> > Added: cfe/trunk/test/Sema/declspec-<wbr>naked.c<br>
>> > URL:<br>
>> > <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/declspec-naked.c?rev=299774&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/test/Sema/<wbr>declspec-naked.c?rev=299774&<wbr>view=auto</a><br>
>> ><br>
>> > ==============================<wbr>==============================<wbr>==================<br>
>> > --- cfe/trunk/test/Sema/declspec-<wbr>naked.c (added)<br>
>> > +++ cfe/trunk/test/Sema/declspec-<wbr>naked.c Fri Apr 7 10:13:47 2017<br>
>> > @@ -0,0 +1,11 @@<br>
>> > +// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fsyntax-only<br>
>> > -fdeclspec -verify %s<br>
>> > +// RUN: %clang_cc1 -triple thumbv7-unknown-windows-msvc -fsyntax-only<br>
>> > -fdeclspec -verify %s<br>
>> > +// RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -fsyntax-only<br>
>> > -fdeclspec -verify %s<br>
>> > +#if defined(_M_IX86) || defined(_M_ARM)<br>
>> > +// CHECK: expected-no-diagnostics<br>
>> > +#endif<br>
>> > +<br>
>> > +void __declspec(naked) f(void) {}<br>
>> > +#if !defined(_M_IX86) && !defined(_M_ARM)<br>
>> > +// expected-error@-2{{'naked' attribute is not supported on 'x86_64'}}<br>
>> > +#endif<br>
>> ><br>
>> > Modified: cfe/trunk/test/Sema/ms-inline-<wbr>asm.c<br>
>> > URL:<br>
>> > <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/ms-inline-asm.c?rev=299774&r1=299773&r2=299774&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/test/Sema/<wbr>ms-inline-asm.c?rev=299774&r1=<wbr>299773&r2=299774&view=diff</a><br>
>> ><br>
>> > ==============================<wbr>==============================<wbr>==================<br>
>> > --- cfe/trunk/test/Sema/ms-inline-<wbr>asm.c (original)<br>
>> > +++ cfe/trunk/test/Sema/ms-inline-<wbr>asm.c Fri Apr 7 10:13:47 2017<br>
>> > @@ -1,5 +1,5 @@<br>
>> > // REQUIRES: x86-registered-target<br>
>> > -// RUN: %clang_cc1 %s -triple x86_64-apple-darwin10 -fms-extensions<br>
>> > -fasm-blocks -Wno-microsoft -Wunused-label -verify -fsyntax-only<br>
>> > +// RUN: %clang_cc1 %s -triple i386-apple-darwin10 -fms-extensions<br>
>> > -fasm-blocks -Wno-microsoft -Wunused-label -verify -fsyntax-only<br>
>> ><br>
>> > void t1(void) {<br>
>> > __asm __asm // expected-error {{__asm used with no assembly<br>
>> > instructions}}<br>
>> ><br>
>> ><br>
>> > ______________________________<wbr>_________________<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/<wbr>mailman/listinfo/cfe-commits</a><br>
>> ______________________________<wbr>_________________<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/<wbr>mailman/listinfo/cfe-commits</a><br>
><br>
><br>
><br>
> ______________________________<wbr>_________________<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/<wbr>mailman/listinfo/cfe-commits</a><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>