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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:5305
+
+  if (hasFunctionProto(D) && getFunctionOrMethodNumParams(D) != 0) {
+    S.Diag(D->getLocation(), diag::warn_riscv_interrupt_attribute) << 0;
----------------
I would have assumed this would be: `!hasFunctionProto(D) || getFunctionOrMethodNumParams(D) != 0`, but it depends on whether you want to support K&R C functions.


================
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:
> > 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.
Ah, the reason is because the parsed attributes that share a `ParseKind` can have different subject lists, so there's no way to do the semantic checking at that point -- we don't know which semantic attribute to check the subjects against until later.

Please put the `Subjects` list back in to Attr.td; it's still useful declarative information and I may solve this problem someday in the future.

I am not tied to whether the attribute appertains to a function and an obj-c method as that depends on the attribute in question, but the code as it stands is wrong. It checks whether the declaration is a function or a method and then tells the user the attribute can only appertain to a function and not a method. Which is correct?


================
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:
> > 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.
Please document this in AttrDocs.td.


================
Comment at: test/Sema/riscv-interrupt-attr.c:23
+                                                      // expected-note {{repeated RISC-V 'interrupt' attribute is here}}
+__attribute__((interrupt("user"))) void foo8() {}
+__attribute__((interrupt("supervisor"))) void foo9() {}
----------------
aaron.ballman wrote:
> Do you intend for functions without a prototype to be accepted? foo8() can be passed an arbitrary number of arguments, which is a bit different than what I thought you wanted the semantic check to be.
This question remains outstanding.


https://reviews.llvm.org/D48412