Page MenuHomePhabricator

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

Authored by mibintc on Mar 2 2021, 5:16 AM.

Details

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

Diff Detail

Unit TestsFailed

TimeTest
30 msx64 windows > LLVM.ExecutionEngine/JITLink/AArch64::MachO_arm64_ehframe.test
Script: -- : 'RUN: at line 1'; c:\ws\w64\llvm-project\premerge-checks\build\bin\llvm-jitlink.exe -noexec C:\ws\w64\llvm-project\premerge-checks\llvm\test\ExecutionEngine\JITLink\AArch64/Inputs/MachO_arm64_ehframe.o
40 msx64 windows > LLVM.ExecutionEngine/JITLink/AArch64::MachO_arm64_relocations.s
Script: -- : 'RUN: at line 1'; rm -rf C:\ws\w64\llvm-project\premerge-checks\build\test\ExecutionEngine\JITLink\AArch64\Output\MachO_arm64_relocations.s.tmp && mkdir -p C:\ws\w64\llvm-project\premerge-checks\build\test\ExecutionEngine\JITLink\AArch64\Output\MachO_arm64_relocations.s.tmp
50 msx64 windows > LLVM.ExecutionEngine/JITLink/X86::ELF_skip_debug_sections.s
Script: -- : 'RUN: at line 2'; c:\ws\w64\llvm-project\premerge-checks\build\bin\llvm-mc.exe -triple=x86_64-pc-linux-gnu -filetype=obj -o C:\ws\w64\llvm-project\premerge-checks\build\test\ExecutionEngine\JITLink\X86\Output\ELF_skip_debug_sections.s.tmp C:\ws\w64\llvm-project\premerge-checks\llvm\test\ExecutionEngine\JITLink\X86\ELF_skip_debug_sections.s
60 msx64 windows > LLVM.ExecutionEngine/JITLink/X86::ELF_weak_definitions.s
Script: -- : 'RUN: at line 1'; rm -rf C:\ws\w64\llvm-project\premerge-checks\build\test\ExecutionEngine\JITLink\X86\Output\ELF_weak_definitions.s.tmp && mkdir -p C:\ws\w64\llvm-project\premerge-checks\build\test\ExecutionEngine\JITLink\X86\Output\ELF_weak_definitions.s.tmp
40 msx64 windows > LLVM.ExecutionEngine/JITLink/X86::ELF_x86-64_common.s
Script: -- : 'RUN: at line 1'; rm -rf C:\ws\w64\llvm-project\premerge-checks\build\test\ExecutionEngine\JITLink\X86\Output\ELF_x86-64_common.s.tmp && mkdir -p C:\ws\w64\llvm-project\premerge-checks\build\test\ExecutionEngine\JITLink\X86\Output\ELF_x86-64_common.s.tmp
View Full Test Results (19 Failed)

Event Timeline

mibintc created this revision.Mar 2 2021, 5:16 AM
mibintc requested review of this revision.Mar 2 2021, 5:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2021, 5:16 AM
aaron.ballman added inline comments.Mar 2 2021, 5:42 AM
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 updated this revision to Diff 327446.Mar 2 2021, 7:19 AM

@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 accepted this revision.Mar 2 2021, 8:00 AM

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.

This revision is now accepted and ready to land.Mar 2 2021, 8:00 AM
mibintc updated this revision to Diff 327465.Mar 2 2021, 8:19 AM

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

This revision was landed with ongoing or failed builds.Mar 3 2021, 7:12 AM
This revision was automatically updated to reflect the committed changes.

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.

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?

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?

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