apazos 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;
----------------
aaron.ballman wrote:
> 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.
hasFunctionProto also returns true for a function definition like this one
__attribute__((interrupt)) void foo1(int) {}.
================
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:
> > > 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?
Sure I can add Subjects back in.
I will remove the helper function and use the simple check D->getFunctionType()
== nullptr.
================
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:
> > > 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.
Sure, I can add that info to the description.
================
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:
> 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.
The checks are validating both function definitions and function prototypes
like these:
_attribute__((interrupt)) void foo1() {}
__attribute__((interrupt)) void foo(void);
Not sure what the confusion is.
https://reviews.llvm.org/D48412
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
