[clang][patch] To solve PR26413, x86 interrupt routines may only call routines with no_saved_reg

[mibintc]

Summary

In the bug report, @DavidKreitzer wrote:
clang should be giving an error for this test, because we have no good way to efficiently save & restore the non-GPR state.

The interrupt handler is required to save & restore all the register state that it uses. And according to the ABI, the call to subroutine1() may clobber arbitrary XMM, YMM, or ZMM state. The only way to reliably save & restore that state is to use xsave/xrstor, which would be very inefficient and is probably not what we want.

This patch implements the check described. There's a similar check for arm architecture and they give a warning. Folks tend to ignore warnings so I'll propose this as an error in the first go-round. @ABataev wrote a similar error check so I'll add him as reviewer

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

297–298

The diagnostic wording needs a bit more updating though, see below.

clang/lib/Sema/SemaExpr.cpp

6575

I think we should pass in FDecl when emitting the diagnostic because it's possible that there are multiple function calls where only one is problematic. e.g.,

__attribute__((interrupt)) void fooA(void) {
  fine(okay(12), bad(14)); // Would be nice to identify 'bad' as the issue
}

However, I'm not certain what to say when FDecl is null (I don't know what circumstances that happens).

[mibintc]

@aaron.ballman Is this what you have in mind to improve the diagnostic? There are other places in the file that assume FDecl can be null so I just copied that code. Also since the arm warning diagnostic is exactly similar, I added the additional information, note diagnostic, showing where the callee is declared.

[aaron.ballman]

LGTM aside from a small change to the diagnostic (routine -> a function, and adding single quotes around the syntax element in the diagnostic).

clang/include/clang/Basic/DiagnosticSemaKinds.td

297–298

New suggested diagnostic wording.

[mibintc]

Thanks I'll wait to see if there are any more remarks, and push it later today or tomorrow

[mibintc]

I received a bug report that this patch creates error diagnostics for calls to a builtin, like calling 'abort', 'exit' or one of the target builtins like __builtin_ia32_packssw then that call should be allowed without remark but this patch causes the compilation to fail. We could require that all builtin's be declared with "no caller saved reg" but that's a big modification. I'm planning to make a change which igores builtin calls, but continue to error on any implicitly declared or user declared functions without the "no caller saved reg" attribute.

[craig.topper]

In D97764#2685655, @mibintc wrote:

I received a bug report that this patch creates error diagnostics for calls to a builtin, like calling 'abort', 'exit' or one of the target builtins like __builtin_ia32_packssw then that call should be allowed without remark but this patch causes the compilation to fail. We could require that all builtin's be declared with "no caller saved reg" but that's a big modification. I'm planning to make a change which igores builtin calls, but continue to error on any implicitly declared or user declared functions without the "no caller saved reg" attribute.

Builtins like __builtin_ia32_packssw aren't usually called directly. The user should be calling the always_inline wrapper functions in x86intrin.h. Would that also fail?

[mibintc]

In D97764#2685742, @craig.topper wrote:

In D97764#2685655, @mibintc wrote:

I received a bug report that this patch creates error diagnostics for calls to a builtin, like calling 'abort', 'exit' or one of the target builtins like __builtin_ia32_packssw then that call should be allowed without remark but this patch causes the compilation to fail. We could require that all builtin's be declared with "no caller saved reg" but that's a big modification. I'm planning to make a change which igores builtin calls, but continue to error on any implicitly declared or user declared functions without the "no caller saved reg" attribute.

Builtins like __builtin_ia32_packssw aren't usually called directly. The user should be calling the always_inline wrapper functions in x86intrin.h. Would that also fail?

