Warn when calling a non interrupt function from an interrupt on ARM

[jroelofs]

Summary

The idea for this originated from a really tricky bug: ISRs on ARM don't automatically save off the VFP regs, so if say, memcpy gets interrupted and the ISR itself calls memcpy, the regs are left clobbered when the ISR is done.

If the equivalent problem is likely to happen on other arches, I'd be happy to extend this to them. Suggestions on a more arch-neutral spelling of the warning itself would be helpful in that case.

[rengolin]

Seems like a very specific corner case on ARM, but is that attribute guaranteed to be ARM-only?

If so, LGTM as is. If not, avoid mentioning "VFP" on the error message.

[jroelofs]

In D28820#649384, @rengolin wrote:

Seems like a very specific corner case on ARM, but is that attribute guaranteed to be ARM-only?

If so, LGTM as is. If not, avoid mentioning "VFP" on the error message.

Yeah, the attribute is parsed in a target-specific way, so this warning as written won't match on other arches even though mips/x86 have their own similar spellings of it.

[jroelofs]

r292375

[efriedma]

Why can't the compiler handle this case itself transparently? According to your description, the interrupt calling convention is different from the normal hard-float AAPCS calling convention: the VFP registers are all callee-save. The compiler knows this; it should be able to insert the appropriate spills itself.

[jroelofs]

In D28820#649726, @efriedma wrote:

Why can't the compiler handle this case itself transparently? According to your description, the interrupt calling convention is different from the normal hard-float AAPCS calling convention: the VFP registers are all callee-save. The compiler knows this; it should be able to insert the appropriate spills itself.

There would be a big performance penalty for ISRs with callees that don't use VFP regs.

[jroelofs]

In D28820#649738, @jroelofs wrote:

In D28820#649726, @efriedma wrote:

Why can't the compiler handle this case itself transparently? According to your description, the interrupt calling convention is different from the normal hard-float AAPCS calling convention: the VFP registers are all callee-save. The compiler knows this; it should be able to insert the appropriate spills itself.

There would be a big performance penalty for ISRs with callees that don't use VFP regs.

Then again, it probably makes sense to add all the VFP regs to the ISR calling convention's CalleeSavedRegs, as the ISR /should/ be spilling them if it uses them.

[efriedma]

There would be a big performance penalty for ISRs with callees that don't use VFP regs.

Sacrificing correctness for the sake of performance seems like a bad idea... especially given that the optimizer can insert calls to memcpy where they didn't originally exist in the source.

[jroelofs]

In D28820#649770, @efriedma wrote:

There would be a big performance penalty for ISRs with callees that don't use VFP regs.

Sacrificing correctness for the sake of performance seems like a bad idea...

I don't quite see it that way, but I understand where you're coming from.

especially given that the optimizer can insert calls to memcpy where they didn't originally exist in the source.

That, however, is the nail in the coffin of my argument :)

[tyomitch]

When compiling for softfp targets, this new warning doesn't make sense: there are no VFP registers to save.
Jonathan, would you please conditionalize it to only affect hardfp targets?

[jroelofs]

In D28820#695356, @tyomitch wrote:

When compiling for softfp targets, this new warning doesn't make sense: there are no VFP registers to save.
Jonathan, would you please conditionalize it to only affect hardfp targets?

Sure, I can do that.

[kc.austin2017]

Hi , I am looking for some helps on the issue i have in my project after this change for the compiler.

typedef void (*callback_t)(uint32_t icciar, void * context);

callback_t callee1;
void _ _attribute((interrupt("IRQ"))) cs3_isr_irq() {

callee1(icciar , context);

}

the above code gives me the expected error " call to function without interrupt attribute could clobber interruptee's VFP registers"

below is my attempt

typedef void _ _attribute__((interrupt("IRQ"))) (*callback_t)(uint32_t icciar, void * context);

callback_t _ _attribute((interrupt("IRQ"))) callee1;
void _ _attribute((interrupt("IRQ"))) __cs3_isr_irq() {

callee1(icciar , context);

}

However, it is still giving me the error "" call to function without interrupt attribute could clobber interruptee's VFP registers"
What is the best way to modify the code for this compiler change ?

thanks all in advance !!

[efriedma]

What is the best way to modify the code for this compiler change ?

Currently, the "interrupt" attribute only has an effect on functions, not function pointers, so your code won't work the way you want. It's a bug that we don't emit a warning for this.

Currently, this warning doesn't have its own warning flag, instead being lumped under -Wextra. This is also a bug.

We don't emit the warning if your code is compiled for a target which doesn't support floating-point (-mfpu=none or -msoft-float); see https://reviews.llvm.org/D32918. But otherwise, if you're sure your code is actually correct, you can turn off the warning with -Wno-extra or something like that. (The whole thing is kind of awkward because the implementation of the interrupt attribute in clang is buggy: the frontend lies to the backend about the calling convention, so the backend can't save/restore VFP registers correctly.)

[kc.austin2017]

In D28820#944365, @efriedma wrote:

What is the best way to modify the code for this compiler change ?

Currently, the "interrupt" attribute only has an effect on functions, not function pointers, so your code won't work the way you want. It's a bug that we don't emit a warning for this.

Currently, this warning doesn't have its own warning flag, instead being lumped under -Wextra. This is also a bug.

