<div>Yeah, I shouldn't write commit messages late at night (I didn't commit until the morning to keep an eye on the bots).</div><div><br></div><div>I opted to follow cl in terms of erroring. I can make it a warning if you feel strongly about that.</div><div><br></div><div>I think for naked functions being stricter is better. The user can just as easily accomplish the same thing with an out-of-line assembly input (or, something terrible like module level assembly blocks).</div><div><br></div><div>With the GNU spelling, it restricts the function to just an asm blocks without operands. Furthermore, GCC restricts it to ARM, AVR, MSP430 (and MCORE, NDS32, RL87, RX, SPU), which I think we should also do. We currently don't check that.</div><div><br></div><div>The declspec spelling is supposed to prevent inlining at all costs, and can only be applied to non-member functions. What's even more interesting is that you don't have ASM blocks in ARM, so it being available is even more questionable [1]. Plus, I imagine that unwinding would be pretty fragile (since I don't think we could easily handle pdata/xdata emission in that case).</div><div><br></div><div>Technically, it makes no sense for a target to not support the naked attribute: it just elides the frame setup/tear down - we could just skip functions with the naked attribute in PEI. I don't think it's particularly difficult to support the concept at the IR level. The problem arises in trying to analyze it. If you want to completely skip that, why not just use out-of-line assembly? Or how do you generate unwinding information?</div><div><br></div><div>Am I overlooking a use case whuch cannot be accommodated with the same guarantees without lulling the developer into a false sense of safety?</div><div><br></div><div>[1]: naked functions don't have a prologue/epilogue but you don't have an ASM block, so how do add that? Windows ABI requires a FP, RA frame setup, how do you do that? How do you generate the .pdata?</div><div><br><div class="gmail_quote"><div>On Fri, Apr 7, 2017 at 12:06 PM Aaron Ballman via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Fri, Apr 7, 2017 at 2:53 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" class="gmail_msg" target="_blank">richard@metafoo.co.uk</a>> wrote:<br class="gmail_msg">
> On 7 Apr 2017 11:49 am, "David Majnemer via cfe-commits"<br class="gmail_msg">
> <<a href="mailto:cfe-commits@lists.llvm.org" class="gmail_msg" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br class="gmail_msg">
><br class="gmail_msg">
><br class="gmail_msg">
><br class="gmail_msg">
> On Fri, Apr 7, 2017 at 8:30 AM, Aaron Ballman via cfe-commits<br class="gmail_msg">
> <<a href="mailto:cfe-commits@lists.llvm.org" class="gmail_msg" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br class="gmail_msg">
>><br class="gmail_msg">
>> On Fri, Apr 7, 2017 at 11:13 AM, Saleem Abdulrasool via cfe-commits<br class="gmail_msg">
>> <<a href="mailto:cfe-commits@lists.llvm.org" class="gmail_msg" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br class="gmail_msg">
>> > Author: compnerd<br class="gmail_msg">
>> > Date: Fri Apr 7 10:13:47 2017<br class="gmail_msg">
>> > New Revision: 299774<br class="gmail_msg">
>> ><br class="gmail_msg">
>> > URL: <a href="http://llvm.org/viewvc/llvm-project?rev=299774&view=rev" rel="noreferrer" class="gmail_msg" target="_blank">http://llvm.org/viewvc/llvm-project?rev=299774&view=rev</a><br class="gmail_msg">
>> > Log:<br class="gmail_msg">
>> > Sema: prevent __declspec(naked) use on x64<br class="gmail_msg">
>> ><br class="gmail_msg">
>> > MSDN (<a href="https://msdn.microsoft.com/en-us/library/h5w10wxs.aspx" rel="noreferrer" class="gmail_msg" target="_blank">https://msdn.microsoft.com/en-us/library/h5w10wxs.aspx</a>) indicates<br class="gmail_msg">
>> > that `__declspec(naked)` is only permitted on x86 and ARM targets.<br class="gmail_msg">
>> > Testing with cl does confirm this behaviour. Provide a warning for use<br class="gmail_msg">
>> > of `__declspec(naked)` on x64.<br class="gmail_msg">
>><br class="gmail_msg">
>> Your patch is not providing a warning, it's providing an error, which<br class="gmail_msg">
>> seems inappropriate (to me). Why is this attribute not silently<br class="gmail_msg">
>> ignored there instead?<br class="gmail_msg">
><br class="gmail_msg">
><br class="gmail_msg">
> FWIW, MSVC diagnoses this with an error: <a href="https://godbolt.org/g/T5sQr5" rel="noreferrer" class="gmail_msg" target="_blank">https://godbolt.org/g/T5sQr5</a><br class="gmail_msg">
><br class="gmail_msg">
><br class="gmail_msg">
> Is there a reason we can't support this? Was our existing support broken in<br class="gmail_msg">
> some way? We generally allow our features to be combined in arbitrary ways<br class="gmail_msg">
> unless there's a reason not to.<br class="gmail_msg">
<br class="gmail_msg">
For the __declspec spelling of the attribute, we shouldn't really do<br class="gmail_msg">
*more* than what Microsoft allows, should we (even if we can)?<br class="gmail_msg">
<br class="gmail_msg">
As for error vs warning, I was mostly pointing out that (1) the commit<br class="gmail_msg">
comment doesn't match the behavior of the patch, which is always a<br class="gmail_msg">
concern, and (2) most of our attributes warn rather than err, unless<br class="gmail_msg">
there's a solid reason to err. If LLVM gets a naked attribute for a<br class="gmail_msg">
function and the architecture doesn't support the concept, what<br class="gmail_msg">
happens? I thought it ignored it (which suggests a warning rather than<br class="gmail_msg">
an error, to a certain extent), but I could be wrong.<br class="gmail_msg">
<br class="gmail_msg">
~Aaron<br class="gmail_msg">
<br class="gmail_msg">
><br class="gmail_msg">
>> (Btw, I did not see a review thread for this, did I miss one?) More<br class="gmail_msg">
>> comments below.<br class="gmail_msg">
>><br class="gmail_msg">
>> ><br class="gmail_msg">
>> > Added:<br class="gmail_msg">
>> > cfe/trunk/test/Sema/declspec-naked.c<br class="gmail_msg">
>> > Modified:<br class="gmail_msg">
>> > cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br class="gmail_msg">
>> > cfe/trunk/lib/Sema/SemaDeclAttr.cpp<br class="gmail_msg">
>> > cfe/trunk/test/Sema/ms-inline-asm.c<br class="gmail_msg">
>> ><br class="gmail_msg">
>> > Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br class="gmail_msg">
>> > URL:<br class="gmail_msg">
>> > <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" class="gmail_msg" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=299774&r1=299773&r2=299774&view=diff</a><br class="gmail_msg">
>> ><br class="gmail_msg">
>> > ==============================================================================<br class="gmail_msg">
>> > --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)<br class="gmail_msg">
>> > +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Fri Apr 7<br class="gmail_msg">
>> > 10:13:47 2017<br class="gmail_msg">
>> > @@ -3231,7 +3231,8 @@ def err_attribute_regparm_invalid_number<br class="gmail_msg">
>> > "'regparm' parameter must be between 0 and %0 inclusive">;<br class="gmail_msg">
>> > def err_attribute_not_supported_in_lang : Error<<br class="gmail_msg">
>> > "%0 attribute is not supported in %select{C|C++|Objective-C}1">;<br class="gmail_msg">
>> > -<br class="gmail_msg">
>> > +def err_attribute_not_supported_on_arch<br class="gmail_msg">
>> > + : Error<"%0 attribute is not supported on '%1'">;<br class="gmail_msg">
>> ><br class="gmail_msg">
>> > // Clang-Specific Attributes<br class="gmail_msg">
>> > def warn_attribute_iboutlet : Warning<<br class="gmail_msg">
>> ><br class="gmail_msg">
>> > Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp<br class="gmail_msg">
>> > URL:<br class="gmail_msg">
>> > <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=299774&r1=299773&r2=299774&view=diff" rel="noreferrer" class="gmail_msg" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=299774&r1=299773&r2=299774&view=diff</a><br class="gmail_msg">
>> ><br class="gmail_msg">
>> > ==============================================================================<br class="gmail_msg">
>> > --- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original)<br class="gmail_msg">
>> > +++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Fri Apr 7 10:13:47 2017<br class="gmail_msg">
>> > @@ -1923,6 +1923,17 @@ static void handleNakedAttr(Sema &S, Dec<br class="gmail_msg">
>> > Attr.getName()))<br class="gmail_msg">
>> > return;<br class="gmail_msg">
>> ><br class="gmail_msg">
>> > + if (Attr.isDeclspecAttribute()) {<br class="gmail_msg">
>> > + const auto &Triple = S.getASTContext().getTargetInfo().getTriple();<br class="gmail_msg">
>><br class="gmail_msg">
>> No need to have the Triple variable (which is a rather poor name given<br class="gmail_msg">
>> that it's also the type name).<br class="gmail_msg">
>> > + const auto &Arch = Triple.getArch();<br class="gmail_msg">
>><br class="gmail_msg">
>> Do not use auto for either of these.<br class="gmail_msg">
>><br class="gmail_msg">
>> ~Aaron<br class="gmail_msg">
>><br class="gmail_msg">
>> > + if (Arch != llvm::Triple::x86 &&<br class="gmail_msg">
>> > + (Arch != llvm::Triple::arm && Arch != llvm::Triple::thumb)) {<br class="gmail_msg">
>> > + S.Diag(Attr.getLoc(), diag::err_attribute_not_supported_on_arch)<br class="gmail_msg">
>> > + << Attr.getName() << Triple.getArchName();<br class="gmail_msg">
>> > + return;<br class="gmail_msg">
>> > + }<br class="gmail_msg">
>> > + }<br class="gmail_msg">
>> > +<br class="gmail_msg">
>> > D->addAttr(::new (S.Context) NakedAttr(Attr.getRange(), S.Context,<br class="gmail_msg">
>> ><br class="gmail_msg">
>> > Attr.getAttributeSpellingListIndex()));<br class="gmail_msg">
>> > }<br class="gmail_msg">
>> ><br class="gmail_msg">
>> > Added: cfe/trunk/test/Sema/declspec-naked.c<br class="gmail_msg">
>> > URL:<br class="gmail_msg">
>> > <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/declspec-naked.c?rev=299774&view=auto" rel="noreferrer" class="gmail_msg" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/declspec-naked.c?rev=299774&view=auto</a><br class="gmail_msg">
>> ><br class="gmail_msg">
>> > ==============================================================================<br class="gmail_msg">
>> > --- cfe/trunk/test/Sema/declspec-naked.c (added)<br class="gmail_msg">
>> > +++ cfe/trunk/test/Sema/declspec-naked.c Fri Apr 7 10:13:47 2017<br class="gmail_msg">
>> > @@ -0,0 +1,11 @@<br class="gmail_msg">
>> > +// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fsyntax-only<br class="gmail_msg">
>> > -fdeclspec -verify %s<br class="gmail_msg">
>> > +// RUN: %clang_cc1 -triple thumbv7-unknown-windows-msvc -fsyntax-only<br class="gmail_msg">
>> > -fdeclspec -verify %s<br class="gmail_msg">
>> > +// RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -fsyntax-only<br class="gmail_msg">
>> > -fdeclspec -verify %s<br class="gmail_msg">
>> > +#if defined(_M_IX86) || defined(_M_ARM)<br class="gmail_msg">
>> > +// CHECK: expected-no-diagnostics<br class="gmail_msg">
>> > +#endif<br class="gmail_msg">
>> > +<br class="gmail_msg">
>> > +void __declspec(naked) f(void) {}<br class="gmail_msg">
>> > +#if !defined(_M_IX86) && !defined(_M_ARM)<br class="gmail_msg">
>> > +// expected-error@-2{{'naked' attribute is not supported on 'x86_64'}}<br class="gmail_msg">
>> > +#endif<br class="gmail_msg">
>> ><br class="gmail_msg">
>> > Modified: cfe/trunk/test/Sema/ms-inline-asm.c<br class="gmail_msg">
>> > URL:<br class="gmail_msg">
>> > <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" class="gmail_msg" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/ms-inline-asm.c?rev=299774&r1=299773&r2=299774&view=diff</a><br class="gmail_msg">
>> ><br class="gmail_msg">
>> > ==============================================================================<br class="gmail_msg">
>> > --- cfe/trunk/test/Sema/ms-inline-asm.c (original)<br class="gmail_msg">
>> > +++ cfe/trunk/test/Sema/ms-inline-asm.c Fri Apr 7 10:13:47 2017<br class="gmail_msg">
>> > @@ -1,5 +1,5 @@<br class="gmail_msg">
>> > // REQUIRES: x86-registered-target<br class="gmail_msg">
>> > -// RUN: %clang_cc1 %s -triple x86_64-apple-darwin10 -fms-extensions<br class="gmail_msg">
>> > -fasm-blocks -Wno-microsoft -Wunused-label -verify -fsyntax-only<br class="gmail_msg">
>> > +// RUN: %clang_cc1 %s -triple i386-apple-darwin10 -fms-extensions<br class="gmail_msg">
>> > -fasm-blocks -Wno-microsoft -Wunused-label -verify -fsyntax-only<br class="gmail_msg">
>> ><br class="gmail_msg">
>> > void t1(void) {<br class="gmail_msg">
>> > __asm __asm // expected-error {{__asm used with no assembly<br class="gmail_msg">
>> > instructions}}<br class="gmail_msg">
>> ><br class="gmail_msg">
>> ><br class="gmail_msg">
>> > _______________________________________________<br class="gmail_msg">
>> > cfe-commits mailing list<br class="gmail_msg">
>> > <a href="mailto:cfe-commits@lists.llvm.org" class="gmail_msg" target="_blank">cfe-commits@lists.llvm.org</a><br class="gmail_msg">
>> > <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" class="gmail_msg" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br class="gmail_msg">
>> _______________________________________________<br class="gmail_msg">
>> cfe-commits mailing list<br class="gmail_msg">
>> <a href="mailto:cfe-commits@lists.llvm.org" class="gmail_msg" target="_blank">cfe-commits@lists.llvm.org</a><br class="gmail_msg">
>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" class="gmail_msg" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br class="gmail_msg">
><br class="gmail_msg">
><br class="gmail_msg">
><br class="gmail_msg">
> _______________________________________________<br class="gmail_msg">
> cfe-commits mailing list<br class="gmail_msg">
> <a href="mailto:cfe-commits@lists.llvm.org" class="gmail_msg" target="_blank">cfe-commits@lists.llvm.org</a><br class="gmail_msg">
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" class="gmail_msg" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br class="gmail_msg">
><br class="gmail_msg">
><br class="gmail_msg">
_______________________________________________<br class="gmail_msg">
cfe-commits mailing list<br class="gmail_msg">
<a href="mailto:cfe-commits@lists.llvm.org" class="gmail_msg" target="_blank">cfe-commits@lists.llvm.org</a><br class="gmail_msg">
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" class="gmail_msg" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br class="gmail_msg">
</blockquote></div></div><div dir="ltr">-- <br></div><div data-smartmail="gmail_signature">Saleem Abdulrasool<br>compnerd (at) compnerd (dot) org</div>