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

Aaron Ballman aaron at aaronballman.com
Mon Sep 23 09:06:18 PDT 2013


Comments on the clang side of the patch:

> commit f0517be612b7e062673e273146a7833a178c15a5
> 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..e80c653 100644
> --- a/include/clang/Basic/Attr.td
> +++ b/include/clang/Basic/Attr.td
> @@ -210,6 +210,13 @@ def Annotate : InheritableParamAttr {
>    let Args = [StringArgument<"Annotation">];
>  }
>
> +def ARMInterrupt : InheritableAttr, TargetSpecificAttr {
> +  let Spellings = [];

This has a spelling of "interrupt" and it's a GNU attribute.

> +  let Args = [EnumArgument<"Interrupt", "InterruptType",
> +                           ["IRQ", "FIQ", "SWI", "ABORT", "UNDEF", ""],
> +                           ["IRQ", "FIQ", "SWI", "ABORT", "UNDEF",
"Generic"]>];

This should be flagged as an optional argument.

> +}
> +
>  def AsmLabel : InheritableAttr {
>    let Spellings = [];
>    let Args = [StringArgument<"Label">];
> diff --git a/include/clang/Basic/DiagnosticSemaKinds.td
b/include/clang/Basic/DiagnosticSemaKinds.td
> index 8486ff3..ba71e28 100644
> --- a/include/clang/Basic/DiagnosticSemaKinds.td
> +++ b/include/clang/Basic/DiagnosticSemaKinds.td
> @@ -1818,6 +1818,8 @@ def err_init_priority_object_attr : Error<
>    "of objects of class type">;
>  def err_attribute_argument_vec_type_hint : Error<
>    "invalid attribute argument %0 - expecting a vector or vectorizable
scalar type">;
> +def err_attribute_argument_invalid : Error<
> +  "'%0' attribute parameter '%1' is invalid">;

This is not needed (more below).

>  def err_attribute_argument_out_of_bounds : Error<
>    "'%0' attribute parameter %1 is out of bounds">;
>  def err_attribute_uuid_malformed_guid : Error<
> diff --git a/lib/CodeGen/TargetInfo.cpp b/lib/CodeGen/TargetInfo.cpp
> index 0706a28..872500e 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,43 @@ 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;

Return should be on its own line.

> +
> +    const ARMInterruptAttr *Attr = FD->getAttr<ARMInterruptAttr>();
> +    if (!Attr) return;

Same here.

> +
> +    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;

This could be hoisted higher in the function.

> +
> +    // 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..0cc77fc 100644
> --- a/lib/Sema/TargetAttributesSema.cpp
> +++ b/lib/Sema/TargetAttributesSema.cpp
> @@ -26,6 +26,60 @@ 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;
> +
> +  typedef ARMInterruptAttr::InterruptType IntKind;
> +
> +  Optional<IntKind> Kind = llvm::StringSwitch<Optional<IntKind> >(Str)
> +                               .Case("", ARMInterruptAttr::Generic)
> +                               .Case("IRQ", ARMInterruptAttr::IRQ)
> +                               .Case("FIQ", ARMInterruptAttr::FIQ)
> +                               .Case("SWI", ARMInterruptAttr::SWI)
> +                               .Case("ABORT", ARMInterruptAttr::ABORT)
> +                               .Case("UNDEF", ARMInterruptAttr::UNDEF)
> +                               .Default(Optional<IntKind>());

There is code auto-generated for you that does this, named something like
ConvertStrToInterruptType; you should use that, and get rid of the custom
logic for error handling.

> +
> +  if (!Kind) {
> +    S.Diag(Attr.getLoc(), diag::err_attribute_argument_invalid)
> +        << "interrupt" << Str << ArgLoc;
> +    return;
> +  }
> +
> +  d->addAttr(::new (S.Context)
> +             ARMInterruptAttr(Attr.getLoc(), S.Context,
Kind.getValue()));
> +  d->addAttr(::new (S.Context) UsedAttr(Attr.getLoc(), S.Context));

This breaks the as-written attributes.  Also, failing to set the spelling
list index (I see other places are failing to do this for target attributes
as well, but it should be added for new code.

> +}
> +
> +namespace {
> +  class ARMAttributesSema : public TargetAttributesSema {
> +  public:
> +    ARMAttributesSema() { }
> +    bool ProcessDeclAttribute(Scope *scope, Decl *D,
> +                              const AttributeList &Attr, Sema &S) const {
> +      if (Attr.getName()->getName() == "interrupt") {
> +        HandleARMInterruptAttr(D, Attr, S);
> +        return true;
> +      }
> +      return false;
> +    }
> +  };
> +}
> +
>  static void HandleMSP430InterruptAttr(Decl *d,
>                                        const AttributeList &Attr, Sema
&S) {
>      // Check the attribute arguments.
> @@ -292,6 +346,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..cc06526
> --- /dev/null
> +++ b/test/Sema/arm-interrupt-attr.c
> @@ -0,0 +1,10 @@
> +// 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-error
{{'interrupt' attribute parameter 'irq' is invalid}}
> +
> +__attribute__((interrupt("IRQ", 1))) void foo2() {} // expected-error
{{attribute takes no more than 1 argument}}
> +
> +__attribute__((interrupt("IRQ"))) void foo3() {}
> +__attribute__((interrupt)) void foo4() {}
> +__attribute__((interrupt())) void foo5() {}
>

~Aaron


On Mon, Sep 23, 2013 at 11:26 AM, Tim Northover <t.p.northover at gmail.com>wrote:

> Hi all,
>
> These two patches should implement something very much like the GNU
> extension allowing C-functions to be used as interrupt handlers on
> ARM. This has a couple of high-level effects: special return
> instructions (except for M-class CPUs) and many more callee-saved
> registers.
>
> Some issues that may be debatable:
>
> 1. I don't automatically save d0-d31. That would be insane (and also
> isn't what GCC does).
> 2. On the LLVM side, I've modelled it as an attribute rather than a
> calling-convention. Either could be made to work, but "cc(64)" seemed
> a little less obvious and ARM's already got more than its fair share
> of official calling conventions.
> 3. I completely co-opted the Thumb2 SUBS_PC_LR to match what CodeGen
> actually expects of a return rather than, necessarily, reality.
>
> Any comments or suggestions would be appreciated.
>
> Cheers.
>
> Tim.
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130923/7069fa3b/attachment.html>


More information about the llvm-commits mailing list