From 588a6d7de612bd19dc262563978711233b74e783 Mon Sep 17 00:00:00 2001 From: Chris Copeland Date: Wed, 10 Jul 2024 08:32:48 -0700 Subject: [PATCH] [clang][ARM] Fix warning for using VFP from interrupts. (#91870) [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. --- clang/docs/ReleaseNotes.rst | 12 +++++ .../clang/Basic/DiagnosticSemaKinds.td | 9 ++-- clang/lib/Sema/SemaARM.cpp | 4 ++ clang/lib/Sema/SemaExpr.cpp | 26 ++++------ clang/test/Sema/arm-interrupt-attr.c | 50 +++++-------------- 5 files changed, 44 insertions(+), 57 deletions(-) diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 0730edabcf37..d0930ccfc603 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -457,6 +457,9 @@ New Compiler Flags that relies on it. Users should carefully consider this possibiilty when using the flag. +- 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 ------------------------- @@ -499,6 +502,13 @@ Modified Compiler Flags now include dianostics about C++26 features that are not present in older versions. +- 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 ------------------------- @@ -693,6 +703,8 @@ Improvements to Clang's diagnostics - Clang no longer emits a "no previous prototype" warning for Win32 entry points under ``-Wmissing-prototypes``. Fixes #GH94366. +- 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 fa02622f1227..940f9ac226ca 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -336,9 +336,12 @@ def warn_anyx86_excessive_regsave : Warning< " with attribute 'no_caller_saved_registers'" " or be compiled with '-mgeneral-regs-only'">, InGroup>; -def warn_arm_interrupt_calling_convention : Warning< - "call to function without interrupt attribute could clobber interruptee's VFP registers">, - InGroup; +def warn_arm_interrupt_vfp_clobber : Warning< + "interrupt service routine with vfp enabled may clobber the " + "interruptee's vfp state">, + InGroup>; +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/SemaARM.cpp b/clang/lib/Sema/SemaARM.cpp index 370db341e997..d8dd4fe16e3a 100644 --- a/clang/lib/Sema/SemaARM.cpp +++ b/clang/lib/Sema/SemaARM.cpp @@ -1329,6 +1329,10 @@ 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 45991e66b3e4..63496b327da4 100644 --- a/clang/lib/Sema/SemaExpr.cpp +++ b/clang/lib/Sema/SemaExpr.cpp @@ -6625,27 +6625,21 @@ 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()) { - Diag(Fn->getExprLoc(), diag::err_anyx86_interrupt_called); - return ExprError(); + if (FDecl) { + if (FDecl->hasAttr()) { + Diag(Fn->getExprLoc(), diag::err_anyx86_interrupt_called); + return ExprError(); + } + if (FDecl->hasAttr()) { + Diag(Fn->getExprLoc(), diag::err_arm_interrupt_called); + 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()) { - bool VFP = Context.getTargetInfo().hasFeature("vfp"); - if (VFP && (!FDecl || !FDecl->hasAttr())) { - Diag(Fn->getExprLoc(), diag::warn_arm_interrupt_calling_convention); - if (FDecl) - Diag(FDecl->getLocation(), diag::note_callee_decl) << FDecl; - } - } if (Caller->hasAttr() || Caller->hasAttr()) { const TargetInfo &TI = Context.getTargetInfo(); diff --git a/clang/test/Sema/arm-interrupt-attr.c b/clang/test/Sema/arm-interrupt-attr.c index 3537fba8521a..f2698eedacea 100644 --- a/clang/test/Sema/arm-interrupt-attr.c +++ b/clang/test/Sema/arm-interrupt-attr.c @@ -1,52 +1,26 @@ -// 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 -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}} + +#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}} - __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(); -} +__attribute__((interrupt("IRQ"))) void callee(void) {} -#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 caller(void) +{ + callee(); // expected-error {{interrupt service routine cannot be called directly}} } - -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(); -} -#endif +#endif // __ARM_FP