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

[chrisnc]

[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.

[clang][ARM] Emit an error when an interrupt handler is called.

Closes #95359.

[llvmbot]

@llvm/pr-subscribers-backend-aarch64
@llvm/pr-subscribers-backend-arm

@llvm/pr-subscribers-clang

Author: Chris Copeland (chrisnc)

Changes

This warning has two issues:

• The interrupt attribute doesn't only change how volatile registers
  are treated; it also 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 the 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.
• 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 both issues. Rather than check that the callee has
the interrupt attribute, check that it uses the soft-float feature,
which will prevent use of VFP state. The warning is also given its own
diagnostic group.

---

Full diff: https://github.com/llvm/llvm-project/pull/91870.diff

5 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+11)
• (modified) clang/include/clang/Basic/Attr.td (+7)
• (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+4-3)
• (modified) clang/lib/Sema/SemaExpr.cpp (+10-9)
• (modified) clang/test/Sema/arm-interrupt-attr.c (+3-3)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 7c5dcc59c7016..3f9f81bc2bfac 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -352,6 +352,10 @@ 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`` but did not have its own flag. Added
+  ``-Warm-interrupt-vfp-clobber`` that enables the modified warning.
+
 Removed Compiler Flags
 -------------------------
 
@@ -484,6 +488,13 @@ Improvements to Clang's diagnostics
        }
      };
 
+- On ARM, Clang no longer suggests adding ``__attribute__((interrupt))`` to normal
+  functions that are called from interrupt handlers to prevent clobbering VFP
+  registers as part of ``-Wextra``. Following this suggestion leads to
+  unpredictable behavior. Instead, ``-Warm-interrupt-vfp-clobber`` can now be
+  used to detect calling functions that don't have VFP disabled with
+  ``__attribute((target("soft-float")))`` from an interrupt handler.
+
 Improvements to Clang's time-trace
 ----------------------------------
 
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 52552ba488560..04e3b8f949992 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -3090,6 +3090,13 @@ def Target : InheritableAttr {
       }
     }
 
+    bool hasFeature(StringRef Feature) const {
+      StringRef Features = getFeaturesStr();
+      SmallVector<StringRef, 1> AttrFeatures;
+      Features.split(AttrFeatures, ",");
+      return Features.contains(Feature);
+    }
+
     bool isDefaultVersion() const { return getFeaturesStr() == "default"; }
   }];
 }
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index d6863f90edb6e..1c5f5ffb03dc5 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/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<
+   "calling a VFP-enabled function from an interrupt could clobber the "
+   "interruptee's VFP registers">,
+   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">,
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index c688cb21f2364..c514820bd899c 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -6942,22 +6942,23 @@ 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
-  // no_caller_saved_registers since there is no efficient way to
-  // save and restore the non-GPR state.
   if (auto *Caller = getCurFunctionDecl()) {
+    // Interrupt handlers don't save volatile VFP registers automatically on
+    // ARM, so calling other functions that use VFP will likely cause the
+    // interruptee's VFP state to be clobbered. This is easy to diagnose here,
+    // but can be very challenging to debug.
     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 (VFP && (!FDecl || !FDecl->hasAttr<TargetAttr>() ||
+                  !FDecl->getAttr<TargetAttr>()->hasFeature("soft-float"))) {
+        Diag(Fn->getExprLoc(), diag::warn_arm_interrupt_vfp_clobber);
         if (FDecl)
           Diag(FDecl->getLocation(), diag::note_callee_decl) << FDecl;
       }
     }
