llvm dev - May 2018 - MSP430: interrupt vector number out of bounds error in v6/trunk (with patch)

When building with Clang for the MSP430 architecture against headers
distributed with TI MSP GCC, interrupt service routine
interrupt(vector_number) attribute is rejected:

   __attribute__ ((interrupt(USCI_A0_VECTOR))) void ISR(void) { }

   error: 'interrupt' attribute parameter 48 is out of bounds

This is due to the check in tools/clang/lib/Sema/SemaDeclAttr.cpp:5104

    unsigned Num = NumParams.getLimitedValue(255);
    if ((Num & 1) || Num > 30) {
        S.Diag(AL.getLoc(), diag::err_attribute_argument_out_of_bounds)
            << AL.getName() << (int)NumParams.getSExtValue()
            << NumParamsExpr->getSourceRange();
        return;
    }

Also, the __isr_ symbol is emitted with the vector number divided by 2.

    // Step 3: Emit ISR vector alias.
    unsigned Num = attr->getNumber() / 2;
    llvm::GlobalAlias::create(llvm::Function::ExternalLinkage,
                              "__isr_" + Twine(Num), F);

Eliminating the check causes a duplicate symbol error among __isr_
symbols, because odd and even vector numbers generate the same symbol.

Neither the check for even and under 30, nor the division by 2 is
necessary when compiling and assembling with the TI MSP GCC, whose
msp430*.h headers define vector number macros, like.

    #define EUSCI_A0_VECTOR    (48)   /* 0xFFF0 */

My guess is that the current LLVM code from above has been left over
from very old and deprecated community project MSPGCC, which is very
different from TI MSP GCC and is no longer supported.

To make interrupts work, I had to apply attached patch and then in the
app code, define the symbol manually (name of the symbol does not
matter) and allocate it to the section __interrupt_vector_ for that
vector number that is defined by the linker script shipped by TI MSP
GCC:

    #define VNUM(x) x // to drop parenthesis around *_VECTOR values from TI
    #define ISR_NAME(vect) CONCAT(ISR_, vect)
    #define ISR(vect) \
        void ISR_NAME(VNUM vect)(void); \
        __attribute__((section("__interrupt_vector_" STR(VNUM vect)),
aligned(2))) \
        void(*CONCAT(__vector_, VNUM vect))(void) = ISR_NAME(VNUM vect); \
        __attribute__ ((interrupt(vect))) void ISR_NAME(VNUM vect)(void)

   ISR(USCI_A0_VECTOR) { }

Perhaps a better fix would be to emit the alias and allocate it in the
correct section (by name, as I did above manually), but I couldn't find
a way to do this in LLVM. With the current patch, the vector number is
unused, so might need to be removed entirely.

Note that with TI MSP GCC, it is not necessary to define the symbol
and allocate it manually; the following just works:

    #define ISR(vect) \
        __attribute__ ((interrupt(vect))) void ISR_NAME(VNUM vect)(void)

    ISR(USCI_A0_VECTOR) { }

Same info here:
https://stackoverflow.com/questions/33266132/how-to-define-an-interrupt-service-routine-for-msp430-with-llvm-clanggcc/33266133
-------------- next part --------------
>From 241036d9abc5b754d5ef8afbb6cbf28c6b40768f Mon Sep 17 00:00:00 2001
From: Alexei Colin <ac at alexeicolin.com>
Date: Fri, 25 May 2018 18:16:56 -0400
Subject: [PATCH] MSP430: accept all interrupt numbers and don't emit alias

This works around compile error that interrupt vector number in the
interrupt() attribute is out of bounds, when building with TI MSP GCC.

Despite both GCC and Clang using the same linker script, for Clang, we
need to manually create a symbol and allocate it in the per-vector
section (named with a name that matches the names that the linker script
expects). Note that Clang creates __isr_* symbols automatically, but
these are only interpreted by the deprecated MSPGCC compiler/linker, not
by TI GCC for MSP. See more on this below.

Clang creates __isr_* symbols for MSPGCC (see lib/CodeGen/TargetInfo.cpp
MSP430TargetCodeGenInfo::SetTargetAttributes), these are not relevant
for TI GCC, but they are not entirely harmless. They cause a problem
because Clang generates __isr_N where N is divided by 2 (because MSPGCC
wants it that way, I guess). So, if we pass a vector number, the odd and
even vector numbers will generate the same symbol and therefore linking
will fail with a multiply-defined symbol error. As a workaround we pass
the doubled value.

There is one more problem: Clang rejects vector numbers that are odd or
above 30. So, this problem with the above problem form a catch-22
situation: if we pass N/2 then the atttribute is accepted, but we get
multiply-defined values; if we pass N, it's rejected; and if we pass
N*2, it's rejected. So, the workaround is to not emit the __isr symbol
at all, and to accept the full range of interrupt vector numbers.
---
 lib/CodeGen/TargetInfo.cpp | 5 -----
 lib/Sema/SemaDeclAttr.cpp  | 2 +-
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/lib/CodeGen/TargetInfo.cpp b/lib/CodeGen/TargetInfo.cpp
index 4e25c72cfb..a42248edcf 100644
--- a/lib/CodeGen/TargetInfo.cpp
+++ b/lib/CodeGen/TargetInfo.cpp
@@ -6655,11 +6655,6 @@ void MSP430TargetCodeGenInfo::setTargetAttributes(
 
       // Step 2: Add attributes goodness.
       F->addFnAttr(llvm::Attribute::NoInline);
-
-      // Step 3: Emit ISR vector alias.
-      unsigned Num = attr->getNumber() / 2;
-      llvm::GlobalAlias::create(llvm::Function::ExternalLinkage,
-                                "__isr_" + Twine(Num), F);
     }
   }
 }
diff --git a/lib/Sema/SemaDeclAttr.cpp b/lib/Sema/SemaDeclAttr.cpp
index e4532a7e67..094087c886 100644
--- a/lib/Sema/SemaDeclAttr.cpp
+++ b/lib/Sema/SemaDeclAttr.cpp
@@ -5101,7 +5101,7 @@ static void handleMSP430InterruptAttr(Sema &S, Decl
*D,
   }
 
   unsigned Num = NumParams.getLimitedValue(255);
-  if ((Num & 1) || Num > 30) {
+  if (Num > 55) {
     S.Diag(AL.getLoc(), diag::err_attribute_argument_out_of_bounds)
       << AL.getName() << (int)NumParams.getSExtValue()
       << NumParamsExpr->getSourceRange();
-- 
2.17.0

For some time I've been using this defines that allowed me compile the code
using both clang and gcc:

#ifdef __clang__
#define ISR_BEGIN(id) __attribute__((interrupt(0)))
#else
#define ISR_BEGIN(id) __attribute__((interrupt(id), used))
#endif

#ifdef __clang__
#define ISR_END(name)                                                          \
    __attribute__((section("__interrupt_vector_" #name), aligned(2)))
void (   \
        *__vector_##name)(void) = name;
#else
#define ISR_END(name)
#endif

And I use it like this

ISR_BEGIN(USCI_A0_VECTOR)
void usci_a0(void)
{
}
ISR_END(usci_a0)

ISR_BEGIN(USCI_B0_VECTOR)
void usci_b0(void)
{
}
ISR_END(usci_b0)

But of course fixing it in the compiler would be great.