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

Richard Smith via cfe-commits cfe-commits at lists.llvm.org
Fri Apr 7 13:48:00 PDT 2017


On 7 April 2017 at 12:06, Aaron Ballman <aaron at aaronballman.com> 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)?
>

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.

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.


+1

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.
> >>, prior to which
> >> ~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
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170407/941103c1/attachment.html>


More information about the cfe-commits mailing list