[clang] fa1dc06 - [clang][X86] Update excessive register save diagnostic to more closely follow the interrupt attribute spec
Phoebe Wang via cfe-commits
cfe-commits at lists.llvm.org
Tue Aug 29 20:52:12 PDT 2023
Author: Antonio Abbatangelo
Date: 2023-08-30T11:52:04+08:00
New Revision: fa1dc06a1b3997f025b0d81539c31a74e6b6ec79
URL: https://github.com/llvm/llvm-project/commit/fa1dc06a1b3997f025b0d81539c31a74e6b6ec79
DIFF: https://github.com/llvm/llvm-project/commit/fa1dc06a1b3997f025b0d81539c31a74e6b6ec79.diff
LOG: [clang][X86] Update excessive register save diagnostic to more closely follow the interrupt attribute spec
The original diagnostic does not cover all cases according to my reading
of the spec.
For the interrupt attribute, the spec indicates that if the compiler
does not support saving SSE, MMX, or x87 then the function should
be compiled with '-mgeneral-regs-only' (GCC requires this).
Alternatively, calling functions with the `no_caller_saved_registers`
attribute will not clobber state and can be done without disabling
these features.
The warning as implemented in upstream only detects the latter case but
does not consider that disabling the above features also solves the issue
of these register saves being undesirable due to inefficiency.
For the no_caller_saved_registers attribute, the interrupt spec also
indicates that in the absence of saving SSE, MMX and x87 state, these
functions should be compiled with '-mgeneral-regs-only' (also required
by GCC). It does not make any statements about calling other functions
with the attribute, but by extension the result is the same as with the
interrupt attribute (in clang, at least).
This patch handles the remaining cases by adjusting the diagnostic to:
1. Not be shown if the function is compiled without the SSE, MMX or x87
features enabled (i.e. with '-mgeneral-regs-only')
2. Also be shown for functions with the 'no_caller_saved_registers'
attribute
3. In addition to advising that the function should only call functions
with the `no_caller_saved_registers` attribute, the text also suggests
compiling with `-mgeneral-regs-only` as an alternative.
The interrupt spec is available at https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=5ed3cc7b66af4758f7849ed6f65f4365be8223be
and was taken from the issue that resulted in this diagnostic being
added (#26787)
Reviewed By: pengfei
Differential Revision: https://reviews.llvm.org/D159068
Added:
clang/test/Sema/x86-diag-excessive-regsave.c
Modified:
clang/docs/ReleaseNotes.rst
clang/include/clang/Basic/AttrDocs.td
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Sema/SemaExpr.cpp
clang/test/Sema/attr-x86-interrupt.c
Removed:
################################################################################
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 262bebfc0bde9b..1bd295c68b83b7 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -122,12 +122,18 @@ Modified Compiler Flags
-----------------------
* ``-Woverriding-t-option`` is renamed to ``-Woverriding-option``.
+* ``-Winterrupt-service-routine`` is renamed to ``-Wexcessive-regsave`` as a generalization
Removed Compiler Flags
-------------------------
Attribute Changes in Clang
--------------------------
+- On X86, a warning is now emitted if a function with ``__attribute__((no_caller_saved_registers))``
+ calls a function without ``__attribute__((no_caller_saved_registers))``, and is not compiled with
+ ``-mgeneral-regs-only``
+- On X86, a function with ``__attribute__((interrupt))`` can now call a function without
+ ``__attribute__((no_caller_saved_registers))`` provided that it is compiled with ``-mgeneral-regs-only``
- When a non-variadic function is decorated with the ``format`` attribute,
Clang now checks that the format string would match the function's parameters'
diff --git a/clang/include/clang/Basic/AttrDocs.td b/clang/include/clang/Basic/AttrDocs.td
index f19fe66bfedc1f..f11ea89d14bad0 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -4815,6 +4815,10 @@ The user can call functions specified with the 'no_caller_saved_registers'
attribute from an interrupt handler without saving and restoring all
call-clobbered registers.
+Functions specified with the 'no_caller_saved_registers' attribute should only
+call other functions with the 'no_caller_saved_registers' attribute, or should be
+compiled with the '-mgeneral-regs-only' flag to avoid saving unused non-GPR registers.
+
Note that 'no_caller_saved_registers' attribute is not a calling convention.
In fact, it only overrides the decision of which registers should be saved by
the caller, but not how the parameters are passed from the caller to the callee.
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index a065015cfe02eb..0ac4df8edb242f 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -310,10 +310,12 @@ def err_anyx86_interrupt_attribute : Error<
"a pointer as the first parameter|a %2 type as the second parameter}1">;
def err_anyx86_interrupt_called : Error<
"interrupt service routine cannot be called directly">;
-def warn_anyx86_interrupt_regsave : Warning<
- "interrupt service routine should only call a function"
- " with attribute 'no_caller_saved_registers'">,
- InGroup<DiagGroup<"interrupt-service-routine">>;
+def warn_anyx86_excessive_regsave : Warning<
+ "%select{interrupt service routine|function with attribute"
+ " 'no_caller_saved_registers'}0 should only call a function"
+ " 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>;
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index f9badf4ede847f..1d457e116bb5b1 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -7413,11 +7413,18 @@ ExprResult Sema::BuildResolvedCallExpr(Expr *Fn, NamedDecl *NDecl,
Diag(FDecl->getLocation(), diag::note_callee_decl) << FDecl;
}
}
- if (Caller->hasAttr<AnyX86InterruptAttr>() &&
- ((!FDecl || !FDecl->hasAttr<AnyX86NoCallerSavedRegistersAttr>()))) {
- Diag(Fn->getExprLoc(), diag::warn_anyx86_interrupt_regsave);
- if (FDecl)
- Diag(FDecl->getLocation(), diag::note_callee_decl) << FDecl;
+ if (Caller->hasAttr<AnyX86InterruptAttr>() ||
+ Caller->hasAttr<AnyX86NoCallerSavedRegistersAttr>()) {
+ const TargetInfo &TI = Context.getTargetInfo();
+ bool HasNonGPRRegisters =
+ TI.hasFeature("sse") || TI.hasFeature("x87") || TI.hasFeature("mmx");
+ if (HasNonGPRRegisters &&
+ (!FDecl || !FDecl->hasAttr<AnyX86NoCallerSavedRegistersAttr>())) {
+ Diag(Fn->getExprLoc(), diag::warn_anyx86_excessive_regsave)
+ << (Caller->hasAttr<AnyX86InterruptAttr>() ? 0 : 1);
+ if (FDecl)
+ Diag(FDecl->getLocation(), diag::note_callee_decl) << FDecl;
+ }
}
}
diff --git a/clang/test/Sema/attr-x86-interrupt.c b/clang/test/Sema/attr-x86-interrupt.c
index 564704a5647700..84c60227135fe9 100644
--- a/clang/test/Sema/attr-x86-interrupt.c
+++ b/clang/test/Sema/attr-x86-interrupt.c
@@ -40,23 +40,6 @@ __attribute__((interrupt)) void foo6(float *a, int b) {}
__attribute__((interrupt)) void foo7(int *a, unsigned b) {}
__attribute__((interrupt)) void foo8(int *a) {}
-#ifdef _LP64
-typedef unsigned long Arg2Type;
-#elif defined(__x86_64__)
-typedef unsigned long long Arg2Type;
-#else
-typedef unsigned int Arg2Type;
-#endif
-#ifndef NOCALLERSAVE
-__attribute__((no_caller_saved_registers))
-#else
-// expected-note at +3 {{'foo9' declared here}}
-// expected-warning at +4 {{interrupt service routine should only call a function with attribute 'no_caller_saved_registers'}}
-#endif
-void foo9(int *a, Arg2Type b) {}
-__attribute__((interrupt)) void fooA(int *a, Arg2Type b) {
- foo9(a, b);
-}
void g(void (*fp)(int *));
int main(int argc, char **argv) {
void *ptr = (void *)&foo7;
diff --git a/clang/test/Sema/x86-diag-excessive-regsave.c b/clang/test/Sema/x86-diag-excessive-regsave.c
new file mode 100644
index 00000000000000..8b580e60c54514
--- /dev/null
+++ b/clang/test/Sema/x86-diag-excessive-regsave.c
@@ -0,0 +1,34 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fsyntax-only -verify %s -DNO_CALLER_SAVED_REGISTERS=1
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -target-feature -x87 -target-feature -mmx -target-feature -sse -fsyntax-only -verify %s -DGPRONLY=1
+
+#if defined(NO_CALLER_SAVED_REGS) || defined(GPRONLY)
+// expected-no-diagnostics
+#else
+#define EXPECT_WARNING
+#endif
+
+#ifdef NO_CALLER_SAVED_REGS
+__attribute__((no_caller_saved_registers))
+#endif
+#ifdef EXPECT_WARNING
+// expected-note at +3 {{'foo' declared here}}
+// expected-note at +2 {{'foo' declared here}}
+#endif
+extern void foo(void *);
+
+__attribute__((no_caller_saved_registers))
+void no_caller_saved_registers(void *arg) {
+#ifdef EXPECT_WARNING
+// expected-warning at +2 {{function with attribute 'no_caller_saved_registers' should only call a function with attribute 'no_caller_saved_registers' or be compiled with '-mgeneral-regs-only'}}
+#endif
+ foo(arg);
+}
+
+__attribute__((interrupt))
+void interrupt(void *arg) {
+#ifdef EXPECT_WARNING
+// expected-warning at +2 {{interrupt service routine should only call a function with attribute 'no_caller_saved_registers' or be compiled with '-mgeneral-regs-only'}}
+#endif
+ foo(arg);
+}
More information about the cfe-commits
mailing list