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

[Ana Pazos via Phabricator via cfe-commits]

apazos added inline comments.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:5301
+
+  if (!isFunctionOrMethod(D)) {
+    S.Diag(D->getLocation(), diag::warn_attribute_wrong_decl_type)
----------------
aaron.ballman wrote:
> 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?
It looks like handleCommonAttributeFeatures should take care of the check, but I do not see it happening, it returns true in AL.diagnoseAppertainsTo(S, D) even when we have 
struct a test __attribute__((interrupt));

I will remove the Subjects in Attr.td and keep the checks as they are in handleRISCVInterruptAttr.

Several other targets do the same thing, they are reusing the helper functions that apply to both Function or Method.  We would have to create helper functions just for function types.


================
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}}
----------------
aaron.ballman wrote:
> 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.
The warning for repeated attribute is when it occurs more than once in the same declaration. If you have repeated declarations, the last one prevails.


https://reviews.llvm.org/D48412