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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
