[clang][ARM] Fix warning for using VFP from interrupts.

This warning has three issues:
 - The interrupt attribute causes the function to return using an exception
   return instruction. This warning allows calls from one function with
   the interrupt attribute to another, and the diagnostic text suggests
   that not having the attribute on the callee is a problem. Actually
   making such a call will lead to a double exception return, which is
   unpredictable according to the ARM architecture manual section
   B9.1.1, "Restrictions on exception return instructions". Even on
   machines where an exception return from user/system mode is
   tolerated, if the callee's interrupt type is anything other than a
   supervisor call or secure monitor call, it will also return to a
   different address than a normal function would. For example,
   returning from an "IRQ" handler will return to lr - 4, which will
   generally result in calling the same function again.
 - The interrupt attribute currently does not cause caller-saved VFP
   registers to be saved and restored if they are used, so putting
   __attribute__((interrupt)) on a called function doesn't prevent it
   from clobbering VFP state.
 - It is part of the -Wextra diagnostic group and can't be individually
   disabled when using -Wextra, which also means the diagnostic text of
   this specific warning appears in the documentation of -Wextra.

This change addresses all three issues by instead generating a warning
for any interrupt handler where the vfp feature is enabled. The warning is
also given its own diagnostic group.

Closes #34876.

Author: chrisnc

clang/docs/ReleaseNotes.rst

@@ -448,6 +448,11 @@ Modified Compiler Flags
   evaluating to ``true`` and an empty body such as ``while(1);``)
   are considered infinite, even when the ``-ffinite-loop`` flag is set.
 
+- Removed "arm interrupt calling convention" warning that was included in
+  ``-Wextra`` without its own flag.
+
+- Added ``-Warm-interrupt-vfp-clobber``, with its own warning group.
+
 Removed Compiler Flags
 -------------------------
 
