Page MenuHomePhabricator

[mips] Interrupt attribute support.
ClosedPublic

Authored by sdardis on Jun 29 2015, 6:25 AM.

Diff Detail

Event Timeline

sdardis updated this revision to Diff 28666.Jun 29 2015, 6:25 AM
sdardis retitled this revision from to [mips] Interrupt attribute support..
sdardis updated this object.
sdardis edited the test plan for this revision. (Show Details)
sdardis added a reviewer: dsanders.
sdardis added a subscriber: Unknown Object (MLST).
dsanders accepted this revision.Sep 7 2015, 5:17 AM
dsanders edited edge metadata.

LGTM with a few nits

include/clang/Basic/Attr.td
864–865

if -> If
ARMInterrupts's -> ARMInterrupt's

Also, you should update the comments for ARMInterrupt and MSP430Interrupt to mention MipsInterrupt.

include/clang/Basic/AttrDocs.td
726–727

Aside from "eic" shouldn't these be "vector=..."?

lib/Sema/SemaDeclAttr.cpp
4479

I think you also need to cover aarch64, thumb, etc. It's probably best to leave it as an else clause and add mips above it.

This revision is now accepted and ready to land.Sep 7 2015, 5:17 AM
aaron.ballman requested changes to this revision.Sep 7 2015, 6:10 AM
aaron.ballman added a reviewer: aaron.ballman.
aaron.ballman added a subscriber: aaron.ballman.

Missing tests for the semantic warnings. For instance, I don't see any tests exercising err_attribute_too_many_arguments or warn_attribute_type_not_supported.

~Aaron

This revision now requires changes to proceed.Sep 7 2015, 6:10 AM
sdardis updated this revision to Diff 38102.Oct 22 2015, 2:25 AM
sdardis updated this object.
sdardis edited edge metadata.
sdardis marked 3 inline comments as done.

Nits addressed and XFAIL tests added for functions having arguments and the interrupt attribute.

Test added to cover semantic warnings.

Rejects functions with mips16 & interrupt attributes.

Text in the bullet points of MipsInterruptDocs section expanded.

sdardis updated this revision to Diff 38533.EditedOct 27 2015, 6:14 AM
sdardis edited edge metadata.

Updated tests along the lines of what vkalintiris did for corresponding llvm part as of r251295.

aaron.ballman requested changes to this revision.Nov 1 2015, 5:08 PM
aaron.ballman edited edge metadata.

This is coming along nicely! There are some minor nits that should be trivial to fix, but the fatal errors need to become semantic errors, and some tests are missing.

include/clang/Basic/AttrDocs.td
721

s/ot/to

725

Comma after "By default"

730

Comma after mode.

746

Comma after prologue.

749

The function return?

lib/CodeGen/TargetInfo.cpp
5897

No space between * and Attr. May want to run clang-format over the patch.

5902

This should be a semantic error for improved diagnostic behavior.

5906

Same here. Also, missing a semantic test for this.

lib/Sema/SemaDeclAttr.cpp
4431

Please have Str in quotes in the diagnostic (since it's something the user wrote).

4435

Sink Index into the constructor arguments.

test/CodeGen/mips-interrupt-attr-arg.c
5 ↗(On Diff #38533)

This should be folded into the other semantic test when this becomes a semantic error instead of a hard failure.

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

Also missing semantic tests for placing the attribute on something other than a function (which I presume should be an error?).

This revision now requires changes to proceed.Nov 1 2015, 5:08 PM
sdardis updated this revision to Diff 40382.Nov 17 2015, 3:42 AM
sdardis edited edge metadata.
sdardis marked 12 inline comments as done.

Updated documentation text.

Switched the various hard failures to semantic warnings/failures.

Strangely, the 'interrupt' attribute on a non-function defaults to a warning.

Thanks.

aaron.ballman added inline comments.Nov 17 2015, 6:31 AM
include/clang/Basic/DiagnosticSemaKinds.td
255

I would combine these, and name the result so that it is obvious it relates to the MIPS interrupt attribute. This can then become: "MIPS 'interrupt' attribute only applies to functions %select{with no parameters|a 'void' return type}0" Generally speaking, we make these warnings instead of errors (though the choice is obviously yours), and add them to the IgnoredAttributes group (so you warn, and then never attach the attribute to the declaration).

lib/Sema/SemaDeclAttr.cpp
4441

No need for this; you can pass any NamedDecl * into Diag() and it will get quoted and displayed properly. Since you've already checked that it's a function or method, you can simply use cast<NamedDecl>(D) when needed.

4448

No need for the extra set of parens.

4453

Please use checkAttrMutualExclusion() instead. Also, you need to add a similar check to the Mips16 attribute handler to ensure the mutual exclusion is properly checked. Should have semantic test cases for both orderings:

__attribute__((mips16)) __attribute__((interrupt)) void f();
__attribute__((interrupt)) __attribute__((mips16)) void g();

What about the nomips16 attribute? Should this be mutually exclusive as well?

sdardis updated this revision to Diff 40782.Nov 20 2015, 8:15 AM
sdardis edited edge metadata.
sdardis marked 4 inline comments as done.

Updated comments, used cast as suggested, added mutual exclusion check for mips16+interrupt combination.

Extended mips16/nomips16 attribute handlers for mutual exclusion, add to pre-existing test.

Thanks.

aaron.ballman added inline comments.Nov 20 2015, 11:10 AM
include/clang/Basic/DiagnosticSemaKinds.td
256

No need for \'; you can use ' directly.

I think the wording is still a bit awkward. Function declarations generally don't "take arguments", but have parameters, and "the void return type" reads weird. I think:

"MIPS 'interrupt' attribute only applies to functions with %select{no parameters|a 'void' return type}0"

would be an improvement.

lib/Sema/SemaDeclAttr.cpp
4439

No need for \', you can use ' directly.

4462

No need for \', can use ' directly.

4484

This is a good change, but should be as part of a separate patch since it has nothing to do with MIPS interrupt.

4495

Same with this.

test/Sema/mips16_attr_allowed.c
22 ↗(On Diff #40782)

This should be part of a separate patch.

sdardis updated this revision to Diff 41041.Nov 24 2015, 7:02 AM

Updated text of return type/parameters warning to Aaron's suggestion.
Dropped '\' escape characters from warning/error text.
Removed mips16/nomips16 check.
Tweak of comment in handleMipsInterruptAttr.

aaron.ballman accepted this revision.Nov 24 2015, 12:35 PM
aaron.ballman edited edge metadata.

LGTM, once one minor nit is fixed. Thank you for working on this!

include/clang/Basic/DiagnosticSemaKinds.td
257

Spurious \' around 'void'

This revision is now accepted and ready to land.Nov 24 2015, 12:35 PM
sdardis updated this revision to Diff 41283.Nov 27 2015, 2:03 AM
sdardis edited edge metadata.

Nit addressed.

Daniel or Aaron, can one of you commit on my behalf? Thanks.

Aaron, thanks for the review.

dsanders closed this revision.Nov 27 2015, 9:42 AM