r299774 - Sema: prevent __declspec(naked) use on x64

Saleem Abdulrasool via cfe-commits cfe-commits at lists.llvm.org
Fri Apr 7 13:48:11 PDT 2017


Yeah, I shouldn't write commit messages late at night (I didn't commit
until the morning to keep an eye on the bots).

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?

On Fri, Apr 7, 2017 at 12:06 PM Aaron Ballman via cfe-commits <
cfe-commits at lists.llvm.org> wrote:

> On Fri, Apr 7, 2017 at 2:53 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
> > On 7 Apr 2017 11:49 am, "David Majnemer via cfe-commits"
> > <cfe-commits at lists.llvm.org> wrote:
> >
> >
> >
> > On Fri, Apr 7, 2017 at 8:30 AM, Aaron Ballman via cfe-commits
> > <cfe-commits at lists.llvm.org> wrote:
> >>
> >> On Fri, Apr 7, 2017 at 11:13 AM, Saleem Abdulrasool via cfe-commits
> >> <cfe-commits at 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 at -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 at lists.llvm.org
> >> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> >> _______________________________________________
> >> cfe-commits mailing list
> >> cfe-commits at lists.llvm.org
> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> >
> >
> >
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> >
> >
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-- 
Saleem Abdulrasool
compnerd (at) compnerd (dot) org
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170407/f5e85742/attachment-0001.html>


More information about the cfe-commits mailing list