[clang] [ARM][clang] Fix warning for VFP function calls from interrupts. (PR #91870)
Chris Copeland via cfe-commits
cfe-commits at lists.llvm.org
Wed May 29 09:01:18 PDT 2024
https://github.com/chrisnc updated https://github.com/llvm/llvm-project/pull/91870
>From 846d22ff9d96c64c9b73f0270f49724b7ee1cb70 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] [ARM][clang] Fix warning for VFP function calls from
interrupts.
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.
Closes #34876.
---
clang/docs/ReleaseNotes.rst | 12 ++++++++++++
clang/include/clang/Basic/Attr.td | 7 +++++++
.../clang/Basic/DiagnosticSemaKinds.td | 7 ++++---
clang/lib/Sema/SemaExpr.cpp | 19 ++++++++++---------
clang/test/Sema/arm-interrupt-attr.c | 14 +++++++-------
5 files changed, 40 insertions(+), 19 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index bd92818f0c09d..6b27f5af97039 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -384,6 +384,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
-------------------------
@@ -544,6 +548,14 @@ Improvements to Clang's diagnostics
- Clang no longer emits a "declared here" note for a builtin function that has no declaration in source.
Fixes #GH93369.
+- 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`` (#GH34876). 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 ef9df1e9d8b4a..6bb64e46c7c3e 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -3122,6 +3122,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 f15cba63624ea..bf98c431a2b17 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 ded4f59833ac0..bbb614b78c9ca 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -6754,22 +6754,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..a7b4bd93b4b49 100644
--- a/clang/test/Sema/arm-interrupt-attr.c
+++ b/clang/test/Sema/arm-interrupt-attr.c
@@ -1,7 +1,7 @@
-// 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 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
__attribute__((interrupt(IRQ))) void foo(void) {} // expected-error {{'interrupt' attribute requires a string}}
@@ -23,7 +23,7 @@ __attribute__((interrupt(""))) void foo10(void) {}
// expected-note at +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) {
More information about the cfe-commits
mailing list