We don't emit the warning if your code is compiled for a target which doesn't support floating-point (-mfpu=none or -msoft-float); see https://reviews.llvm.org/D32918. But otherwise, if you're sure your code is actually correct, you can turn off the warning with -Wno-extra or something like that. (The whole thing is kind of awkward because the implementation of the interrupt attribute in clang is buggy: the frontend lies to the backend about the calling convention, so the backend can't save/restore VFP registers correctly.)

so is there any plan to make the "interrupt" attribute not just support on functions?

[jroelofs]

In D28820#944365, @efriedma wrote:

What is the best way to modify the code for this compiler change ?

Currently, the "interrupt" attribute only has an effect on functions, not function pointers, so your code won't work the way you want. It's a bug that we don't emit a warning for this.

https://bugs.llvm.org/show_bug.cgi?id=35527

Currently, this warning doesn't have its own warning flag, instead being lumped under -Wextra. This is also a bug.

https://bugs.llvm.org/show_bug.cgi?id=35528

We don't emit the warning if your code is compiled for a target which doesn't support floating-point (-mfpu=none or -msoft-float); see https://reviews.llvm.org/D32918. But otherwise, if you're sure your code is actually correct, you can turn off the warning with -Wno-extra or something like that. (The whole thing is kind of awkward because the implementation of the interrupt attribute in clang is buggy: the frontend lies to the backend about the calling convention, so the backend can't save/restore VFP registers correctly.)

https://i.imgur.com/BFRoEUO.gif

In D28820#944706, @kc.austin2017 wrote:

so is there any plan to make the "interrupt" attribute not just support on functions?

I'll fix these. Give me a few weeks though.

[kc.austin2017]

In D28820#944865, @jroelofs wrote:

In D28820#944365, @efriedma wrote:

What is the best way to modify the code for this compiler change ?

Currently, the "interrupt" attribute only has an effect on functions, not function pointers, so your code won't work the way you want. It's a bug that we don't emit a warning for this.

https://bugs.llvm.org/show_bug.cgi?id=35527

Currently, this warning doesn't have its own warning flag, instead being lumped under -Wextra. This is also a bug.

https://bugs.llvm.org/show_bug.cgi?id=35528

We don't emit the warning if your code is compiled for a target which doesn't support floating-point (-mfpu=none or -msoft-float); see https://reviews.llvm.org/D32918. But otherwise, if you're sure your code is actually correct, you can turn off the warning with -Wno-extra or something like that. (The whole thing is kind of awkward because the implementation of the interrupt attribute in clang is buggy: the frontend lies to the backend about the calling convention, so the backend can't save/restore VFP registers correctly.)

https://i.imgur.com/BFRoEUO.gif

In D28820#944706, @kc.austin2017 wrote:

so is there any plan to make the "interrupt" attribute not just support on functions?

I'll fix these. Give me a few weeks though.

Thank you very much for recording the bug.

[jroelofs]

In D28820#945118, @kc.austin2017 wrote:

In D28820#944865, @jroelofs wrote:

https://bugs.llvm.org/show_bug.cgi?id=35527

https://bugs.llvm.org/show_bug.cgi?id=35528

I'll fix these. Give me a few weeks though.

Thank you very much for recording the bug.

patch to fix these: https://reviews.llvm.org/D74812

include/clang/Basic/DiagnosticSemaKinds.td

 def err_parameter_name_omitted : Error<"parameter name omitted">;
 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_arm_interrupt_calling_convention : Warning<
+   "call to function without interrupt attribute could clobber interruptee's VFP registers">,
+   InGroup<Extra>;
 def warn_mips_interrupt_attribute : Warning<
    "MIPS 'interrupt' attribute only applies to functions that have "
    "%select{no parameters|a 'void' return type}0">,
    InGroup<IgnoredAttributes>;
 def warn_unused_parameter : Warning<"unused parameter %0">,
   InGroup<UnusedParameter>, DefaultIgnore;
 def warn_unused_variable : Warning<"unused variable %0">,
   InGroup<UnusedVariable>, DefaultIgnore;

lib/Sema/SemaExpr.cpp

   unsigned BuiltinID = (FDecl ? FDecl->getBuiltinID() : 0);
 
   // Functions with 'interrupt' attribute cannot be called directly.
   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())
+    if (Caller->hasAttr<ARMInterruptAttr>())
+      if (!FDecl->hasAttr<ARMInterruptAttr>())
+        Diag(Fn->getExprLoc(), diag::warn_arm_interrupt_calling_convention);
+
   // 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;
   if (BuiltinID &&
       Fn->getType()->isSpecificBuiltinType(BuiltinType::BuiltinFn)) {
     Result = ImpCastExprToType(Fn, Context.getPointerType(FDecl->getType()),
                                CK_BuiltinFnToFnPtr).get();

test/Sema/arm-interrupt-attr.c

 __attribute__((interrupt("FIQ"))) void foo4() {}
 __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() {}
+
+void callee1();
+__attribute__((interrupt("IRQ"))) void callee2();
+void caller1() {
+  callee1();
+  callee2();
+}
+__attribute__((interrupt("IRQ"))) void caller2() {
+  callee1(); // expected-warning {{call to function without interrupt attribute could clobber interruptee's VFP registers}}
+  callee2();
+}