[PATCH] D15709: [X86] Support 'interrupt' attribute for x86

[Alexey Bataev via cfe-commits]

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