On Fri, Apr 7, 2017 at 4:48 PM, Saleem Abdulrasool <compn...@compnerd.org> wrote: > Yeah, I shouldn't write commit messages late at night (I didn't commit until > the morning to keep an eye on the bots).
Also, if you're going to make a change that might break users, having a review for that change is probably a good idea. I don't know of any users who WILL be broken by this, but having the discussion in advance is preferred. > I opted to follow cl in terms of erroring. I can make it a warning if you > feel strongly about that. > > 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). > > 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. > > 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). > > 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? > > Am I overlooking a use case whuch cannot be accommodated with the same > guarantees without lulling the developer into a false sense of safety? > > [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? Given that this is an MS attribute, I think following the MS behavior when first implementing the attribute is sensible (and similar for the GNU behavior). We can always extend that behavior when we find a good use case for it. However, this attribute has been in the wild for quite some time, so I worry about the impact to users by restricting it now -- do you know if this will break anyone? ~Aaron > > On Fri, Apr 7, 2017 at 12:06 PM Aaron Ballman via cfe-commits > <cfe-commits@lists.llvm.org> wrote: >> >> On Fri, Apr 7, 2017 at 2:53 PM, Richard Smith <rich...@metafoo.co.uk> >> wrote: >> > On 7 Apr 2017 11:49 am, "David Majnemer via cfe-commits" >> > <cfe-commits@lists.llvm.org> wrote: >> > >> > >> > >> > On Fri, Apr 7, 2017 at 8:30 AM, Aaron Ballman via cfe-commits >> > <cfe-commits@lists.llvm.org> wrote: >> >> >> >> On Fri, Apr 7, 2017 at 11:13 AM, Saleem Abdulrasool via cfe-commits >> >> <cfe-commits@lists.llvm.org> wrote: >> >> > Author: compnerd >> >> > Date: Fri Apr 7 10:13:47 2017 >> >> > New Revision: 299774 >> >> > >> >> > URL: http://llvm.org/viewvc/llvm-project?rev=299774&view=rev >> >> > Log: >> >> > Sema: prevent __declspec(naked) use on x64 >> >> > >> >> > MSDN (https://msdn.microsoft.com/en-us/library/h5w10wxs.aspx) >> >> > indicates >> >> > that `__declspec(naked)` is only permitted on x86 and ARM targets. >> >> > Testing with cl does confirm this behaviour. Provide a warning for >> >> > use >> >> > of `__declspec(naked)` on x64. >> >> >> >> Your patch is not providing a warning, it's providing an error, which >> >> seems inappropriate (to me). Why is this attribute not silently >> >> ignored there instead? >> > >> > >> > FWIW, MSVC diagnoses this with an error: https://godbolt.org/g/T5sQr5 >> > >> > >> > Is there a reason we can't support this? Was our existing support broken >> > in >> > some way? We generally allow our features to be combined in arbitrary >> > ways >> > unless there's a reason not to. >> >> For the __declspec spelling of the attribute, we shouldn't really do >> *more* than what Microsoft allows, should we (even if we can)? >> >> As for error vs warning, I was mostly pointing out that (1) the commit >> comment doesn't match the behavior of the patch, which is always a >> concern, and (2) most of our attributes warn rather than err, unless >> there's a solid reason to err. If LLVM gets a naked attribute for a >> function and the architecture doesn't support the concept, what >> happens? I thought it ignored it (which suggests a warning rather than >> an error, to a certain extent), but I could be wrong. >> >> ~Aaron >> >> > >> >> (Btw, I did not see a review thread for this, did I miss one?) More >> >> comments below. >> >> >> >> > >> >> > Added: >> >> > cfe/trunk/test/Sema/declspec-naked.c >> >> > Modified: >> >> > cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td >> >> > cfe/trunk/lib/Sema/SemaDeclAttr.cpp >> >> > cfe/trunk/test/Sema/ms-inline-asm.c >> >> > >> >> > Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td >> >> > URL: >> >> > >> >> > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=299774&r1=299773&r2=299774&view=diff >> >> > >> >> > >> >> > ============================================================================== >> >> > --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original) >> >> > +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Fri Apr 7 >> >> > 10:13:47 2017 >> >> > @@ -3231,7 +3231,8 @@ def err_attribute_regparm_invalid_number >> >> > "'regparm' parameter must be between 0 and %0 inclusive">; >> >> > def err_attribute_not_supported_in_lang : Error< >> >> > "%0 attribute is not supported in %select{C|C++|Objective-C}1">; >> >> > - >> >> > +def err_attribute_not_supported_on_arch >> >> > + : Error<"%0 attribute is not supported on '%1'">; >> >> > >> >> > // Clang-Specific Attributes >> >> > def warn_attribute_iboutlet : Warning< >> >> > >> >> > Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp >> >> > URL: >> >> > >> >> > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=299774&r1=299773&r2=299774&view=diff >> >> > >> >> > >> >> > ============================================================================== >> >> > --- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original) >> >> > +++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Fri Apr 7 10:13:47 2017 >> >> > @@ -1923,6 +1923,17 @@ static void handleNakedAttr(Sema &S, Dec >> >> > >> >> > Attr.getName())) >> >> > return; >> >> > >> >> > + if (Attr.isDeclspecAttribute()) { >> >> > + const auto &Triple = >> >> > S.getASTContext().getTargetInfo().getTriple(); >> >> >> >> No need to have the Triple variable (which is a rather poor name given >> >> that it's also the type name). >> >> > + const auto &Arch = Triple.getArch(); >> >> >> >> Do not use auto for either of these. >> >> >> >> ~Aaron >> >> >> >> > + if (Arch != llvm::Triple::x86 && >> >> > + (Arch != llvm::Triple::arm && Arch != llvm::Triple::thumb)) >> >> > { >> >> > + S.Diag(Attr.getLoc(), >> >> > diag::err_attribute_not_supported_on_arch) >> >> > + << Attr.getName() << Triple.getArchName(); >> >> > + return; >> >> > + } >> >> > + } >> >> > + >> >> > D->addAttr(::new (S.Context) NakedAttr(Attr.getRange(), S.Context, >> >> > >> >> > Attr.getAttributeSpellingListIndex())); >> >> > } >> >> > >> >> > Added: cfe/trunk/test/Sema/declspec-naked.c >> >> > URL: >> >> > >> >> > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/declspec-naked.c?rev=299774&view=auto >> >> > >> >> > >> >> > ============================================================================== >> >> > --- cfe/trunk/test/Sema/declspec-naked.c (added) >> >> > +++ cfe/trunk/test/Sema/declspec-naked.c Fri Apr 7 10:13:47 2017 >> >> > @@ -0,0 +1,11 @@ >> >> > +// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fsyntax-only >> >> > -fdeclspec -verify %s >> >> > +// RUN: %clang_cc1 -triple thumbv7-unknown-windows-msvc >> >> > -fsyntax-only >> >> > -fdeclspec -verify %s >> >> > +// RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -fsyntax-only >> >> > -fdeclspec -verify %s >> >> > +#if defined(_M_IX86) || defined(_M_ARM) >> >> > +// CHECK: expected-no-diagnostics >> >> > +#endif >> >> > + >> >> > +void __declspec(naked) f(void) {} >> >> > +#if !defined(_M_IX86) && !defined(_M_ARM) >> >> > +// expected-error@-2{{'naked' attribute is not supported on >> >> > 'x86_64'}} >> >> > +#endif >> >> > >> >> > Modified: cfe/trunk/test/Sema/ms-inline-asm.c >> >> > URL: >> >> > >> >> > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/ms-inline-asm.c?rev=299774&r1=299773&r2=299774&view=diff >> >> > >> >> > >> >> > ============================================================================== >> >> > --- cfe/trunk/test/Sema/ms-inline-asm.c (original) >> >> > +++ cfe/trunk/test/Sema/ms-inline-asm.c Fri Apr 7 10:13:47 2017 >> >> > @@ -1,5 +1,5 @@ >> >> > // REQUIRES: x86-registered-target >> >> > -// RUN: %clang_cc1 %s -triple x86_64-apple-darwin10 -fms-extensions >> >> > -fasm-blocks -Wno-microsoft -Wunused-label -verify -fsyntax-only >> >> > +// RUN: %clang_cc1 %s -triple i386-apple-darwin10 -fms-extensions >> >> > -fasm-blocks -Wno-microsoft -Wunused-label -verify -fsyntax-only >> >> > >> >> > void t1(void) { >> >> > __asm __asm // expected-error {{__asm used with no assembly >> >> > instructions}} >> >> > >> >> > >> >> > _______________________________________________ >> >> > cfe-commits mailing list >> >> > cfe-commits@lists.llvm.org >> >> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> >> _______________________________________________ >> >> cfe-commits mailing list >> >> cfe-commits@lists.llvm.org >> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> > >> > >> > >> > _______________________________________________ >> > cfe-commits mailing list >> > cfe-commits@lists.llvm.org >> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> > >> > >> _______________________________________________ >> cfe-commits mailing list >> cfe-commits@lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > > -- > Saleem Abdulrasool > compnerd (at) compnerd (dot) org _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