@@ -612,6 +617,12 @@ Improvements to Clang's diagnostics
   used rather than when they are needed for constant evaluation or when code is generated for them.
   The check is now stricter to prevent crashes for some unsupported declarations (Fixes #GH95495).
 
+- For the ARM target, Clang no longer suggests adding ``__attribute__((interrupt))`` to
+  functions that are called from interrupt handlers to prevent clobbering VFP registers
+  as part of ``-Wextra`` (#GH34876). Following this suggestion leads to unpredictable
+  behavior. Instead, a new warning, ``-Warm-interrupt-vfp-clobber`` will trigger for
+  interrupt handlers with VFP enabled.
+
 Improvements to Clang's time-trace
 ----------------------------------
 

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -336,9 +336,10 @@ def warn_anyx86_excessive_regsave : Warning<
   " with attribute 'no_caller_saved_registers'"
   " or be compiled with '-mgeneral-regs-only'">,
   InGroup<DiagGroup<"excessive-regsave">>;
-def warn_arm_interrupt_calling_convention : Warning<
-   "call to function without interrupt attribute could clobber interruptee's VFP registers">,
-   InGroup<Extra>;
+def warn_arm_interrupt_vfp_clobber : Warning<
+  "interrupt service routine with vfp enabled may clobber the "
+  "interruptee's vfp state">,
+  InGroup<DiagGroup<"arm-interrupt-vfp-clobber">>;
 def warn_interrupt_attribute_invalid : Warning<
    "%select{MIPS|MSP430|RISC-V}0 'interrupt' attribute only applies to "
    "functions that have %select{no parameters|a 'void' return type}1">,

clang/lib/Sema/SemaARM.cpp

@@ -1326,6 +1326,11 @@ void SemaARM::handleInterruptAttr(Decl *D, const ParsedAttr &AL) {
     return;
   }
 
+  const TargetInfo &TI = getASTContext().getTargetInfo();
+  if (TI.hasFeature("vfp")) {
+    Diag(D->getLocation(), diag::warn_arm_interrupt_vfp_clobber);
+  }
+
   D->addAttr(::new (getASTContext())
                  ARMInterruptAttr(getASTContext(), AL, Kind));
 }

clang/lib/Sema/SemaExpr.cpp

@@ -6770,22 +6770,10 @@ ExprResult Sema::BuildResolvedCallExpr(Expr *Fn, NamedDecl *NDecl,
     return ExprError();
   }
 
-  // Interrupt handlers don't save off the VFP regs automatically on ARM,
-  // so there's some risk when calling out to non-interrupt handler functions
-  // that the callee might not preserve them. This is easy to diagnose here,
-  // but can be very challenging to debug.
-  // Likewise, X86 interrupt handlers may only call routines with attribute
+  // X86 interrupt handlers may only call routines with attribute
   // no_caller_saved_registers since there is no efficient way to
   // save and restore the non-GPR state.
   if (auto *Caller = getCurFunctionDecl()) {
-    if (Caller->hasAttr<ARMInterruptAttr>()) {
-      bool VFP = Context.getTargetInfo().hasFeature("vfp");
-      if (VFP && (!FDecl || !FDecl->hasAttr<ARMInterruptAttr>())) {
-        Diag(Fn->getExprLoc(), diag::warn_arm_interrupt_calling_convention);
-        if (FDecl)
-          Diag(FDecl->getLocation(), diag::note_callee_decl) << FDecl;
-      }
-    }
     if (Caller->hasAttr<AnyX86InterruptAttr>() ||
         Caller->hasAttr<AnyX86NoCallerSavedRegistersAttr>()) {
       const TargetInfo &TI = Context.getTargetInfo();

clang/test/Sema/arm-interrupt-attr.c

@@ -1,52 +1,19 @@
-// RUN: %clang_cc1 %s -triple arm-apple-darwin  -target-feature +vfp2 -verify -fsyntax-only
-// RUN: %clang_cc1 %s -triple thumb-apple-darwin  -target-feature +vfp3 -verify -fsyntax-only
-// RUN: %clang_cc1 %s -triple armeb-none-eabi  -target-feature +vfp4 -verify -fsyntax-only
-// RUN: %clang_cc1 %s -triple thumbeb-none-eabi  -target-feature +neon -verify -fsyntax-only
-// RUN: %clang_cc1 %s -triple thumbeb-none-eabi -target-feature +neon -target-feature +soft-float -DSOFT -verify -fsyntax-only
+// RUN: %clang_cc1 %s -triple arm-none-eabi -DSOFT -verify -fsyntax-only
+// RUN: %clang_cc1 %s -triple arm-none-eabi -target-feature +vfp2 -verify -fsyntax-only
 
-__attribute__((interrupt(IRQ))) void foo(void) {} // expected-error {{'interrupt' attribute requires a string}}
-__attribute__((interrupt("irq"))) void foo1(void) {} // expected-warning {{'interrupt' attribute argument not supported: irq}}
 
+#ifdef SOFT
+__attribute__((interrupt("irq"))) void foo1(void) {} // expected-warning {{'interrupt' attribute argument not supported: irq}}
+__attribute__((interrupt(IRQ))) void foo(void) {} // expected-error {{'interrupt' attribute requires a string}}
 __attribute__((interrupt("IRQ", 1))) void foo2(void) {} // expected-error {{'interrupt' attribute takes no more than 1 argument}}
-
 __attribute__((interrupt("IRQ"))) void foo3(void) {}
 __attribute__((interrupt("FIQ"))) void foo4(void) {}
 __attribute__((interrupt("SWI"))) void foo5(void) {}
 __attribute__((interrupt("ABORT"))) void foo6(void) {}
 __attribute__((interrupt("UNDEF"))) void foo7(void) {}
-
 __attribute__((interrupt)) void foo8(void) {}
 __attribute__((interrupt())) void foo9(void) {}
 __attribute__((interrupt(""))) void foo10(void) {}
-
-#ifndef SOFT
-// expected-note@+2 {{'callee1' declared here}}
-#endif
-void callee1(void);
-__attribute__((interrupt("IRQ"))) void callee2(void);
-void caller1(void) {
-  callee1();
-  callee2();
-}
-
-#ifndef SOFT
-__attribute__((interrupt("IRQ"))) void caller2(void) {
-  callee1(); // expected-warning {{call to function without interrupt attribute could clobber interruptee's VFP registers}}
-  callee2();
-}
-
-void (*callee3)(void);
-__attribute__((interrupt("IRQ"))) void caller3(void) {
-  callee3(); // expected-warning {{call to function without interrupt attribute could clobber interruptee's VFP registers}}
-}
 #else
-__attribute__((interrupt("IRQ"))) void caller2(void) {
-  callee1();
-  callee2();
-}
-
-void (*callee3)(void);
-__attribute__((interrupt("IRQ"))) void caller3(void) {
-  callee3();
-}
+__attribute__((interrupt("IRQ"))) void float_irq(void); // expected-warning {{interrupt service routine with vfp enabled may clobber the interruptee's vfp state}}
 #endif