Thanks for the suggestion Craig! I could change the check to look for "always_inline" [that means they're always inlined and the call would never be created even with optimization disabled right? ] However, I don't see _builtin_ia32_packssw declared in <immintrin.h>. In immintrin it is declared implicitly when the usage is seen for example
_mm_packs_pi16(m64 m1, m64 m2)
{

return (__m64)__builtin_ia32_packsswb((__v4hi)__m1, (__v4hi)__m2);

}

Is it a bug that __builtin_ia32_packsswb is omitted from the x86intrin declarations? Is there some tricky reason it's not declared?

[craig.topper]

In D97764#2685841, @mibintc wrote:

In D97764#2685742, @craig.topper wrote:

In D97764#2685655, @mibintc wrote:

I received a bug report that this patch creates error diagnostics for calls to a builtin, like calling 'abort', 'exit' or one of the target builtins like __builtin_ia32_packssw then that call should be allowed without remark but this patch causes the compilation to fail. We could require that all builtin's be declared with "no caller saved reg" but that's a big modification. I'm planning to make a change which igores builtin calls, but continue to error on any implicitly declared or user declared functions without the "no caller saved reg" attribute.

Builtins like __builtin_ia32_packssw aren't usually called directly. The user should be calling the always_inline wrapper functions in x86intrin.h. Would that also fail?

Thanks for the suggestion Craig! I could change the check to look for "always_inline" [that means they're always inlined and the call would never be created even with optimization disabled right? ] However, I don't see _builtin_ia32_packssw declared in <immintrin.h>. In immintrin it is declared implicitly when the usage is seen for example
_mm_packs_pi16(m64 m1, m64 m2)
{

return (__m64)__builtin_ia32_packsswb((__v4hi)__m1, (__v4hi)__m2);

}

Is it a bug that __builtin_ia32_packsswb is omitted from the x86intrin declarations? Is there some tricky reason it's not declared?

__builtin_ia32* are always defined for the X86 target as they are part of the compiler. They are defined in clang/include/clang/Basic/BuiltinsX86.def and clang/include/clang/Basic/BuiltinsX86_64.def

clang/include/clang/Basic/DiagnosticSemaKinds.td

def ext_parameter_name_omitted_c2x : ExtWarn<
   InGroup<C2x>;
 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 err_anyx86_interrupt_regsave : Error<
+  "interrupt service routine may only call a function"
+  " with attribute 'no_caller_saved_registers'">;
 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 (FDecl && FDecl->hasAttr<AnyX86InterruptAttr>()) {
     Diag(Fn->getExprLoc(), diag::err_anyx86_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.
-  if (auto *Caller = getCurFunctionDecl())
+  // 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()) {
     if (Caller->hasAttr<ARMInterruptAttr>()) {
       bool VFP = Context.getTargetInfo().hasFeature("vfp");
-      if (VFP && (!FDecl || !FDecl->hasAttr<ARMInterruptAttr>()))
+      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::err_anyx86_interrupt_regsave);
+      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)) {
     // Extract the return type from the (builtin) function pointer type.
     // FIXME Several builtins still have setType in

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

 __attribute__((interrupt("SWI"))) void foo5() {}
 __attribute__((interrupt("ABORT"))) void foo6() {}
 __attribute__((interrupt("UNDEF"))) void foo7() {}
 
 __attribute__((interrupt)) void foo8() {}
 __attribute__((interrupt())) void foo9() {}
 __attribute__((interrupt(""))) void foo10() {}
 
+#ifndef SOFT
+// expected-note@+2 {{'callee1' declared here}}
+#endif
 void callee1();
 __attribute__((interrupt("IRQ"))) void callee2();
 void caller1() {
   callee1();
   callee2();
 }
 
 #ifndef SOFT

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

 // RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu  -fsyntax-only -verify %s
 // RUN: %clang_cc1 -triple i386-unknown-linux-gnu  -fsyntax-only -verify %s
 // RUN: %clang_cc1 -triple x86_64-pc-win32  -fsyntax-only -verify %s
 // RUN: %clang_cc1 -triple i386-pc-win32  -fsyntax-only -verify %s
 // RUN: %clang_cc1 -triple x86_64-unknown-linux-gnux32  -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu  -fsyntax-only -verify %s -DNOCALLERSAVE=1
 
 struct a {
   int b;
 };
 
 struct a test __attribute__((interrupt)); // expected-warning {{'interrupt' attribute only applies to non-K&R-style functions}}
 
 __attribute__((interrupt)) int foo1(void) { return 0; }             // expected-error-re {{{{(x86|x86-64)}} 'interrupt' attribute only applies to functions that have a 'void' return type}}

 #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-error@+4 {{interrupt service routine may 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;
 }