aaron.ballman added inline comments.
================
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() {}
----------------
apazos wrote:
> aaron.ballman wrote:
> > apazos wrote:
> > > aaron.ballman wrote:
> > > > apazos wrote:
> > > > > 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.
> > > > Ah, now I see where the confusion is.
> > > >
> > > > In C, an empty parameter list declares a function without a prototype;
> > > > functions without prototypes can accept any number of arguments. To
> > > > declare a function that accepts no arguments, you must have a prototype
> > > > for the function and the parameter list is void. In C++, all functions
> > > > are prototyped and an empty parameter list is equivalent to a parameter
> > > > list of void. The word "prototype" doesn't mean "forward declaration".
> > > > e.g.,
> > > > ```
> > > > // C code
> > > > void foo1(); // Declaration; no prototype; accepts any number of
> > > > arguments.
> > > > void foo2() {} // Definition; no prototype; accepts any number of
> > > > arguments.
> > > > void foo3(void); // Declaration; prototype; accepts no arguments.
> > > > void foo4(void) {} // Definition; prototype; accepts no arguments.
> > > >
> > > > foo2(1, 2, 3); // ok
> > > > foo4(1, 2, 3); // error
> > > > ```
> > > > Because a function without a prototype can accept any number of
> > > > arguments, I think you want to diagnose such a function signature.
> > > Thanks for clarifying.
> > >
> > > I checked GCC behavior and it is less strict. For the example below, it
> > > silently accepts the interrupt attribute.
> > >
> > > extern int foo2();
> > > __attribute__((interrupt)) void foo();
> > > void foo() {
> > > foo2();
> > > }
> > >
> > > while in LLVM we would be rejecting with the message:
> > > RISC-V 'interrupt' attribute only applies to functions that have no
> > > parameters.
> > >
> > > I find the reuse of the message confusing.
> > >
> > > If we want stricter rule then we probably also need a specific message
> > > for the missing prototype.
> > >
> > > I checked GCC behavior and it is less strict. For the example below, it
> > > silently accepts the interrupt attribute.
> >
> > Does it drop the attribute?
> >
> > > If we want stricter rule then we probably also need a specific message
> > > for the missing prototype.
> >
> > If GCC silently drops the attribute in this case then we definitely want a
> > more strict rule. We already have a good diagnostic for this:
> > `warn_attribute_wrong_decl_type` with the expected type diagnostic index
> > being `ExpectedFunctionWithProtoType`.
> It does not drop, it compiles without warnings and it produces the code that
> is expected when interrupt attribute is set.
Oh! In that case, it's perfectly reasonable for us to support the construct as
well. I'm really sorry for the churn this back-and-forth has caused. I just
wanted to make sure that the runtime behavior matches GCC and that we diagnose
any circumstance under which we're dropping the attribute.
I like the most of the state of this test file where it uses `(void)` as the
parameter list for most of the functions, so how about we keep those changes?
I'd leave `foo4()` without the prototype and just remove the diagnostic I asked
you to introduce in the last patch.
https://reviews.llvm.org/D48412
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
