[PATCH] D48412: [RISCV] Add support for interrupt attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.
Herald added a subscriber: the_o.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:5280
+  // Check the attribute arguments.
+  if (AL.getNumArgs() > 1) {
+    S.Diag(AL.getLoc(), diag::err_attribute_too_many_arguments)
----------------
apazos wrote:
> aaron.ballman wrote:
> > Please call `checkAttributeNumArgs()` instead; the error you're using is incorrect (it's used for variadic parameters where you receive more arguments than you expect).
> The argument is optional and at most one argument , I will use checkAttributeAtMostNumArgs instead
Ah, yeah, I didn't pick up the optional part when writing my comment; this LG.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:5301
+
+  if (!isFunctionOrMethod(D)) {
+    S.Diag(D->getLocation(), diag::warn_attribute_wrong_decl_type)
----------------
apazos wrote:
> aaron.ballman wrote:
> > I don't think you need to perform this check -- I believe it's handled automatically (because you don't have custom parsing enabled).
> I think need it. Will double check in the test.
See `handleCommonAttributeFeatures()` -- it calls `diagnoseAppertainsTo()` which handles this for you. As it is, your check here does not match the subject list on the attribute. The declarative bit says it only appertains to a function and this check is for a function or Obj-C method.

Which brings up my next question: should this appertain to an ObjC method?


================
Comment at: test/Sema/riscv-interrupt-attr.c:18
+
+__attribute__((interrupt("user"), interrupt("supervisor"))) void foo6() { } // expected-warning {{repeated RISC-V 'interrupt' attribute}} \
+                                                                            // expected-note {{repeated RISC-V 'interrupt' attribute is here}}
----------------
apazos wrote:
> aaron.ballman wrote:
> > You should also add tests for:
> > ```
> > __attribute__((interrupt("user"))) void f(void);
> > __attribute__((interrupt("machine"))) void f(void);
> > 
> > void f(void) { }
> > 
> > [[gnu::interrupt("user") gnu::interrupt("machine")]] void g() {}
> > 
> > [[gnu::interrupt("user")]] [[gnu::interrupt("machine")]] void h() {}
> > ```
> For this test case tt seems LLVM honors the last setting, "machine".
> But gcc is honoring the first.
> I think the last setting should prevail. Will check with GCC folks.
Do all of these cases get diagnosed as being a repeated interrupt attribute? Should add them as test cases.


https://reviews.llvm.org/D48412