r299774 - Sema: prevent __declspec(naked) use on x64
Aaron Ballman via cfe-commits
cfe-commits at lists.llvm.org
Mon Apr 10 06:23:29 PDT 2017
On Fri, Apr 7, 2017 at 4:48 PM, Saleem Abdulrasool
<compnerd at 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 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
More information about the cfe-commits
mailing list