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

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:259
@@ +258,3 @@
+def err_anyx86_interrupt_attribute : Error<
+  "interrupt service routine %select{must have 'void' return value|"
+  "can only have a pointer argument and an optional integer argument|"
----------------
rjmccall wrote:
> aaron.ballman wrote:
> > rjmccall wrote:
> > > aaron.ballman wrote:
> > > > I still dislike "interrupt service routine" -- there is no interrupt service routine involved here because the attribute does not apply to that function to make it one. The issue is with the attribute's requirements on the function type, so I still prefer "x86 |x86-64'interrupt' attribute only applies to functions that...". Also, "have 'void' return value" is still incorrect -- they need to have a void return type. Something like:
> > > > 
> > > > "%select{x86|x86-64}0 'interrupt' attribute only applies to functions that have %select{a 'void' return type|only a pointer parameter optionally followed by an integer parameter|a pointer as the first parameter|a %2 type as the second parameter}1"
> > > > 
> > > > (I also changed argument to parameter because arguments are on the caller side, and parameters are on the function declaration side.)
> > > It's fine to call it an interrupt service routine because the user intent to declare it as one is obvious.  It's still an ISR, it's just an ill-formed ISR.  It would even make some amount of sense to actually apply the attribute in the AST and just mark the declaration ill-formed.
> > That would make this diagnostic inconsistent with the other interrupt attribute diagnostics used for other targets, and I don't think it adds any value to do so. I don't think it makes sense to have the attribute mark the declaration as ill-formed either -- attributes are usually side information to the declaration (otherwise we implement them as keywords).
> If it's more consistent to use the other wording, that's fine.  This attribute is quite definitely not side information to the declaration, however, as it significantly changes its language and ABI rules; and that's true of many other attributes.  Attributes are very frequently used in lieu of keywords as an open-ended language extension mechanism.
That's true, we tend to be more lax with attributes than the standards committees are. My biggest concern is with consistency, but I don't really care which direction we go. It fits my mental model slightly better to complain about the attribute when the attribute mismatches with the declaration, because the declaration can stand on its own. However, since this is an error and not a warning where we drop the attribute, I can see the logic behind diagnosing that the declaration is wrong. The other attributes are inconsistent in this case. ARM and MIPS warns and drops the attribute, MSP430 errs.


http://reviews.llvm.org/D15709