[clang] [clang][ARM] Fix warning for using VFP from interrupts. (PR #91870)
Chris Copeland via cfe-commits
cfe-commits at lists.llvm.org
Tue Jul 2 08:01:23 PDT 2024
https://github.com/chrisnc updated https://github.com/llvm/llvm-project/pull/91870
>From d4908fb17a57ddc75032e838241ff4f0e929cd36 Mon Sep 17 00:00:00 2001
From: Chris Copeland <chris at chrisnc.net>
Date: Sat, 11 May 2024 00:15:50 -0700
Subject: [PATCH 1/2] [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/docs/ReleaseNotes.rst | 10 +++++
.../clang/Basic/DiagnosticSemaKinds.td | 7 +--
clang/lib/Sema/SemaARM.cpp | 5 +++
clang/lib/Sema/SemaExpr.cpp | 14 +-----
clang/test/Sema/arm-interrupt-attr.c | 45 +++----------------
5 files changed, 26 insertions(+), 55 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 181d10008bc8c..b637fe1f7ebd8 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -442,6 +442,9 @@ New Compiler Flags
Matches MSVC behaviour by defining ``__STDC__`` to ``1`` when
MSVC compatibility mode is used. It has no effect for C++ code.
+- For the ARM target, added ``-Warm-interrupt-vfp-clobber`` that will emit a
+ diagnostic when an interrupt handler is declared and VFP is enabled.
+
Deprecated Compiler Flags
-------------------------
@@ -480,6 +483,13 @@ 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 the "arm interrupt calling convention" warning that was included in
+ ``-Wextra`` without its own flag. This warning suggested adding
+ ``__attribute__((interrupt))`` to functions that are called from interrupt
+ handlers to prevent clobbering VFP registers. Following this suggestion leads
+ to unpredictable behavior by causing multiple exception returns from one
+ exception. Fixes #GH34876.
+
Removed Compiler Flags
-------------------------
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 3df64b2ecef1b..c116c951918ac 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<
+ "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">,
diff --git a/clang/lib/Sema/SemaARM.cpp b/clang/lib/Sema/SemaARM.cpp
index 281d534152054..8259be77d9ee6 100644
--- a/clang/lib/Sema/SemaARM.cpp
+++ b/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));
}
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 852344d895ffd..be579be7bf331 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -6631,22 +6631,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();
diff --git a/clang/test/Sema/arm-interrupt-attr.c b/clang/test/Sema/arm-interrupt-attr.c
index 3537fba8521ad..aed2c262b440e 100644
--- a/clang/test/Sema/arm-interrupt-attr.c
+++ b/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 at +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
>From 75e7fad95a0c324429fee7ae1bd46fcdd6ff9f58 Mon Sep 17 00:00:00 2001
From: Chris Copeland <chris at chrisnc.net>
Date: Thu, 13 Jun 2024 23:34:43 -0700
Subject: [PATCH 2/2] [clang][ARM] Emit an error when an interrupt handler is
called.
Closes #95359.
---
clang/docs/ReleaseNotes.rst | 2 ++
.../include/clang/Basic/DiagnosticSemaKinds.td | 2 ++
clang/lib/Sema/SemaExpr.cpp | 12 +++++++++---
clang/test/Sema/arm-interrupt-attr.c | 17 ++++++++++++-----
4 files changed, 25 insertions(+), 8 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index b637fe1f7ebd8..56025cb31dcd7 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -668,6 +668,8 @@ Improvements to Clang's diagnostics
- Clang now shows implicit deduction guides when diagnosing overload resolution failure. #GH92393.
+- For the ARM target, calling an interrupt handler from another function is now an error. #GH95359.
+
Improvements to Clang's time-trace
----------------------------------
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index c116c951918ac..78851f716c16b 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -340,6 +340,8 @@ 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 err_arm_interrupt_called : Error<
+ "interrupt service routine cannot be called directly">;
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 be579be7bf331..38cbef4b375f5 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -6626,9 +6626,15 @@ ExprResult Sema::BuildResolvedCallExpr(Expr *Fn, NamedDecl *NDecl,
unsigned BuiltinID = (FDecl ? FDecl->getBuiltinID() : 0);
// Functions with 'interrupt' attribute cannot be called directly.
- if (FDecl && FDecl->hasAttr<AnyX86InterruptAttr>()) {
- Diag(Fn->getExprLoc(), diag::err_anyx86_interrupt_called);
- return ExprError();
+ if (FDecl) {
+ if (FDecl->hasAttr<AnyX86InterruptAttr>()) {
+ Diag(Fn->getExprLoc(), diag::err_anyx86_interrupt_called);
+ return ExprError();
+ }
+ if (FDecl->hasAttr<ARMInterruptAttr>()) {
+ Diag(Fn->getExprLoc(), diag::err_arm_interrupt_called);
+ return ExprError();
+ }
}
// X86 interrupt handlers may only call routines with attribute
diff --git a/clang/test/Sema/arm-interrupt-attr.c b/clang/test/Sema/arm-interrupt-attr.c
index aed2c262b440e..f2698eedacea1 100644
--- a/clang/test/Sema/arm-interrupt-attr.c
+++ b/clang/test/Sema/arm-interrupt-attr.c
@@ -1,8 +1,10 @@
-// RUN: %clang_cc1 %s -triple arm-none-eabi -DSOFT -verify -fsyntax-only
+// RUN: %clang_cc1 %s -triple arm-none-eabi -verify -fsyntax-only
// RUN: %clang_cc1 %s -triple arm-none-eabi -target-feature +vfp2 -verify -fsyntax-only
-#ifdef SOFT
+#ifdef __ARM_FP
+__attribute__((interrupt("IRQ"))) void float_irq(void); // expected-warning {{interrupt service routine with vfp enabled may clobber the interruptee's vfp state}}
+#else // !defined(__ARM_FP)
__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}}
@@ -14,6 +16,11 @@ __attribute__((interrupt("UNDEF"))) void foo7(void) {}
__attribute__((interrupt)) void foo8(void) {}
__attribute__((interrupt())) void foo9(void) {}
__attribute__((interrupt(""))) void foo10(void) {}
-#else
-__attribute__((interrupt("IRQ"))) void float_irq(void); // expected-warning {{interrupt service routine with vfp enabled may clobber the interruptee's vfp state}}
-#endif
+
+__attribute__((interrupt("IRQ"))) void callee(void) {}
+
+void caller(void)
+{
+ callee(); // expected-error {{interrupt service routine cannot be called directly}}
+}
+#endif // __ARM_FP
More information about the cfe-commits
mailing list