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