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

[Aaron Ballman via Phabricator via cfe-commits]

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