[AVR] Add support for the 'interrupt' and 'naked' attributes

[dylanmckay]

Summary

This teaches clang how to parse and lower the 'interrupt' and 'naked'
attributes.

This allows interrupt signal handlers to be written.

[dylanmckay]

Lower the 'signal' attribute correctly

I had forgotten to lower this attribute and so it would not have been lowered to IR at all.

Now it works fine.

[aaron.ballman]

The patch is also missing Sema tests that ensure the attributes are properly diagnosed when applied to something other than a function, a target other than AVR, arguments are present, etc.

include/clang/Basic/Attr.td

488

No new undocumented attributes, please.

491

Does this attribute appertain to any specific subjects, or can you apply it to any declaration?

493

No new undocumented attributes, please.

test/CodeGen/avr/attributes/naked.c

4 ↗

(On Diff #83544)

This test seems unrelated as you didn't modify anything about the naked attribute?

[dylanmckay]

[AVR] Document the 'interrupt' and 'naked' attributes

include/clang/Basic/Attr.td

491

It can be applied to function definitions only.

test/CodeGen/avr/attributes/naked.c

4 ↗

(On Diff #83544)

I didn't modify the naked attribute, but I did want to include a test for AVR.

[aaron.ballman]

include/clang/Basic/Attr.td

491

Then it should have a Subjects line like AVRInterrupt does.

lib/CodeGen/TargetInfo.cpp

6914

You can use const auto * here.

lib/Sema/SemaDeclAttr.cpp

5129

You can remove this function.

5136

...and this one.

5164

Just call handleSimpleAttribute() instead.

5727

...same here.

test/CodeGen/avr/attributes/naked.c

4 ↗

(On Diff #83544)

This should probably be a separate commit then (and doesn't really need code review, since it's just adding a test and is otherwise NFC); just make sure the commit message explains why the test is beneficial to add.

[dylanmckay]

Code review from Aaron

• Use 'handleSimpleAttribute' rather than duplicating it
• Use 'auto' where a type is explicitly casted
• Add warnings for when the attribute is not on a function
• Add sema tests for when the attr is not on a function

[dylanmckay]

Remove a test for the 'naked' attribute

[dylanmckay]

Add 'Subjects' field to 'AVRSignal'

[dylanmckay]

lib/Sema/SemaDeclAttr.cpp

5165

I'm pretty sure that this check shouldn't be necessary, because we define Subjects = [Function] in TableGen.

Without it though, the warning doesn't appear. Do you know why that is @aaron.ballman?

[malcolm.parsons]

lib/CodeGen/TargetInfo.cpp

6916

You can use auto * here too.

[aaron.ballman]

lib/Sema/SemaDeclAttr.cpp

5164

Since this is no longer truly a simple attribute (it requires some custom checking), I think my earlier advice was wrong -- you should split this into a handleAVRInterruptAttr() method. I forgot that you need the custom checking because the attribute has custom parsing.

5165

It's because it requires custom parsing.

5166

The diagnostic and the predicate for it don't match your Subjects line. Should this appertain to ObjC methods? If not, you want to use ExpectedFunction instead. If so, you want to add ObjCMethod to the Subjects line.

5727

... same here.

[dylanmckay]

Code review comments

• Add 'Subjects = [ObjCMethod]' to attributes
• Use 'auto' keyword in one place
• Move complex attr parsing logic into static function

[aaron.ballman]

Just one more issue due to the nature of needing a custom parsed attribute.

lib/Sema/SemaDeclAttr.cpp

5130

Because you have to do custom parsing, you also need to manually check that the attribute was not given any arguments and diagnose if they are present.

test/Sema/avr-interrupt-attr.c

5

Please add a test for __attribute__((interrupt(12))), and a test with an Objective-C method. Similar for signal. You should add an ObjC method to the codegen tests as well.

[dylanmckay]

Verify that no arguments are given to the attributes

Also adds a bunch of tests.

[aaron.ballman]

The new tests aren't really what I had in mind. The codegen tests that should be sema tests can just be rolled into your existing sema tests, by the way.

lib/Sema/SemaDeclAttr.cpp

5137

This should simply return rather than attempt to attach an invalid attribute to the declaration (same below).

test/CodeGen/avr/attributes/interrupt-no-args.c

3 ↗

(On Diff #87164)

This should be a test in Sema, not in CodeGen.

test/CodeGen/avr/attributes/interrupt.c

4

As should this.

test/CodeGen/avr/attributes/interrupt.m

1 ↗

(On Diff #87164)

Why objective-c++?

4 ↗

(On Diff #87164)

This is not an Objective-C method decl, so it doesn't really test anything new. The test I was envisioning was something like:

@interface F // There's probably an expected warning here about a missing base class
-(void) foo __attribute__((interrupt));
@end

@implementation F
-(void) foo __attribute__((interrupt)) {}
@end

test/CodeGen/avr/attributes/signal-no-args.c

4 ↗

(On Diff #87164)

This should also be a Sema test.

test/CodeGen/avr/attributes/signal.m

4 ↗

(On Diff #87164)

Same concerns about this not testing anything new as above.

[dylanmckay]

Code review

• Remove support for ObjC methods. We shouldn't do this as it doesn't really make sense
• Move some tests to Sema
• Don't attach invalid attributes when it isn't invalid

lib/Sema/SemaDeclAttr.cpp

5137

Nice catch

test/CodeGen/avr/attributes/interrupt.c

4

It seems like this sort of test _does_ sit in CodeGen - see test/CodeGen/{arm-interrupt-attr.c|mips-interrupt-attr.c}.

[dylanmckay]

test/CodeGen/avr/attributes/interrupt.m

4 ↗

(On Diff #87164)

Removed Obj-C method support.

[aaron.ballman]

lib/Sema/SemaDeclAttr.cpp

5137

No need to call Attr.setInvalid() here or below.

test/CodeGen/avr/attributes/interrupt.c

4

You're correct, this test does belong here. I think I attached my comment to the wrong thing (sorry about that).

[dylanmckay]

Remove 'Attr.setInvalid()'

test/CodeGen/avr/attributes/interrupt.c

4

No problems, thanks for the review!

[aaron.ballman]

LGTM!

include/clang/Basic/Attr.td

 // specify architectures for which the target applies, based off the ArchType
 // enumeration in Triple.h.
 class TargetArch<list<string> arches> {
   list<string> Arches = arches;
   list<string> OSes;
   list<string> CXXABIs;
 }
 def TargetARM : TargetArch<["arm", "thumb", "armeb", "thumbeb"]>;
+def TargetAVR : TargetArch<["avr"]>;
 def TargetMips : TargetArch<["mips", "mipsel"]>;
 def TargetMSP430 : TargetArch<["msp430"]>;
 def TargetX86 : TargetArch<["x86"]>;
 def TargetAnyX86 : TargetArch<["x86", "x86_64"]>;
 def TargetWindows : TargetArch<["x86", "x86_64", "arm", "thumb"]> {
   let OSes = ["Win32"];
 }
 def TargetMicrosoftCXXABI : TargetArch<["x86", "x86_64", "arm", "thumb"]> {
let Args = [EnumArgument<"Interrupt", "InterruptType",
                            ["IRQ", "FIQ", "SWI", "ABORT", "UNDEF", ""],
                            ["IRQ", "FIQ", "SWI", "ABORT", "UNDEF", "Generic"],
                            1>];
   let ParseKind = "Interrupt";
   let HasCustomParsing = 1;
   let Documentation = [ARMInterruptDocs];
 }
 
+def AVRInterrupt : InheritableAttr, TargetSpecificAttr<TargetAVR> {
+  let Spellings = [GNU<"interrupt">];
+  let Subjects = SubjectList<[Function, ObjCMethod]>;
+  let ParseKind = "Interrupt";
+  let Documentation = [AVRInterruptDocs];
+}
+
+def AVRSignal : InheritableAttr, TargetSpecificAttr<TargetAVR> {
+  let Spellings = [GNU<"signal">];
+  let Subjects = SubjectList<[Function, ObjCMethod]>;
+  let Documentation = [AVRSignalDocs];
+}
+
 def AsmLabel : InheritableAttr {
   let Spellings = [Keyword<"asm">, Keyword<"__asm__">];
   let Args = [StringArgument<"Label">];
   let SemaHandler = 0;
   let Documentation = [Undocumented];
 }
 
 def Availability : InheritableAttr {

include/clang/Basic/AttrDocs.td

 - The function return sequence is changed to use an exception return instruction.
 
 - The parameter sets the interrupt mask for the function corresponding to the
   interrupt level specified. If no mask is specified the interrupt mask
   defaults to "eic".
   }];
 }
 
+def AVRInterruptDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
+Clang supports the GNU style ``__attribute__((interrupt))`` attribute on
+AVR targets. This attribute may be attached to a function definition and instructs
+the backend to generate appropriate function entry/exit code so that it can be used
+directly as an interrupt service routine.
+
+On the AVR, the hardware globally disables interrupts when an interrupt is executed.
+The first instruction of an interrupt handler declared with this attribute is a SEI
+instruction to re-enable interrupts. See also the signal attribute that
+does not insert a SEI instruction.
+  }];
+}
+
+def AVRSignalDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
+Clang supports the GNU style ``__attribute__((signal))`` attribute on
+AVR targets. This attribute may be attached to a function definition and instructs
+the backend to generate appropriate function entry/exit code so that it can be used
+directly as an interrupt service routine.
+
+Interrupt handler functions defined with the signal attribute do not re-enable interrupts.
+}];
+}
+
 def TargetDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
 Clang supports the GNU style ``__attribute__((target("OPTIONS")))`` attribute.
 This attribute may be attached to a function definition and instructs
 the backend to use different code generation options than were passed on the
 command line.
 

lib/CodeGen/TargetInfo.cpp

MIPSTargetCodeGenInfo::initDwarfEHRegSizeTable(CodeGen::CodeGenFunction &CGF,
   // 112-143 are the coprocessor 2 registers, $c2r0 - $c2r31.
   // 144-175 are the coprocessor 3 registers, $c3r0 - $c3r31.
   // 176-181 are the DSP accumulator registers.
   AssignToArrayRange(CGF.Builder, Address, Four8, 80, 181);
   return false;
 }
 
 //===----------------------------------------------------------------------===//
+// AVR ABI Implementation.
+//===----------------------------------------------------------------------===//
+
+namespace {
+class AVRTargetCodeGenInfo : public TargetCodeGenInfo {
+public:
+  AVRTargetCodeGenInfo(CodeGenTypes &CGT)
+    : TargetCodeGenInfo(new DefaultABIInfo(CGT)) { }
+
+  void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
+                           CodeGen::CodeGenModule &CGM) const override {
+    const auto *FD = dyn_cast_or_null<FunctionDecl>(D);
+    if (!FD) return;
+    auto *Fn = cast<llvm::Function>(GV);
+
+    if (FD->getAttr<AVRInterruptAttr>())
+      Fn->addFnAttr("interrupt");
+
+    if (FD->getAttr<AVRSignalAttr>())
+      Fn->addFnAttr("signal");
+  }
+};
+}
+
+//===----------------------------------------------------------------------===//
 // TCE ABI Implementation (see http://tce.cs.tut.fi). Uses mostly the defaults.
 // Currently subclassed only to implement custom OpenCL C function attribute
 // handling.
 //===----------------------------------------------------------------------===//
 
 namespace {
 
 class TCETargetCodeGenInfo : public DefaultTargetCodeGenInfo {
case llvm::Triple::mipsel:
     if (Triple.getOS() == llvm::Triple::NaCl)
       return SetCGInfo(new PNaClTargetCodeGenInfo(Types));
     return SetCGInfo(new MIPSTargetCodeGenInfo(Types, true));
 
   case llvm::Triple::mips64:
   case llvm::Triple::mips64el:
     return SetCGInfo(new MIPSTargetCodeGenInfo(Types, false));
 
+  case llvm::Triple::avr:
+    return SetCGInfo(new AVRTargetCodeGenInfo(Types));
+
   case llvm::Triple::aarch64:
   case llvm::Triple::aarch64_be: {
     AArch64ABIInfo::ABIKind Kind = AArch64ABIInfo::AAPCS;
     if (getTarget().getABI() == "darwinpcs")
       Kind = AArch64ABIInfo::DarwinPCS;
 
     return SetCGInfo(new AArch64TargetCodeGenInfo(Types, Kind));
   }

lib/Sema/SemaDeclAttr.cpp

S.Diag(getFunctionOrMethodParamRange(D, 1).getBegin(),
         << 3 << S.Context.getIntTypeForBitwidth(TypeSize, /*Signed=*/false);
     return;
   }
   D->addAttr(::new (S.Context) AnyX86InterruptAttr(
       Attr.getLoc(), S.Context, Attr.getAttributeSpellingListIndex()));
   D->addAttr(UsedAttr::CreateImplicit(S.Context));
 }
 
+static void handleAVRInterruptAttr(Sema &S, Decl *D, const AttributeList &Attr) {
+  if (!isFunctionOrMethod(D)) {
+    S.Diag(D->getLocation(), diag::warn_attribute_wrong_decl_type)
+        << "'interrupt'" << ExpectedFunctionOrMethod;
+    return;
+  }
+
+  handleSimpleAttribute<AVRInterruptAttr>(S, D, Attr);
+}
+
+static void handleAVRSignalAttr(Sema &S, Decl *D, const AttributeList &Attr) {
+  if (!isFunctionOrMethod(D)) {
+    S.Diag(D->getLocation(), diag::warn_attribute_wrong_decl_type)
+        << "'signal'" << ExpectedFunctionOrMethod;
+    return;
+  }
+
+  handleSimpleAttribute<AVRSignalAttr>(S, D, Attr);
+}
+
 static void handleInterruptAttr(Sema &S, Decl *D, const AttributeList &Attr) {
   // Dispatch the interrupt attribute based on the current target.
   switch (S.Context.getTargetInfo().getTriple().getArch()) {
   case llvm::Triple::msp430:
     handleMSP430InterruptAttr(S, D, Attr);
     break;
   case llvm::Triple::mipsel:
   case llvm::Triple::mips:
     handleMipsInterruptAttr(S, D, Attr);
     break;
   case llvm::Triple::x86:
   case llvm::Triple::x86_64:
     handleAnyX86InterruptAttr(S, D, Attr);
     break;
+  case llvm::Triple::avr:
+    handleAVRInterruptAttr(S, D, Attr);
+    break;
   default:
     handleARMInterruptAttr(S, D, Attr);
     break;
   }
 }
 
 static void handleAMDGPUFlatWorkGroupSizeAttr(Sema &S, Decl *D,
                                               const AttributeList &Attr) {
   uint32_t Min = 0;
case AttributeList::AT_AMDGPUWavesPerEU:
     handleAMDGPUWavesPerEUAttr(S, D, Attr);
     break;
   case AttributeList::AT_AMDGPUNumSGPR:
     handleAMDGPUNumSGPRAttr(S, D, Attr);
     break;
   case AttributeList::AT_AMDGPUNumVGPR:
     handleAMDGPUNumVGPRAttr(S, D, Attr);
     break;
+  case AttributeList::AT_AVRSignal:
+    handleAVRSignalAttr(S, D, Attr);
+    break;
   case AttributeList::AT_IBAction:
     handleSimpleAttribute<IBActionAttr>(S, D, Attr);
     break;
   case AttributeList::AT_IBOutlet:
     handleIBOutlet(S, D, Attr);
     break;
   case AttributeList::AT_IBOutletCollection:
     handleIBOutletCollection(S, D, Attr);

test/CodeGen/avr/attributes/interrupt.c

+// RUN: %clang_cc1 -triple avr-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+
+// CHECK: define void @foo() #0
+__attribute__((interrupt)) void foo(void) { }
+
+// CHECK: attributes #0 = {{{.*interrupt.*}}}

test/CodeGen/avr/attributes/signal.c

+// RUN: %clang_cc1 -triple avr-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+
+// CHECK: define void @foo() #0
+__attribute__((signal)) void foo(void) { }
+
+// CHECK: attributes #0 = {{{.*signal.*}}}

test/Sema/avr-interrupt-attr.c

+// RUN: %clang_cc1 %s -triple avr-unknown-unknown -verify -fsyntax-only
+struct a { int b; };
+
+struct a test __attribute__((interrupt)); // expected-warning {{'interrupt' attribute only applies to functions and methods}}
+
+__attribute__((interrupt)) void food() {}

test/Sema/avr-signal-attr.c

+// RUN: %clang_cc1 %s -triple avr-unknown-unknown -verify -fsyntax-only
+struct a { int b; };
+
+struct a test __attribute__((signal)); // expected-warning {{'signal' attribute only applies to functions and methods}}
+
+__attribute__((signal)) void food() {}