+    // 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 (Caller->hasAttr<AnyX86InterruptAttr>() ||
         Caller->hasAttr<AnyX86NoCallerSavedRegistersAttr>()) {
       const TargetInfo &TI = Context.getTargetInfo();
diff --git a/clang/test/Sema/arm-interrupt-attr.c b/clang/test/Sema/arm-interrupt-attr.c
index 3537fba8521ad..112e5b5e8ad1e 100644
--- a/clang/test/Sema/arm-interrupt-attr.c
+++ b/clang/test/Sema/arm-interrupt-attr.c
@@ -23,7 +23,7 @@ __attribute__((interrupt(""))) void foo10(void) {}
 // expected-note@+2 {{'callee1' declared here}}
 #endif
 void callee1(void);
-__attribute__((interrupt("IRQ"))) void callee2(void);
+__attribute__((target("soft-float"))) void callee2(void);
 void caller1(void) {
   callee1();
   callee2();
@@ -31,13 +31,13 @@ void caller1(void) {
 
 #ifndef SOFT
 __attribute__((interrupt("IRQ"))) void caller2(void) {
-  callee1(); // expected-warning {{call to function without interrupt attribute could clobber interruptee's VFP registers}}
+  callee1(); // expected-warning {{calling a VFP-enabled function from an interrupt could clobber the 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}}
+  callee3(); // expected-warning {{calling a VFP-enabled function from an interrupt could clobber the interruptee's VFP registers}}
 }
 #else
 __attribute__((interrupt("IRQ"))) void caller2(void) {

[chrisnc]

Here is an example of the existing warning in action. https://godbolt.org/z/9e84EfeYP
It warns for calling a normal function, but the same function with an interrupt attribute does not warn. The subs pc, lr, 4 in bar_irq goes back to bl bar_irq, creating an infinite loop, where the second subs pc, lr, 4 from bar_irq is also unpredictable, because the processor mode will most likely be system/user.

[chrisnc]

I'd like to also address #47815, but I wasn't able to find the right path to answer "is this M-profile" from TargetInfo. Would appreciate any suggestions on that. I think it may require dynamic_cast-ing the TargetInfo to an ARMTargetInfo, and then change one of the methods in there to be public...

[chrisnc]

ping @ostannard @smithp35

[chrisnc]

Rebased and fixed conflict in release notes.

[chrisnc]

ping (rebased and fixed another release notes conflict)

[chrisnc]

Ping

[chrisnc]

Ping @DavidSpickett @jthackray

[DavidSpickett]

I can't fully approve this, I leave that to other members of @llvm/pr-subscribers-arm .

2 overall comments:

• Calling a function marked interrupt from a function marked interrupt is still UB isn't it? Should there be another warning to cover that scenario as well?
• I see that gcc has a warning in -Wattributes for this, you should raise a bug there equivalent to the one llvm has, perhaps there even is one already.

[DavidSpickett]

I'd like to also address #47815, but I wasn't able to find the right path to answer "is this M-profile" from TargetInfo.

Any ARM folks know if there are existing M profile only warnings to learn from?

[chrisnc]

Thank you for the review!

• Calling a function marked interrupt from a function marked interrupt is still UB isn't it? Should there be another warning to cover that scenario as well?

Correct. I can create a separate issue to describe such a warning. Calling an interrupt handler from a normal function is also UB for similar reasons; when it returns it will attempt an exception return for an exception that hasn't occurred, so either it's immediately undefined because the caller was already in system/user, or the caller was in supervisor mode from reset, and an exception return will update CPSR to whatever garbage was in SPSR_svc, and also it will return to the calling instruction, rather than one past it.

• I see that gcc has a warning in -Wattributes for this, you should raise a bug there equivalent to the one llvm has, perhaps there even is one already.

It does, but gcc's -Wattributes doesn't suffer from any of the issues being addressed here: it's already a separate flag, it cannot be silenced by putting __attribute__((interrupt)) on the callee, and its warning text describes compiling with -mgeneral-regs-only to avoid the issue, though this only works when passed on the command line, not with __attribute__((target("general-regs-only"))). As far as I can tell, -Wattributes fires on any function with __attribute__((interrupt)) if the code is being compiled for a floating-point target and without -mgeneral-regs-only, even if that function is empty: https://godbolt.org/z/rh69Tbxv6

It seems that LLVM does not have this target feature for 32-bit Arm, so I opted for soft-float as the escape hatch, which should achieve the same result, and check for it in function attributes (the command-line case is already covered by the check for VFP). It is not checked transitively though, so this PR will not catch cases where an interrupt calls a soft-float function that then calls a function that uses VFP. The latter call is otherwise allowed though. IMO this is still a lot better than not having the escape hatch, but I'm open to suggestions.

[chrisnc]

I've rebased, updated the warning text and release notes, and created #95359 for the future improvement to warn about calling interrupt handlers. (Edit: also updated the tests to expect the new warning text.)

[chrisnc]

While doing some more research and comparing gcc and clang, I noticed that clang doesn't even save the volatile VFP registers in an interrupt handler that uses them directly, so there's no point in just checking for function calls. gcc will save volatile VFP regs before use in an interrupt handler, but it won't save fpscr before doing floating-point comparisons. Maybe the behavior was different when this warning was introduced, but as of now, using VFP at all in an interrupt handler will likely clobber VFP state, so I think the right path here is to just implement -Wattributes in essentially the same way gcc does...

[chrisnc]

Updated to just warn if the attribute is used while vfp is enabled, and added an error for calling an interrupt handler, similar to x86.

[evodius96]

Somewhat related to this -- we (TI) are presently in the process of upstreaming our mod to save/restore VFP registers with the interrupt_save_fp attribute. See #89654. I think the patch needs to be updated in light of upstream changes to the Arm frame lowering code.

[chrisnc]

Rebased to fix release notes conflict. @DavidSpickett @llvm/pr-subscribers-arm

[chrisnc]

Rebased again, clarified the release notes changes and added a test case for the new error.

[chrisnc]

Fixed more release notes conflicts.

[chrisnc]

Fixed another release notes conflict.

Ping @DavidSpickett @ostannard @jthackray.

[DavidSpickett]

Everything LGTM but I'd like someone who works on Arm's downstream compiler to approve. @ostannard perhaps?

(thanks for your patience @chrisnc)

[DavidSpickett]

Note to other reviewers: This is in the modified flags section because -wextra itself is being modified. A part of it was removed but that part was never a separate flag so this doesn't need to go in the "removed compiler flags" section.

[ostannard]

LGTM

[chrisnc]

Force-pushed to fix the single-line if style; did not rebase so builds should still be passing. Ready to merge (need someone else to).

[chrisnc]

Thanks!

[ahmedbougacha]

• Calling a function marked interrupt from a function marked interrupt is still UB isn't it? Should there be another warning to cover that scenario as well?

Correct. I can create a separate issue to describe such a warning. Calling an interrupt handler from a normal function is also UB for similar reasons; when it returns it will attempt an exception return for an exception that hasn't occurred, so either it's immediately undefined because the caller was already in system/user, or the caller was in supervisor mode from reset, and an exception return will update CPSR to whatever garbage was in SPSR_svc, and also it will return to the calling instruction, rather than one past it.

@chrisnc Catching up with this: as far as I can tell this is true for A/R profiles, but M profile doesn't have this restriction, because the exception return sequence isn't special. And the plain return we codegen only behaves as an exception return when the LR bits are setup in the special way. So it can return just fine if LR is a regular return address. I read that as saying there's no restriction to calling interrupt functions when targeting M profile. Does that sound correct to you all? Is there another reason to restrict calls even for M profile? (beyond "why would anyone ever do that" ;)

I see you mentioned the related #47815 :

I'd like to also address #47815, but I wasn't able to find the right path to answer "is this M-profile" from TargetInfo. Would appreciate any suggestions on that. I think it may require dynamic_cast-ing the TargetInfo to an ARMTargetInfo, and then change one of the methods in there to be public...

I think the somewhat-clean way to do that would be to add a bit in TargetInfo (see e.g., ARMCDECoprocMask), initialize it in ARMTargetInfo based on ArchProfile == M, and checking it in Sema via Context.getTargetInfo()

[chrisnc]

@ahmedbougacha yes, I think it's correct that calling an __attribute__((interrupt)) function is safe to do on M-profile because there is no difference in the return sequence; all it does is not assume that the stack is 8-byte aligned initially. Even then, this is only needed on actual exception handlers when not using the STKALIGN bit in CCR, which is on by default on most (all?) Cortex-M processors. This feature, along with the automatic volatile register saving, mean that __attribute__((interrupt)) has almost no legitimate uses on M-profile.