[clang][X86] Update excessive register save diagnostic to more closely follow the interrupt attribute spec

[antangelo]

Summary

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)

[aaron.ballman]

Changing the reviewers up a bit; the changes generally seem reasonable to me, but I'd appreciate some x86 reviewer viewpoints.

clang/include/clang/Basic/DiagnosticSemaKinds.td

317

Can you add a test case showing that -mgeneral-regs-only causes the diagnostic to be silenced?

[pengfei]

clang/include/clang/Basic/DiagnosticSemaKinds.td

317

It's a driver only option. I think we are not suggested to invoke %clang out of driver testsing, right?

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

317

Oh! I hadn't realized this was a driver-only option. Looking at it harder, I see that for x86, setting the driver option removes the x87, mmx, and sse features which we do have test coverage for. Thanks @pengfei for pointing that out, my request was already covered!

[pengfei]

I don't have prior experience about interrupt diagnostic but know something about mgeneral-regs-only. I think the diagnostic great.

[antangelo]

Thank you for the quick reviews! I don't have currently have commit access to commit the changes myself, is it possible for someone to commit on my behalf? My name and email are:

Name: Antonio Abbatangelo
Email: [e-mail]

clang/docs/ReleaseNotes.rst

 
 Deprecated Compiler Flags
 -------------------------
 
 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'
   types after default argument promotion. As a result, it's no longer an
   automatic diagnostic to use parameters of types that the format style
   supports but that are never the result of default argument promotion, such as
   ``float``. (`#59824: <https://github.com/llvm/llvm-project/issues/59824>`_)
 

clang/include/clang/Basic/AttrDocs.td

 from the function.
 The compiler saves and restores any modified registers that were not used for
 passing or returning arguments to the function.
 
 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.
 
 For example:
 
   .. code-block:: c
 

clang/include/clang/Basic/DiagnosticSemaKinds.td

def ext_parameter_name_omitted_c23 : ExtWarn<
   InGroup<C23>;
 def err_anyx86_interrupt_attribute : Error<
   "%select{x86|x86-64}0 'interrupt' attribute only applies to functions that "
   "have %select{a 'void' return type|"
   "only a pointer parameter optionally followed by an integer parameter|"
   "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>;
 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">,
    InGroup<IgnoredAttributes>;
 def warn_riscv_repeated_interrupt_attribute : Warning<

clang/lib/Sema/SemaExpr.cpp

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>() &&
-        ((!FDecl || !FDecl->hasAttr<AnyX86NoCallerSavedRegistersAttr>()))) {
-      Diag(Fn->getExprLoc(), diag::warn_anyx86_interrupt_regsave);
+    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;
       }
     }
+  }
 
   // Promote the function operand.
   // We special-case function promotion here because we only allow promoting
   // builtin functions to function pointers in the callee of a call.
   ExprResult Result;
   QualType ResultTy;
   if (BuiltinID &&
       Fn->getType()->isSpecificBuiltinType(BuiltinType::BuiltinFn)) {

clang/test/Sema/attr-x86-interrupt.c

 #ifdef _LP64
 // expected-error-re@+4 {{{{(x86|x86-64)}} 'interrupt' attribute only applies to functions that have a 'unsigned long' type as the second parameter}}
 #elif defined(__x86_64__)
 // expected-error-re@+2 {{{{(x86|x86-64)}} 'interrupt' attribute only applies to functions that have a 'unsigned long long' type as the second parameter}}
 #endif
 __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@+3 {{'foo9' declared here}}
-// expected-warning@+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;
   g(foo8);
 
   (void)ptr;
 #ifndef __x86_64__
   // expected-error@+2 {{interrupt service routine cannot be called directly}}
 #endif
   foo7((int *)argv, argc);
   foo8((int *)argv);       // expected-error {{interrupt service routine cannot be called directly}}
   return 0;
 }
 

clang/test/Sema/x86-diag-excessive-regsave.c

+// 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@+3 {{'foo' declared here}}
+// expected-note@+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@+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@+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);
+}