Page MenuHomePhabricator

[MSP430] Improve support of 'interrupt' attribute
ClosedPublic

Authored by krisb on Jan 14 2019, 6:30 AM.

Details

Summary
  • Accept as an argument constants in range 0..63 (aligned with TI headers and linker scripts provided with TI GCC toolchain).
  • Emit function attribute 'interrupt'='xx' instead of aliases (used in the backend to create a section for particular interrupt vector).
  • Add more diagnostics.

Diff Detail

Repository
rL LLVM

Event Timeline

krisb created this revision.Jan 14 2019, 6:30 AM
krisb edited the summary of this revision. (Show Details)Jan 14 2019, 6:31 AM
krisb added a reviewer: asl.
krisb edited the summary of this revision. (Show Details)
grimar added a subscriber: grimar.Jan 14 2019, 6:54 AM
grimar added inline comments.
lib/Sema/SemaDeclAttr.cpp
5381 ↗(On Diff #181546)

nit: code comments usually have a dot . at the end of the sentence.

aaron.ballman added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
277–280 ↗(On Diff #181546)

I'd fold this in to the preceding diagnostics using %select{}. Perhaps the new diagnostic name should be warn_interrupt_attribute_invalid_subject or some such?

lib/CodeGen/TargetInfo.cpp
6777 ↗(On Diff #181546)

Might as well switch this to using const auto *; also, the variable should be renamed to not conflict with the name of a type.

lib/Sema/SemaDeclAttr.cpp
5400 ↗(On Diff #181546)

Missing a full stop here as well.

krisb added inline comments.Jan 15 2019, 12:55 PM
include/clang/Basic/DiagnosticSemaKinds.td
277–280 ↗(On Diff #181546)

I thought about this but decided to not touch other targets in this patch. So, I'd prefer to make a follow-up patch to refactor these 3 diagnostics (for mips, riscv and msp430) to a single. What do you think?

aaron.ballman added inline comments.Jan 15 2019, 1:00 PM
include/clang/Basic/DiagnosticSemaKinds.td
277–280 ↗(On Diff #181546)

I'm fine with that, thanks!

krisb updated this revision to Diff 181866.Jan 15 2019, 1:17 PM

Addressed feedback.

asl accepted this revision.Jan 16 2019, 5:44 AM
This revision is now accepted and ready to land.Jan 16 2019, 5:44 AM
This revision was automatically updated to reflect the committed changes.
aaron.ballman added inline comments.Jan 16 2019, 5:51 AM
lib/Sema/SemaDeclAttr.cpp
5388 ↗(On Diff #181866)

What should happen if the function does not have a prototype? Should that be diagnosed? e.g., a K&R C function in C mode accepts an arbitrary number of arguments (which can include zero).

krisb added inline comments.Jan 16 2019, 7:29 AM
lib/Sema/SemaDeclAttr.cpp
5388 ↗(On Diff #181866)

Currently, in a case like this

__attribute__((interrupt(0))) void foo();

void foo(int i) {
  i++;
}

frontend reports no warnings, but backend fails with the following error:

fatal error: error in backend: ISRs cannot have arguments

But definitely, that should be diagnosed early.
I'll take care of this. Thanks!