ABataev marked 8 inline comments as done.
ABataev added a comment.
Aaron, thanks for the review!
================
Comment at: include/clang/Basic/AttrDocs.td:1867
@@ +1866,3 @@
+Clang supports the GNU style ``__attribute__((interrupt))`` attribute on
+x86 targets. This attribute may be attached to a function definition and
+instructs the backend to generate appropriate function entry/exit code so that
----------------
aaron.ballman wrote:
> Should we also explicitly list x86-64 as well?
Ok, will do
================
Comment at: include/clang/Basic/AttrDocs.td:1889
@@ +1888,3 @@
+
+ and user must properly define the structure the pointer pointing to.
+
----------------
aaron.ballman wrote:
> "the pointer pointing to"
>
> What do you mean by "properly define"? Do you mean it cannot be an opaque
> pointer, or that there is a particular structure that must be used?
I updated the description of this attribute to the latest one. Example also
changed
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2494
@@ +2493,3 @@
+def err_interrupt_function_wrong_return_type : Error<
+ "interrupt service routine must have void return value">;
+def err_interrupt_function_wrong_args : Error<
----------------
aaron.ballman wrote:
> It would be good to model these new diagnostics after the MIPS interrupt
> diagnostics.
> ```
> def warn_mips_interrupt_attribute : Warning<
> "MIPS 'interrupt' attribute only applies to functions that have "
> "%select{no parameters|a 'void' return type}0">,
> InGroup<IgnoredAttributes>;
> ```
>
Ok, will do
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2501
@@ +2500,3 @@
+ "interrupt service routine should have one of unsigned integer types as the
second argument">;
+def err_interrupt_function_called : Error<
+ "interrupt service routine can't be used directly">;
----------------
aaron.ballman wrote:
> "can't be used directly": what does it mean to "use"? Take the address of?
> Call? Pass as an argument to another function call?
You can't do anything with these functions directly: you can't call it, you
can't take an address, you can't pass it as an argument.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:4555
@@ +4554,3 @@
+ // e) The 2nd argument (if any) must be an unsigned integer.
+ if (!isFunctionOrMethod(D) || !hasFunctionProto(D) ||
+ !D->getDeclContext()->isFileContext()) {
----------------
aaron.ballman wrote:
> Based on this, the Subject line in Attr.td should be HasFunctionProto.
Ok, will do
================
Comment at: lib/Sema/SemaDeclAttr.cpp:4556
@@ +4555,3 @@
+ if (!isFunctionOrMethod(D) || !hasFunctionProto(D) ||
+ !D->getDeclContext()->isFileContext()) {
+ S.Diag(Attr.getLoc(), diag::err_attribute_wrong_decl_type)
----------------
aaron.ballman wrote:
> This means it is acceptable to have:
> ```
> namespace Foo {
> struct interrupt_frame;
>
> __attribute__ ((interrupt))
> void f (struct interrupt_frame *frame) {
> }
> }
> ```
> But not okay to have:
> ```
> struct interrupt_frame;
> class Foo {
> __attribute__ ((interrupt))
> static void f (struct interrupt_frame *frame) {
> }
> }
> ```
> Is that expected? If you want to disallow namespaces, I think you want
> isTranslationUnit() instead of isFileContext().
Yes, we allow any non-member functions.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:4581
@@ +4580,3 @@
+ if (NumParams == 2 &&
+ !getFunctionOrMethodParamType(D, 1)->isUnsignedIntegerType()) {
+ S.Diag(getFunctionOrMethodParamRange(D, 1).getBegin(),
----------------
aaron.ballman wrote:
> This allows types like bool or __uint128_t; is that permissible?
Checked with gcc, made it more strict. Thanks for catching this!
http://reviews.llvm.org/D15709
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
