Page MenuHomePhabricator

[RISCV] Add support for interrupt attribute
ClosedPublic

Authored by apazos on Jun 20 2018, 6:48 PM.

Details

Summary

Clang supports the GNU style `__attribute__((interrupt))` attribute on RISCV targets.
Permissible values for this parameter are user, supervisor, and machine.
If there is no parameter, then it defaults to machine.
Reference: https://gcc.gnu.org/onlinedocs/gcc/RISC-V-Function-Attributes.html
Based on initial patch by Zhaoshi Zheng.

Diff Detail

Repository
rC Clang

Event Timeline

apazos created this revision.Jun 20 2018, 6:48 PM
apazos edited the summary of this revision. (Show Details)Jun 20 2018, 6:49 PM

I think this should also cover mismatched arguments if the attribute appears several times, and reject/warn about the attribute combination in these cases.

For example, __attribute__((interrupt("machine"))) __attribute__((interrupt("user"))) void foo() {} is accepted, and adds the attribute "interrupt"="user". It seems whichever is first used appears on the function.

I don't have an up to date GCC to compare to see what it does in this case, nor am sure whether repeating the same interrupt type should be accepted.

apazos updated this revision to Diff 152416.Jun 21 2018, 5:27 PM

Hi Simon, I have added a warning for repeated interrupt attributes.

apazos retitled this revision from [RISCV] Support for __attribute__((interrupt)) to [RISCV] Add support for interrupt attribute.Jun 21 2018, 5:28 PM
apazos edited the summary of this revision. (Show Details)
aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added inline comments.
lib/CodeGen/TargetInfo.cpp
8980

You can use const auto * here instead of repeating the type.

lib/Sema/SemaDeclAttr.cpp
5386

Please call checkAttributeNumArgs() instead; the error you're using is incorrect (it's used for variadic parameters where you receive more arguments than you expect).

5407

I don't think you need to perform this check -- I believe it's handled automatically (because you don't have custom parsing enabled).

test/Sema/riscv-interrupt-attr.c
19

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() {}
24

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.

27

I'm a bit surprised that this is not an error -- the argument is provided, so I don't know why this should be treated as acceptable.

apazos added inline comments.Jun 22 2018, 10:58 AM
lib/CodeGen/TargetInfo.cpp
8980

Thanks Aaron, will do the cleanup.

lib/Sema/SemaDeclAttr.cpp
5386

The argument is optional and at most one argument , I will use checkAttributeAtMostNumArgs instead

5407

I think need it. Will double check in the test.

test/Sema/riscv-interrupt-attr.c
19

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.

27

Good catch, In include/clang/Basic/Attr.td I mapped "" to the default machine mode.
I will change it to align with GCC behavior.

apazos updated this revision to Diff 152560.Jun 22 2018, 5:20 PM

Addressed review comments.

aaron.ballman added inline comments.Jun 25 2018, 5:30 AM
lib/Sema/SemaDeclAttr.cpp
5386

Ah, yeah, I didn't pick up the optional part when writing my comment; this LG.

5407

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?

test/Sema/riscv-interrupt-attr.c
19

Do all of these cases get diagnosed as being a repeated interrupt attribute? Should add them as test cases.

apazos added inline comments.Jun 28 2018, 4:25 PM
lib/Sema/SemaDeclAttr.cpp
5407

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.

test/Sema/riscv-interrupt-attr.c
19

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.

apazos updated this revision to Diff 153419.Jun 28 2018, 4:27 PM

Updated tests and removed Subjects from Attr.td

aaron.ballman added inline comments.Jun 29 2018, 5:35 AM
lib/Sema/SemaDeclAttr.cpp
5407

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?

5411

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.

test/Sema/riscv-interrupt-attr.c
19

Please document this in AttrDocs.td.

24

This question remains outstanding.

apazos added inline comments.Jun 29 2018, 11:02 AM
lib/Sema/SemaDeclAttr.cpp
5407

Sure I can add Subjects back in.
I will remove the helper function and use the simple check D->getFunctionType() == nullptr.

5411

hasFunctionProto also returns true for a function definition like this one
attribute((interrupt)) void foo1(int) {}.

test/Sema/riscv-interrupt-attr.c
19

Sure, I can add that info to the description.

24

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.

apazos updated this revision to Diff 153544.Jun 29 2018, 11:04 AM
aaron.ballman added inline comments.Jun 29 2018, 11:59 AM
lib/Sema/SemaDeclAttr.cpp
5411

That's correct, which is why I recommended the predicate the way I did. If the function has no prototype, we want to diagnose it. If the function has a prototype, we want to see how many parameters are listed and if there are more than zero parameters, we want to diagnose it.

test/Sema/riscv-interrupt-attr.c
24

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.

apazos added inline comments.Jul 5 2018, 1:35 PM
test/Sema/riscv-interrupt-attr.c
24

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.

aaron.ballman added inline comments.Jul 5 2018, 1:47 PM
test/Sema/riscv-interrupt-attr.c
24

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.

apazos added inline comments.Jul 5 2018, 2:16 PM
test/Sema/riscv-interrupt-attr.c
24

It does not drop, it compiles without warnings and it produces the code that is expected when interrupt attribute is set.

apazos updated this revision to Diff 154322.Jul 5 2018, 2:47 PM

Made the check/warning for prototype explicit.

aaron.ballman added inline comments.Jul 6 2018, 5:06 AM
test/Sema/riscv-interrupt-attr.c
24

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.

apazos updated this revision to Diff 154447.Jul 6 2018, 1:19 PM

Thanks Aaron, I appreciate you taking the time to review.
I have updated the test and removed the warning as discussed.

aaron.ballman accepted this revision.Jul 6 2018, 1:23 PM

LGTM! Do you need me to commit on your behalf, or do you have commit privileges?

This revision is now accepted and ready to land.Jul 6 2018, 1:23 PM
apazos added a comment.Jul 6 2018, 4:12 PM

Hi Aaron, I have commit rights and will commit if after Alex and other RISC-V colleagues get a chance to review too.

asb accepted this revision.Jul 19 2018, 6:58 AM

This looks good to me with two caveats

  • the tests don't seem to check that the "machine" is the default mode when the "interrupt" attribute has no arguments.
  • Although the conversion from RISCVInterruptAttr::user to "user" etc is trivial, it would probably be worth testing the emitted attributes are as expected?
apazos updated this revision to Diff 156343.Jul 19 2018, 12:45 PM
  • Rebased the patch.
  • Udpated test case to check for IR attribute.
asb accepted this revision.Jul 24 2018, 7:41 AM

Looks good to me.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2019, 1:53 PM