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

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.

================
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
----------------
Should we also explicitly list x86-64 as well?

================
Comment at: include/clang/Basic/AttrDocs.td:1889
@@ +1888,3 @@
+
+  and user must properly define the structure the pointer pointing to.
+
----------------
"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?

================
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<
----------------
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>;
```


================
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">;
----------------
"can't be used directly": what does it mean to "use"? Take the address of? Call? Pass as an argument to another function call?

================
Comment at: lib/CodeGen/TargetInfo.cpp:2013
@@ -1998,1 +2012,3 @@
 
+  if (const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(D)) {
+    if (FD->hasAttr<IAInterruptAttr>()) {
----------------
Is there a way to prevent this code duplication?

================
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()) {
----------------
Based on this, the Subject line in Attr.td should be HasFunctionProto.

================
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)
----------------
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().

================
Comment at: lib/Sema/SemaDeclAttr.cpp:4558
@@ +4557,3 @@
+    S.Diag(Attr.getLoc(), diag::err_attribute_wrong_decl_type)
+        << Attr.getName() << ExpectedFunction;
+    return;
----------------
This should be using ExpectedFunctionWithProtoType instead.

================
Comment at: lib/Sema/SemaDeclAttr.cpp:4568
@@ +4567,3 @@
+  // Interrupt handler must have 1 or 2 parameters.
+  auto NumParams = getFunctionOrMethodNumParams(D);
+  if (NumParams < 1 || NumParams > 2) {
----------------
Please do not use auto here.

================
Comment at: lib/Sema/SemaDeclAttr.cpp:4579
@@ +4578,3 @@
+  }
+  // The second argument must be an unsigned integer.
+  if (NumParams == 2 &&
----------------
second argument, if present, must be an unsigned integer.

================
Comment at: lib/Sema/SemaDeclAttr.cpp:4581
@@ +4580,3 @@
+  if (NumParams == 2 &&
+      !getFunctionOrMethodParamType(D, 1)->isUnsignedIntegerType()) {
+    S.Diag(getFunctionOrMethodParamRange(D, 1).getBegin(),
----------------
This allows types like bool or __uint128_t; is that permissible?


http://reviews.llvm.org/D15709