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