[PATCH] ARM: Implement __attribute__((interrupt(...)))

Aaron Ballman aaron at aaronballman.com
Tue Sep 24 12:00:03 PDT 2013


Looking really good, just a few minor nits and a question:

> commit 538466ecfae45d96bb80874adbc7ea9292b0061b
> Author: Tim Northover <tnorthover at apple.com>
> Date:   Fri Sep 20 17:16:32 2013 +0100
>
>     Implement ARM interrupt attribute
>
> diff --git a/docs/LanguageExtensions.rst b/docs/LanguageExtensions.rst
> index f6a7a4e..da4bb16 100644
> --- a/docs/LanguageExtensions.rst
> +++ b/docs/LanguageExtensions.rst
> @@ -1843,6 +1843,48 @@ Which compiles to (on X86-32):
>            movl    %gs:(%eax), %eax
>            ret
>
> +ARM Language Extensions
> +-----------------------
> +
> +Interrupt attribute
> +^^^^^^^^^^^^^^^^^^^
> +
> +Clang supports the GNU style ``__attribite__((interrupt("TYPE")))`` attribute on
> +ARM targets. This attribute may be attached to a function definiton and
> +instructs the backend to generate appropriate function entry/exit code so that
> +it can be used directly as an interrupt service routine.
> +
> + The parameter passed to the interrupt attribute is optional, but if
> +provided it must be a string literal with one of the following values: "IRQ",
> +"FIQ", "SWI", "ABORT", "UNDEF".
> +
> +The semantics are as follows:
> +
> +- If the function is AAPCS, Clang instructs the backend to realign the stack to
> +  8 bytes on entry. This is a general requirement of the AAPCS at public
> +  interfaces, but may not hold when an exception is taken. Doing this allows
> +  other AAPCS functions to be called.
> +- If the CPU is M-class this is all that needs to be done since the architecture
> +  itself is designed in such a way that functions obeying the normal AAPCS ABI
> +  constraints are valid exception handlers.
> +- If the CPU is not M-class, the prologue and epilogue are modified to save all
> +  non-banked registers that are used, so that upon return the user-mode state
> +  will not be corrupted. Note that to avoid unnecessary overhead, only
> +  general-purpose (integer) registers are saved in this way. If VFP operations
> +  are needed, that state must be saved manually.
> +
> +  Specifically, interrupt kinds other than "FIQ" will save all core registers
> +  except "lr" and "sp". "FIQ" interrupts will save r0-r7.
> +- If the CPU is not M-class, the return instruction is changed to one of the
> +  canonical sequences permitted by the architecture for exception return. Where
> +  possible the function itself will make the necessary "lr" adjustments so that
> +  the "preferred return address" is selected.
> +
> +  Unfortunately the compiler is unable to make this guarantee for nn "UNDEF"
> +  handler, where the offset from "lr" to the preferred return address depends on
> +  the execution state of the code which generated the exception. In this case
> +  a sequence equivalent to "movs pc, lr" will be used.
> +
>  Extensions for Static Analysis
>  ==============================
>
> diff --git a/include/clang/Basic/Attr.td b/include/clang/Basic/Attr.td
> index 5681daf..852667d 100644
> --- a/include/clang/Basic/Attr.td
> +++ b/include/clang/Basic/Attr.td
> @@ -210,6 +210,14 @@ def Annotate : InheritableParamAttr {
>    let Args = [StringArgument<"Annotation">];
>  }
>
> +def ARMInterrupt : InheritableAttr, TargetSpecificAttr {
> +  let Spellings = [GNU<"interrupt">];
> +  let Args = [EnumArgument<"Interrupt", "InterruptType",
> +                           ["IRQ", "FIQ", "SWI", "ABORT", "UNDEF", ""],
> +                           ["IRQ", "FIQ", "SWI", "ABORT", "UNDEF", "Generic"],
> +                           1>];
> +}
> +
>  def AsmLabel : InheritableAttr {
>    let Spellings = [];
>    let Args = [StringArgument<"Label">];
> diff --git a/lib/CodeGen/TargetInfo.cpp b/lib/CodeGen/TargetInfo.cpp
> index 0706a28..01efd11 100644
> --- a/lib/CodeGen/TargetInfo.cpp
> +++ b/lib/CodeGen/TargetInfo.cpp
> @@ -3056,9 +3056,9 @@ public:
>              Env == "android" || Env == "androideabi");
>    }
>
> -private:
>    ABIKind getABIKind() const { return Kind; }
>
> +private:
>    ABIArgInfo classifyReturnType(QualType RetTy) const;
>    ABIArgInfo classifyArgumentType(QualType RetTy, int *VFPRegs,
>                                    unsigned &AllocatedVFP,
> @@ -3105,6 +3105,45 @@ public:
>      if (getABIInfo().isEABI()) return 88;
>      return TargetCodeGenInfo::getSizeOfUnwindException();
>    }
> +
> +  void SetTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
> +                           CodeGen::CodeGenModule &CGM) const {
> +    const FunctionDecl *FD = dyn_cast<FunctionDecl>(D);
> +    if (!FD)
> +      return;
> +
> +    const ARMInterruptAttr *Attr = FD->getAttr<ARMInterruptAttr>();
> +    if (!Attr)
> +      return;
> +
> +    const char *Kind;
> +    switch (Attr->getInterrupt()) {
> +    case ARMInterruptAttr::Generic: Kind = ""; break;
> +    case ARMInterruptAttr::IRQ:     Kind = "IRQ"; break;
> +    case ARMInterruptAttr::FIQ:     Kind = "FIQ"; break;
> +    case ARMInterruptAttr::SWI:     Kind = "SWI"; break;
> +    case ARMInterruptAttr::ABORT:   Kind = "ABORT"; break;
> +    case ARMInterruptAttr::UNDEF:   Kind = "UNDEF"; break;
> +    }
> +
> +    llvm::Function *Fn = cast<llvm::Function>(GV);
> +
> +    Fn->addFnAttr("interrupt", Kind);
> +
> +    if (cast<ARMABIInfo>(getABIInfo()).getABIKind() == ARMABIInfo::APCS)
> +      return;
> +
> +    // AAPCS guarantees that sp will be 8-byte aligned on any public interface,
> +    // however this is not necessarily true on taking any interrupt. Instruct
> +    // the backend to perform a realignment as part of the function prologue.
> +    llvm::AttrBuilder B;
> +    B.addStackAlignmentAttr(8);
> +    Fn->addAttributes(llvm::AttributeSet::FunctionIndex,
> +                      llvm::AttributeSet::get(CGM.getLLVMContext(),
> +                                              llvm::AttributeSet::FunctionIndex,
> +                                              B));
> +  }
> +
>  };
>
>  }
> diff --git a/lib/Sema/SemaDeclAttr.cpp b/lib/Sema/SemaDeclAttr.cpp
> index 950c8df..c6337a5 100644
> --- a/lib/Sema/SemaDeclAttr.cpp
> +++ b/lib/Sema/SemaDeclAttr.cpp
> @@ -289,7 +289,7 @@ static bool checkFunctionOrMethodArgumentIndex(Sema &S, const Decl *D,
>  /// literal.
>  bool Sema::checkStringLiteralArgumentAttr(const AttributeList &Attr,
>                                            unsigned ArgNum, StringRef &Str,
> -                                          SourceLocation *ArgLocation = 0) {
> +                                          SourceLocation *ArgLocation) {
>    // Look for identifiers. If we have one emit a hint to fix it to a literal.
>    if (Attr.isArgIdent(ArgNum)) {
>      IdentifierLoc *Loc = Attr.getArgAsIdent(ArgNum);
> diff --git a/lib/Sema/TargetAttributesSema.cpp b/lib/Sema/TargetAttributesSema.cpp
> index 7db0b24..cae8fa6 100644
> --- a/lib/Sema/TargetAttributesSema.cpp
> +++ b/lib/Sema/TargetAttributesSema.cpp
> @@ -26,6 +26,50 @@ bool TargetAttributesSema::ProcessDeclAttribute(Scope *scope, Decl *D,
>    return false;
>  }
>
> +static void HandleARMInterruptAttr(Decl *d,
> +                                   const AttributeList &Attr, Sema &S) {
> +  // Check the attribute arguments.
> +  if (Attr.getNumArgs() > 1) {
> +    S.Diag(Attr.getLoc(), diag::err_attribute_too_many_arguments)
> +        << 1;
> +    return;
> +  }
> +
> +  StringRef Str;
> +  SourceLocation ArgLoc;
> +
> +  if (Attr.getNumArgs() == 0) {
> +    Str = "";
> +  } else if (!S.checkStringLiteralArgumentAttr(Attr, 0, Str, &ArgLoc))
> +    return;

Spurious set of curly braces for the if statement.

> +
> +  ARMInterruptAttr::InterruptType Kind;
> +  if (!ARMInterruptAttr::ConvertStrToInterruptType(Str, Kind)) {
> +    S.Diag(Attr.getLoc(), diag::warn_attribute_type_not_supported)
> +        << "interrupt" << Str << ArgLoc;

This diagnostic should be using Attr.getName() instead of spelling the
attribute directly.

> +    return;
> +  }
> +
> +  unsigned Index = Attr.getAttributeSpellingListIndex();
> +  d->addAttr(::new (S.Context)
> +             ARMInterruptAttr(Attr.getLoc(), S.Context, Kind, Index));
> +}
> +
> +namespace {
> +  class ARMAttributesSema : public TargetAttributesSema {
> +  public:
> +    ARMAttributesSema() { }
> +    bool ProcessDeclAttribute(Scope *scope, Decl *D,
> +                              const AttributeList &Attr, Sema &S) const {
> +      if (Attr.getName()->getName() == "interrupt") {

Since this is a GNU attribute, can it be spelled __interrupt__ as
well, and be expected to work?

> +        HandleARMInterruptAttr(D, Attr, S);
> +        return true;
> +      }
> +      return false;
> +    }
> +  };
> +}
> +
>  static void HandleMSP430InterruptAttr(Decl *d,
>                                        const AttributeList &Attr, Sema &S) {
>      // Check the attribute arguments.
> @@ -292,6 +336,9 @@ const TargetAttributesSema &Sema::getTargetAttributesSema() const {
>
>    const llvm::Triple &Triple(Context.getTargetInfo().getTriple());
>    switch (Triple.getArch()) {
> +  case llvm::Triple::arm:
> +  case llvm::Triple::thumb:
> +    return *(TheTargetAttributesSema = new ARMAttributesSema);
>    case llvm::Triple::msp430:
>      return *(TheTargetAttributesSema = new MSP430AttributesSema);
>    case llvm::Triple::x86:
> diff --git a/test/CodeGen/arm-interrupt-attr.c b/test/CodeGen/arm-interrupt-attr.c
> new file mode 100644
> index 0000000..73f1cfe
> --- /dev/null
> +++ b/test/CodeGen/arm-interrupt-attr.c
> @@ -0,0 +1,38 @@
> +// RUN: %clang_cc1 -triple thumb-apple-darwin -target-abi aapcs -target-cpu cortex-m3 -emit-llvm -o - %s | FileCheck %s
> +// RUN: %clang_cc1 -triple arm-apple-darwin -target-abi apcs-gnu -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-APCS
> +
> +__attribute__((interrupt)) void test_generic_interrupt() {
> +  // CHECK: define arm_aapcscc void @test_generic_interrupt() [[GENERIC_ATTR:#[0-9]+]]
> +
> +  // CHECK-APCS: define void @test_generic_interrupt() [[GENERIC_ATTR:#[0-9]+]]
> +}
> +
> +__attribute__((interrupt("IRQ"))) void test_irq_interrupt() {
> +  // CHECK: define arm_aapcscc void @test_irq_interrupt() [[IRQ_ATTR:#[0-9]+]]
> +}
> +
> +__attribute__((interrupt("FIQ"))) void test_fiq_interrupt() {
> +  // CHECK: define arm_aapcscc void @test_fiq_interrupt() [[FIQ_ATTR:#[0-9]+]]
> +}
> +
> +__attribute__((interrupt("SWI"))) void test_swi_interrupt() {
> +  // CHECK: define arm_aapcscc void @test_swi_interrupt() [[SWI_ATTR:#[0-9]+]]
> +}
> +
> +__attribute__((interrupt("ABORT"))) void test_abort_interrupt() {
> +  // CHECK: define arm_aapcscc void @test_abort_interrupt() [[ABORT_ATTR:#[0-9]+]]
> +}
> +
> +
> +__attribute__((interrupt("UNDEF"))) void test_undef_interrupt() {
> +  // CHECK: define arm_aapcscc void @test_undef_interrupt() [[UNDEF_ATTR:#[0-9]+]]
> +}
> +
> +// CHECK: attributes [[GENERIC_ATTR]] = { nounwind alignstack=8 {{"interrupt"[^=]}}
> +// CHECK: attributes [[IRQ_ATTR]] = { nounwind alignstack=8 "interrupt"="IRQ"
> +// CHECK: attributes [[FIQ_ATTR]] = { nounwind alignstack=8 "interrupt"="FIQ"
> +// CHECK: attributes [[SWI_ATTR]] = { nounwind alignstack=8 "interrupt"="SWI"
> +// CHECK: attributes [[ABORT_ATTR]] = { nounwind alignstack=8 "interrupt"="ABORT"
> +// CHECK: attributes [[UNDEF_ATTR]] = { nounwind alignstack=8 "interrupt"="UNDEF"
> +
> +// CHECK-APCS: attributes [[GENERIC_ATTR]] = { nounwind "interrupt"
> diff --git a/test/Sema/arm-interrupt-attr.c b/test/Sema/arm-interrupt-attr.c
> new file mode 100644
> index 0000000..a267137
> --- /dev/null
> +++ b/test/Sema/arm-interrupt-attr.c
> @@ -0,0 +1,16 @@
> +// RUN: %clang_cc1 %s -triple arm-apple-darwin -verify -fsyntax-only
> +
> +__attribute__((interrupt(IRQ))) void foo() {} // expected-error {{'interrupt' attribute requires a string}}
> +__attribute__((interrupt("irq"))) void foo1() {} // expected-warning {{interrupt attribute argument not supported: irq}}
> +
> +__attribute__((interrupt("IRQ", 1))) void foo2() {} // expected-error {{attribute takes no more than 1 argument}}
> +
> +__attribute__((interrupt("IRQ"))) void foo3() {}
> +__attribute__((interrupt("FIQ"))) void foo4() {}
> +__attribute__((interrupt("SWI"))) void foo5() {}
> +__attribute__((interrupt("ABORT"))) void foo6() {}
> +__attribute__((interrupt("UNDEF"))) void foo7() {}
> +
> +__attribute__((interrupt)) void foo8() {}
> +__attribute__((interrupt())) void foo9() {}
> +__attribute__((interrupt(""))) void foo10() {}
>


~Aaron

On Tue, Sep 24, 2013 at 9:48 AM, Tim Northover <t.p.northover at gmail.com> wrote:
> Ah, that all makes sense now. How does this version look?
>
> I think I'm using as much auto-generated Sema as possible, made the
> other corrections (as I understand them) and stopped adding
> attribute((used)) -- it seems entirely reasonable that the programmer
> can put that in if he turns out to need it.
>
> Tim.



More information about the llvm-commits mailing list