[MSP430] Improve support of 'interrupt' attribute

[krisb]

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.

[grimar]

lib/Sema/SemaDeclAttr.cpp

5381 ↗

(On Diff #181546)

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

[aaron.ballman]

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]

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]

include/clang/Basic/DiagnosticSemaKinds.td

277–280 ↗

(On Diff #181546)

I'm fine with that, thanks!

[krisb]

Addressed feedback.

[aaron.ballman]

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]

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!

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

 def warn_riscv_repeated_interrupt_attribute : Warning<
   "repeated RISC-V 'interrupt' attribute">, InGroup<IgnoredAttributes>;
 def note_riscv_repeated_interrupt_attribute : Note<
   "repeated RISC-V 'interrupt' attribute is here">;
 def warn_riscv_interrupt_attribute : Warning<
    "RISC-V 'interrupt' attribute only applies to functions that have "
    "%select{no parameters|a 'void' return type}0">,
    InGroup<IgnoredAttributes>;
+def warn_msp430_interrupt_attribute : Warning<
+   "MSP430 'interrupt' attribute only applies to functions that have "
+   "%select{no parameters|a 'void' return type}0">,
+   InGroup<IgnoredAttributes>;
 def warn_unused_parameter : Warning<"unused parameter %0">,
   InGroup<UnusedParameter>, DefaultIgnore;
 def warn_unused_variable : Warning<"unused variable %0">,
   InGroup<UnusedVariable>, DefaultIgnore;
 def warn_unused_local_typedef : Warning<
   "unused %select{typedef|type alias}0 %1">,
   InGroup<UnusedLocalTypedef>, DefaultIgnore;
 def warn_unused_property_backing_ivar :

cfe/trunk/lib/CodeGen/TargetInfo.cpp

 
 }
 
 void MSP430TargetCodeGenInfo::setTargetAttributes(
     const Decl *D, llvm::GlobalValue *GV, CodeGen::CodeGenModule &M) const {
   if (GV->isDeclaration())
     return;
   if (const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(D)) {
-    if (const MSP430InterruptAttr *attr = FD->getAttr<MSP430InterruptAttr>()) {
-      // Handle 'interrupt' attribute:
-      llvm::Function *F = cast<llvm::Function>(GV);
+    const auto *InterruptAttr = FD->getAttr<MSP430InterruptAttr>();
+    if (!InterruptAttr)
+      return;
+
 
-      // Step 1: Set ISR calling convention.
-      F->setCallingConv(llvm::CallingConv::MSP430_INTR);
 
-      // 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);
-    }
+    F->addFnAttr("interrupt", llvm::utostr(InterruptAttr->getNumber()));
   }
 }
 
 //===----------------------------------------------------------------------===//
 // MIPS ABI Implementation.  This works for both little-endian and
 // big-endian variants.
 //===----------------------------------------------------------------------===//
 

cfe/trunk/lib/Sema/SemaDeclAttr.cpp

static void handleARMInterruptAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   }
 
   unsigned Index = AL.getAttributeSpellingListIndex();
   D->addAttr(::new (S.Context)
              ARMInterruptAttr(AL.getLoc(), S.Context, Kind, Index));
 }
 
 static void handleMSP430InterruptAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  // MSP430 'interrupt' attribute is applied to
+  // a function with no parameters and void return type.
+  if (!isFunctionOrMethod(D)) {
+    S.Diag(D->getLocation(), diag::warn_attribute_wrong_decl_type)
+        << "'interrupt'" << ExpectedFunctionOrMethod;
+    return;
+  }
+
+  if (hasFunctionProto(D) && getFunctionOrMethodNumParams(D) != 0) {
+    S.Diag(D->getLocation(), diag::warn_msp430_interrupt_attribute)
+        << 0;
+    return;
+  }
+
+  if (!getFunctionOrMethodResultType(D)->isVoidType()) {
+    S.Diag(D->getLocation(), diag::warn_msp430_interrupt_attribute)
+        << 1;
+    return;
+  }
+
+  // The attribute takes one integer argument.
   if (!checkAttributeNumArgs(S, AL, 1))
     return;
 
   if (!AL.isArgExpr(0)) {
     S.Diag(AL.getLoc(), diag::err_attribute_argument_type)
         << AL << AANT_ArgumentIntegerConstant;
     return;
   }
 
-  // FIXME: Check for decl - it should be void ()(void).
-
   Expr *NumParamsExpr = static_cast<Expr *>(AL.getArgAsExpr(0));
   llvm::APSInt NumParams(32);
   if (!NumParamsExpr->isIntegerConstantExpr(NumParams, S.Context)) {
     S.Diag(AL.getLoc(), diag::err_attribute_argument_type)
         << AL << AANT_ArgumentIntegerConstant
         << NumParamsExpr->getSourceRange();
     return;
   }
-
+  // The argument should be in range 0..63.
   unsigned Num = NumParams.getLimitedValue(255);
-  if ((Num & 1) || Num > 30) {
+  if (Num > 63) {
     S.Diag(AL.getLoc(), diag::err_attribute_argument_out_of_bounds)
         << AL << (int)NumParams.getSExtValue()
         << NumParamsExpr->getSourceRange();
     return;
   }
 
   D->addAttr(::new (S.Context)
               MSP430InterruptAttr(AL.getLoc(), S.Context, Num,

cfe/trunk/test/CodeGen/attr-msp430.c

+// RUN: %clang_cc1 -triple msp430-unknown-unknown -emit-llvm < %s| FileCheck %s
+
+__attribute__((interrupt(1))) void foo(void) {}
+// CHECK: @llvm.used
+// CHECK-SAME: @foo
+
+// CHECK: define msp430_intrcc void @foo() #0
+// CHECK: attributes #0
+// CHECK-SAME: noinline
+// CHECK-SAME: "interrupt"="1"

cfe/trunk/test/Sema/attr-msp430.c

 // RUN: %clang_cc1 -triple msp430-unknown-unknown -fsyntax-only -verify %s
 
+__attribute__((interrupt(1))) int t; // expected-warning {{'interrupt' attribute only applies to functions}}
+
 int i;
-void f(void) __attribute__((interrupt(i))); /* expected-error {{'interrupt' attribute requires an integer constant}} */
+__attribute__((interrupt(i))) void f(void); // expected-error {{'interrupt' attribute requires an integer constant}}
+__attribute__((interrupt(1, 2))) void f2(void); // expected-error {{'interrupt' attribute takes one argument}}
+__attribute__((interrupt(1))) int f3(void); // expected-warning {{MSP430 'interrupt' attribute only applies to functions that have a 'void' return type}}
+__attribute__((interrupt(1))) void f4(int a); // expected-warning {{MSP430 'interrupt' attribute only applies to functions that have no parameters}}
+__attribute__((interrupt(64))) void f5(void); // expected-error {{'interrupt' attribute parameter 64 is out of bounds}}
 
-void f2(void) __attribute__((interrupt(12)));
+__attribute__((interrupt(0))) void f6(void);
+__attribute__((interrupt(63))) void f7(void);