Fix Windows EH IP2State tables (remove +1 bias) #144745
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-debuginfo @llvm/pr-subscribers-backend-x86 Author: None (sivadeilra) ChangesThis fixes a long-standing bug in LLVM's support for generating Windows EH tables, specifically the IP2State tables. On most Windows platforms (AMD64, ARM64, ARM32, IA64, but not x86-32), exception handling works by looking up instruction pointers in lookup tables. These lookup tables are stored in If a function has any instructions that require cleanup during exception unwinding, then it will have an IP2State table. Each entry in the IP2State table describes a range of bytes in the function's instruction stream, and associates an "EH state number" with that range of instructions. A value of -1 means "the null state", which does not require any code to execute. A value other than -1 is an index into the State table. The entries in the IP2State table contain byte offsets within the instruction stream of the function. The Windows ABI requires that these offsets are aligned to instruction boundaries; they are not permitted to point to a byte that is not the first byte of an instruction. Unfortunately, CALL instructions present a problem during unwinding. CALL instructions push the address of the instruction after the CALL instruction, so that execution can resume after the CALL. If the CALL is the last instruction within an IP2State region, then the return address (on the stack) points to the next IP2State region. This means that the unwinder will use the wrong cleanup funclet during unwinding. To fix this problem, MSVC will insert a NOP after a CALL instruction, if the CALL instruction is the last instruction within an IP2State region. The NOP is placed within the same IP2State region as the CALL, so that the return address points to the NOP and the unwinder will locate the correct region. Previously, LLVM fixed this by adding 1 to the instruction offsets in the IP2State table. This caused the instruction boundary to point within the instruction after a CALL. This works for the purposes of unwinding, since there are no AMD64 instructions that can be encoded in a single byte and which throw C++ exceptions. Unfortunately, this violates the Windows ABI specification, which requires that the IP2State table entries point to the boundaries between exceptions. To fix this properly, LLVM will now insert a 1-byte NOP after CALL instructions, in the same situations that MSVC does. In performance tests, the NOP has no detectable significance. The NOP is rarely inserted, since it is only inserted when the CALL is the last instruction before an IP2State transition or the CALL is the last instruction before the function epilogue. NOP padding is only necessary on Windows AMD64 targets. On ARM64 and ARM32, instructions have a fixed size so the unwinder knows how to "back up" by one instruction. Interaction with Import Call Optimization (ICO): Import Call Optimization (ICO) is a compiler + OS feature on Windows which improves the performance and security of DLL imports. ICO relies on using a specific CALL idiom that can be replaced by the OS DLL loader. This removes a load and indirect CALL and replaces it with a single direct CALL. To achieve this, ICO also inserts NOPs after the CALL instruction. If the end of the CALL is aligned with an EH state transition, we also insert a single-byte NOP. Both forms of NOPs must be preserved. They cannot be combined into a single larger NOP; nor can the second NOP be removed. This is necessary because, if ICO is active and the call site is modified by the loader, the loader will end up overwriting the NOPs that were inserted for ICO. That means that those NOPs cannot be used for the correct termination of the exception handling region (the IP2State transition), so we still need an additional NOP instruction. The NOPs cannot be combined into a longer NOP (which is ordinarily desirable) because then ICO would split one instruction, producing a malformed instruction after the ICO call. Patch is 31.56 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/144745.diff 9 Files Affected:
diff --git a/clang/test/CodeGenCXX/microsoft-abi-eh-async.cpp b/clang/test/CodeGenCXX/microsoft-abi-eh-async.cpp
new file mode 100644
index 0000000000000..00bcdaf55aacc
--- /dev/null
+++ b/clang/test/CodeGenCXX/microsoft-abi-eh-async.cpp
@@ -0,0 +1,180 @@
+// RUN: %clang_cl -c --target=x86_64-windows-msvc /EHa -O2 /GS- \
+// RUN: -Xclang=-import-call-optimization \
+// RUN: /clang:-S /clang:-o- %s 2>&1 \
+// RUN: | FileCheck %s
+
+#ifdef __clang__
+#define NO_TAIL __attribute((disable_tail_calls))
+#else
+#define NO_TAIL
+#endif
+
+void might_throw();
+void other_func(int x);
+
+void does_not_throw() noexcept(true);
+
+extern "C" void __declspec(dllimport) some_dll_import();
+
+class HasDtor {
+ int x;
+ char foo[40];
+
+public:
+ explicit HasDtor(int x);
+ ~HasDtor();
+};
+
+class BadError {
+public:
+ int errorCode;
+};
+
+void normal_has_regions() {
+ // CHECK-LABEL: .def "?normal_has_regions@@YAXXZ"
+ // CHECK: .seh_endprologue
+
+ // <-- state -1 (none)
+ {
+ HasDtor hd{42};
+
+ // <-- state goes from -1 to 0
+ // because state changes, we expect the HasDtor::HasDtor() call to have a NOP
+ // CHECK: call "??0HasDtor@@QEAA@H@Z"
+ // CHECK-NEXT: nop
+
+ might_throw();
+ // CHECK: call "?might_throw@@YAXXZ"
+ // CHECK-NEXT: nop
+
+ // <-- state goes from 0 to -1 because we're about to call HasDtor::~HasDtor()
+ // CHECK: call "??1HasDtor@@QEAA@XZ"
+ // <-- state -1
+ }
+
+ // <-- state -1
+ other_func(10);
+ // CHECK: call "?other_func@@YAXH@Z"
+ // CHECK-NEXT: nop
+ // CHECK: .seh_startepilogue
+
+ // <-- state -1
+}
+
+// This tests a tail call to a destructor.
+void case_dtor_arg_empty_body(HasDtor x)
+{
+ // CHECK-LABEL: .def "?case_dtor_arg_empty_body@@YAXVHasDtor@@@Z"
+ // CHECK: jmp "??1HasDtor@@QEAA@XZ"
+}
+
+int case_dtor_arg_empty_with_ret(HasDtor x)
+{
+ // CHECK-LABEL: .def "?case_dtor_arg_empty_with_ret@@YAHVHasDtor@@@Z"
+ // CHECK: .seh_endprologue
+
+ // CHECK: call "??1HasDtor@@QEAA@XZ"
+ // CHECK-NOT: nop
+
+ // The call to HasDtor::~HasDtor() should NOT have a NOP because the
+ // following "mov eax, 100" instruction is in the same EH state.
+
+ return 100;
+
+ // CHECK: mov eax, 100
+ // CHECK: .seh_startepilogue
+ // CHECK: .seh_endepilogue
+ // CHECK: .seh_endproc
+}
+
+int case_noexcept_dtor(HasDtor x) noexcept(true)
+{
+ // CHECK: .def "?case_noexcept_dtor@@YAHVHasDtor@@@Z"
+ // CHECK: call "??1HasDtor@@QEAA@XZ"
+ // CHECK-NEXT: mov eax, 100
+ // CHECK: .seh_startepilogue
+ return 100;
+}
+
+void case_except_simple_call() NO_TAIL
+{
+ does_not_throw();
+}
+
+void case_noexcept_simple_call() noexcept(true) NO_TAIL
+{
+ does_not_throw();
+}
+
+// This tests that the destructor is called right before SEH_BeginEpilogue,
+// but in a function that has a return value.
+int case_dtor_arg_calls_no_throw(HasDtor x)
+{
+ does_not_throw(); // no NOP expected
+ return 100;
+}
+
+// Check the behavior of CALLs that are at the end of MBBs. If a CALL is within
+// a non-null EH state (state -1) and is at the end of an MBB, then we expect
+// to find an EH_LABEL after the CALL. This causes us to insert a NOP, which
+// is the desired result.
+void case_dtor_runs_after_join(int x) {
+ // CHECK-LABEL: .def "?case_dtor_runs_after_join@@YAXH@Z"
+ // CHECK: .seh_endprologue
+
+ // <-- EH state -1
+
+ // ctor call does not need a NOP, because it has real instructions after it
+ HasDtor hd{42};
+ // CHECK: call "??0HasDtor@@QEAA@H@Z"
+ // CHECK-NEXT: nop
+ // CHECK: test
+
+ // <-- EH state transition from -1 0
+ if (x) {
+ might_throw(); // <-- NOP expected (at end of BB w/ EH_LABEL)
+ // CHECK: call "?might_throw@@YAXXZ"
+ // CHECK-NEXT: nop
+ } else {
+ other_func(10); // <-- NOP expected (at end of BB w/ EH_LABEL)
+ // CHECK: call "?other_func@@YAXH@Z"
+ // CHECK-NEXT: nop
+ }
+ does_not_throw();
+ // <-- EH state transition 0 to -1
+ // ~HasDtor() runs
+
+ // CHECK: .seh_endproc
+
+ // CHECK: "$ip2state$?case_dtor_runs_after_join@@YAXH@Z":
+ // CHECK-NEXT: .long [[func_begin:.Lfunc_begin([0-9]+)@IMGREL]]
+ // CHECK-NEXT: .long -1
+ // CHECK-NEXT: .long [[tmp1:.Ltmp([0-9]+)]]@IMGREL
+ // CHECK-NEXT: .long 0
+ // CHECK-NEXT: .long [[tmp2:.Ltmp([0-9]+)]]@IMGREL
+ // CHECK-NEXT: .long -1
+}
+
+
+// Check the behavior of NOP padding around tail calls.
+// We do not expect to insert NOPs around tail calls.
+// However, the first call (to other_func()) does get a NOP
+// because it comes before .seh_startepilogue.
+void case_tail_call_no_eh() {
+ // CHECK-LABEL: .def "?case_tail_call_no_eh@@YAXXZ"
+ // CHECK: .seh_endprologue
+
+ // ordinary call
+ other_func(10);
+ // CHECK: call "?other_func@@YAXH@Z"
+ // CHECK-NEXT: nop
+
+ // tail call; no NOP padding after JMP
+ does_not_throw();
+
+ // CHECK: .seh_startepilogue
+ // CHECK: .seh_endepilogue
+ // CHECK: jmp "?does_not_throw@@YAXXZ"
+ // CHECK-NOT: nop
+ // CHECK: .seh_endproc
+}
diff --git a/clang/test/CodeGenCXX/microsoft-abi-eh-disabled.cpp b/clang/test/CodeGenCXX/microsoft-abi-eh-disabled.cpp
new file mode 100644
index 0000000000000..16fb381d814f2
--- /dev/null
+++ b/clang/test/CodeGenCXX/microsoft-abi-eh-disabled.cpp
@@ -0,0 +1,139 @@
+// RUN: %clang_cl -c --target=x86_64-windows-msvc /EHs-c- -O2 /GS- \
+// RUN: -Xclang=-import-call-optimization \
+// RUN: /clang:-S /clang:-o- %s 2>&1 \
+// RUN: | FileCheck %s
+
+#ifdef __clang__
+#define NO_TAIL __attribute((disable_tail_calls))
+#else
+#define NO_TAIL
+#endif
+
+void might_throw();
+void other_func(int x);
+
+void does_not_throw() noexcept(true);
+
+extern "C" void __declspec(dllimport) some_dll_import();
+
+class HasDtor {
+ int x;
+ char foo[40];
+
+public:
+ explicit HasDtor(int x);
+ ~HasDtor();
+};
+
+void normal_has_regions() {
+ {
+ HasDtor hd{42};
+
+ // because state changes, we expect the HasDtor::HasDtor() call to have a NOP
+ might_throw();
+ }
+
+ other_func(10);
+}
+// CHECK-LABEL: .def "?normal_has_regions@@YAXXZ"
+// CHECK: .seh_endprologue
+// CHECK: call "??0HasDtor@@QEAA@H@Z"
+// CHECK-NEXT: call "?might_throw@@YAXXZ"
+// CHECK-NEXT: mov
+// CHECK: call "??1HasDtor@@QEAA@XZ"
+// CHECK-NEXT: mov ecx, 10
+// CHECK-NEXT: call "?other_func@@YAXH@Z"
+// CHECK-NEXT: nop
+// CHECK-NEXT: .seh_startepilogue
+// CHECK-NOT: "$ip2state$?normal_has_regions@@YAXXZ"
+
+// This tests a tail call to a destructor.
+void case_dtor_arg_empty_body(HasDtor x)
+{
+}
+// CHECK-LABEL: .def "?case_dtor_arg_empty_body@@YAXVHasDtor@@@Z"
+// CHECK: jmp "??1HasDtor@@QEAA@XZ"
+
+int case_dtor_arg_empty_with_ret(HasDtor x)
+{
+ // The call to HasDtor::~HasDtor() should NOT have a NOP because the
+ // following "mov eax, 100" instruction is in the same EH state.
+ return 100;
+}
+// CHECK-LABEL: .def "?case_dtor_arg_empty_with_ret@@YAHVHasDtor@@@Z"
+// CHECK: .seh_endprologue
+// CHECK: call "??1HasDtor@@QEAA@XZ"
+// CHECK-NOT: nop
+// CHECK: mov eax, 100
+// CHECK: .seh_startepilogue
+// CHECK: .seh_endepilogue
+// CHECK: .seh_endproc
+
+void case_except_simple_call() NO_TAIL
+{
+ does_not_throw();
+}
+
+// This tests that the destructor is called right before SEH_BeginEpilogue,
+// but in a function that has a return value.
+int case_dtor_arg_calls_no_throw(HasDtor x)
+{
+ does_not_throw(); // no NOP expected
+ return 100;
+}
+
+// Check the behavior of CALLs that are at the end of MBBs. If a CALL is within
+// a non-null EH state (state -1) and is at the end of an MBB, then we expect
+// to find an EH_LABEL after the CALL. This causes us to insert a NOP, which
+// is the desired result.
+void case_dtor_runs_after_join(int x) {
+
+ // ctor call does not need a NOP, because it has real instructions after it
+ HasDtor hd{42};
+
+ if (x) {
+ might_throw();
+ } else {
+ other_func(10);
+ }
+ does_not_throw();
+ // ~HasDtor() runs
+}
+
+// CHECK-LABEL: .def "?case_dtor_runs_after_join@@YAXH@Z"
+// CHECK: .seh_endprologue
+// CHECK: call "??0HasDtor@@QEAA@H@Z"
+// CHECK-NEXT: test
+// CHECK: call "?might_throw@@YAXXZ"
+// CHECK-NEXT: jmp
+// CHECK: call "?other_func@@YAXH@Z"
+// CHECK-NEXT: .LBB
+// CHECK: call "?does_not_throw@@YAXXZ"
+// CHECK-NEXT: lea
+// CHECK-NEXT: call "??1HasDtor@@QEAA@XZ"
+// CHECK-NEXT: nop
+// CHECK-NEXT: .seh_startepilogue
+// CHECK-NOT: "$ip2state$?case_dtor_runs_after_join@@YAXH@Z":
+
+
+// Check the behavior of NOP padding around tail calls.
+// We do not expect to insert NOPs around tail calls.
+// However, the first call (to other_func()) does get a NOP
+// because it comes before .seh_startepilogue.
+void case_tail_call_no_eh() {
+ // ordinary call
+ other_func(10);
+
+ // tail call; no NOP padding after JMP
+ does_not_throw();
+}
+
+// CHECK-LABEL: .def "?case_tail_call_no_eh@@YAXXZ"
+// CHECK: .seh_endprologue
+// CHECK: call "?other_func@@YAXH@Z"
+// CHECK-NEXT: nop
+// CHECK-NEXT: .seh_startepilogue
+// CHECK: .seh_endepilogue
+// CHECK: jmp "?does_not_throw@@YAXXZ"
+// CHECK-NOT: nop
+// CHECK: .seh_endproc
diff --git a/clang/test/CodeGenCXX/microsoft-abi-eh-ip2state.cpp b/clang/test/CodeGenCXX/microsoft-abi-eh-ip2state.cpp
new file mode 100644
index 0000000000000..5e3f89c4b4680
--- /dev/null
+++ b/clang/test/CodeGenCXX/microsoft-abi-eh-ip2state.cpp
@@ -0,0 +1,241 @@
+// RUN: %clang_cl -c --target=x86_64-windows-msvc -O2 /EHsc /GS- \
+// RUN: -Xclang=-import-call-optimization \
+// RUN: /clang:-S /clang:-o- %s 2>&1 \
+// RUN: | FileCheck %s
+
+#ifdef __clang__
+#define NO_TAIL __attribute((disable_tail_calls))
+#else
+#define NO_TAIL
+#endif
+
+void might_throw();
+void other_func(int x);
+
+void does_not_throw() noexcept(true);
+
+extern "C" void __declspec(dllimport) some_dll_import();
+
+class HasDtor {
+ int x;
+ char foo[40];
+
+public:
+ explicit HasDtor(int x);
+ ~HasDtor();
+};
+
+class BadError {
+public:
+ int errorCode;
+};
+
+// Verify that when NOP padding for IP2State is active *and* Import Call
+// Optimization is active that we see both forms of NOP padding.
+void case_calls_dll_import() NO_TAIL {
+ some_dll_import();
+}
+// CHECK-LABEL: .def "?case_calls_dll_import@@YAXXZ"
+// CHECK: .seh_endprologue
+// CHECK: .Limpcall{{[0-9]+}}:
+// CHECK-NEXT: rex64
+// CHECK-NEXT: call __imp_some_dll_import
+// CHECK-NEXT: nop dword ptr {{\[.*\]}}
+// CHECK-NEXT: nop
+// CHECK-NEXT: .seh_startepilogue
+
+void normal_has_regions() {
+
+ // <-- state -1 (none)
+ {
+ HasDtor hd{42};
+
+ // <-- state goes from -1 to 0
+ // because state changes, we expect the HasDtor::HasDtor() call to have a NOP
+
+ might_throw();
+
+ // <-- state goes from 0 to -1 because we're about to call HasDtor::~HasDtor()
+ // <-- state -1
+ }
+
+ // <-- state -1
+ other_func(10);
+
+ // <-- state -1
+}
+// CHECK-LABEL: .def "?normal_has_regions@@YAXXZ"
+// CHECK: .seh_endprologue
+// CHECK: call "??0HasDtor@@QEAA@H@Z"
+// CHECK-NEXT: nop
+// CHECK: call "?might_throw@@YAXXZ"
+// CHECK-NEXT: nop
+// CHECK: call "??1HasDtor@@QEAA@XZ"
+// CHECK: call "?other_func@@YAXH@Z"
+// CHECK-NEXT: nop
+// CHECK: .seh_startepilogue
+
+// This tests a tail call to a destructor.
+void case_dtor_arg_empty_body(HasDtor x)
+{
+}
+// CHECK-LABEL: .def "?case_dtor_arg_empty_body@@YAXVHasDtor@@@Z"
+// CHECK: jmp "??1HasDtor@@QEAA@XZ"
+
+int case_dtor_arg_empty_with_ret(HasDtor x)
+{
+ // CHECK-LABEL: .def "?case_dtor_arg_empty_with_ret@@YAHVHasDtor@@@Z"
+ // CHECK: .seh_endprologue
+
+ // CHECK: call "??1HasDtor@@QEAA@XZ"
+ // CHECK-NOT: nop
+
+ // The call to HasDtor::~HasDtor() should NOT have a NOP because the
+ // following "mov eax, 100" instruction is in the same EH state.
+
+ return 100;
+
+ // CHECK: mov eax, 100
+ // CHECK: .seh_startepilogue
+ // CHECK: .seh_endepilogue
+ // CHECK: .seh_endproc
+}
+
+int case_noexcept_dtor(HasDtor x) noexcept(true)
+{
+ // CHECK: .def "?case_noexcept_dtor@@YAHVHasDtor@@@Z"
+ // CHECK: call "??1HasDtor@@QEAA@XZ"
+ // CHECK-NEXT: mov eax, 100
+ // CHECK-NEXT: .seh_startepilogue
+ return 100;
+}
+
+// Simple call of a function that can throw
+void case_except_simple_call() NO_TAIL
+{
+ might_throw();
+}
+// CHECK-LABEL: .def "?case_except_simple_call@@YAXXZ"
+// CHECK: .seh_endprologue
+// CHECK-NEXT: call "?might_throw@@YAXXZ"
+// CHECK-NEXT: nop
+// CHECK-NEXT: .seh_startepilogue
+
+// Simple call of a function that cannot throw, in a noexcept context.
+void case_noexcept_simple_call() noexcept(true) NO_TAIL
+{
+ does_not_throw();
+}
+// CHECK-LABEL: .def "?case_noexcept_simple_call@@YAXXZ"
+// CHECK: .seh_endprologue
+// CHECK-NEXT: call "?does_not_throw@@YAXXZ"
+// CHECK-NEXT: nop
+// CHECK-NEXT: .seh_startepilogue
+
+
+// This tests that the destructor is called right before SEH_BeginEpilogue,
+// but in a function that has a return value.
+int case_dtor_arg_calls_no_throw(HasDtor x)
+{
+ does_not_throw(); // no NOP expected
+ return 100;
+}
+
+// Check the behavior of CALLs that are at the end of MBBs. If a CALL is within
+// a non-null EH state (state -1) and is at the end of an MBB, then we expect
+// to find an EH_LABEL after the CALL. This causes us to insert a NOP, which
+// is the desired result.
+void case_dtor_runs_after_join(int x) {
+ // CHECK-LABEL: .def "?case_dtor_runs_after_join@@YAXH@Z"
+ // CHECK: .seh_endprologue
+
+ // <-- EH state -1
+
+ // ctor call does not need a NOP, because it has real instructions after it
+ HasDtor hd{42};
+ // CHECK: call "??0HasDtor@@QEAA@H@Z"
+ // CHECK-NEXT: test
+
+ // <-- EH state transition from -1 0
+ if (x) {
+ might_throw(); // <-- NOP expected (at end of BB w/ EH_LABEL)
+ // CHECK: call "?might_throw@@YAXXZ"
+ // CHECK-NEXT: nop
+ } else {
+ other_func(10); // <-- NOP expected (at end of BB w/ EH_LABEL)
+ // CHECK: call "?other_func@@YAXH@Z"
+ // CHECK-NEXT: nop
+ }
+ does_not_throw();
+ // <-- EH state transition 0 to -1
+ // ~HasDtor() runs
+
+ // CHECK: .seh_endproc
+
+ // CHECK: "$ip2state$?case_dtor_runs_after_join@@YAXH@Z":
+ // CHECK-NEXT: .long [[func_begin:.Lfunc_begin([0-9]+)@IMGREL]]
+ // CHECK-NEXT: .long -1
+ // CHECK-NEXT: .long [[tmp1:.Ltmp([0-9]+)]]@IMGREL
+ // CHECK-NEXT: .long 0
+ // CHECK-NEXT: .long [[tmp2:.Ltmp([0-9]+)]]@IMGREL
+ // CHECK-NEXT: .long -1
+}
+
+
+// Check the behavior of NOP padding around tail calls.
+// We do not expect to insert NOPs around tail calls.
+// However, the first call (to other_func()) does get a NOP
+// because it comes before .seh_startepilogue.
+void case_tail_call_no_eh() {
+ // CHECK-LABEL: .def "?case_tail_call_no_eh@@YAXXZ"
+ // CHECK: .seh_endprologue
+
+ // ordinary call
+ other_func(10);
+ // CHECK: call "?other_func@@YAXH@Z"
+ // CHECK-NEXT: nop
+
+ // tail call; no NOP padding after JMP
+ does_not_throw();
+
+ // CHECK: .seh_startepilogue
+ // CHECK: .seh_endepilogue
+ // CHECK: jmp "?does_not_throw@@YAXXZ"
+ // CHECK-NOT: nop
+ // CHECK: .seh_endproc
+}
+
+
+// Check the behavior of a try/catch
+int case_try_catch() {
+ // CHECK-LABEL: .def "?case_try_catch@@YAHXZ"
+ // CHECK: .seh_endprologue
+
+ // Because of the EH_LABELs, the ctor and other_func() get NOPs.
+
+ int result = 0;
+ try {
+ // CHECK: call "??0HasDtor@@QEAA@H@Z"
+ // CHECK-NEXT: nop
+ HasDtor hd{20};
+
+ // CHECK: call "?other_func@@YAXH@Z"
+ // CHECK-NEXT: nop
+ other_func(10);
+
+ // CHECK: call "??1HasDtor@@QEAA@XZ"
+ // CHECK: mov
+ } catch (BadError& e) {
+ result = 1;
+ }
+ return result;
+
+ // CHECK: .seh_endproc
+
+ // CHECK: .def "?dtor$4@?0??case_try_catch@@YAHXZ@4HA"
+ // CHECK: .seh_endprologue
+ // CHECK: call "??1HasDtor@@QEAA@XZ"
+ // CHECK-NEXT: nop
+ // CHECK: .seh_startepilogue
+ // CHECK: .seh_endproc
+}
diff --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
index e13e92378d4aa..6545b8b6404e0 100644
--- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
@@ -1858,6 +1858,7 @@ void AsmPrinter::emitFunctionBody() {
OutStreamer->emitLabel(MI.getOperand(0).getMCSymbol());
break;
case TargetOpcode::EH_LABEL:
+ OutStreamer->AddComment("EH_LABEL");
OutStreamer->emitLabel(MI.getOperand(0).getMCSymbol());
// For AsynchEH, insert a Nop if followed by a trap inst
// Or the exception won't be caught.
@@ -1932,8 +1933,9 @@ void AsmPrinter::emitFunctionBody() {
auto CountInstruction = [&](const MachineInstr &MI) {
// Skip Meta instructions inside bundles.
- if (MI.isMetaInstruction())
+ if (MI.isMetaInstruction()) {
return;
+ }
++NumInstsInFunction;
if (CanDoExtraAnalysis) {
StringRef Name = getMIMnemonic(MI, *OutStreamer);
diff --git a/llvm/lib/CodeGen/AsmPrinter/WinException.cpp b/llvm/lib/CodeGen/AsmPrinter/WinException.cpp
index 55d1350e446ab..258349c241369 100644
--- a/llvm/lib/CodeGen/AsmPrinter/WinException.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/WinException.cpp
@@ -325,12 +325,6 @@ const MCExpr *WinException::getLabel(const MCSymbol *Label) {
Asm->OutContext);
}
-const MCExpr *WinException::getLabelPlusOne(const MCSymbol *Label) {
- return MCBinaryExpr::createAdd(getLabel(Label),
- MCConstantExpr::create(1, Asm->OutContext),
- Asm->OutContext);
-}
-
const MCExpr *WinException::getOffset(const MCSymbol *OffsetOf,
const MCSymbol *OffsetFrom) {
return MCBinaryExpr::createSub(
@@ -657,7 +651,7 @@ void WinException::emitSEHActionsForRange(const WinEHFuncInfo &FuncInfo,
AddComment("LabelStart");
OS.emitValue(getLabel(BeginLabel), 4);
AddComment("LabelEnd");
- OS.emitValue(getLabelPlusOne(EndLabel), 4);
+ OS.emitValue(getLabel(EndLabel), 4);
AddComment(UME.IsFinally ? "FinallyFunclet" : UME.Filter ? "FilterFunction"
: "CatchAll");
OS.emitValue(FilterOrFinally, 4);
@@ -952,13 +946,7 @@ void WinException::computeIP2StateTable(
if (!ChangeLabel)
ChangeLabel = StateChange.PreviousEndLabel;
// Emit an entry indicating that PCs after 'Label' have this EH state.
- // NOTE: On ARM architectures, the StateFromIp automatically takes into
- // account that the return address is after the call instruction (whose EH
- // state we should be using), but on other platforms we need to +1 to the
- // label so that we are using the correct EH state.
- const MCExpr *LabelExpression = (isAArch64 || isThumb)
- ? getLabel(ChangeLabel)
- : getLabelPlusOne(ChangeLabel);
+ const MCExpr *LabelExpression = getLabel(ChangeLabel);
IPToStateTable.push_back(
std::make_pair(LabelExpression, StateChange.NewState));
// FIXME: assert that NewState is between CatchLow and CatchHigh.
diff --git a/llvm/lib/CodeGen/AsmPrinter/WinException.h b/llvm/lib/CodeGen/AsmPrinter/WinException.h
index 638589adf0ddc..47dd30cef133d 100644
--- a/llvm/lib/CodeGen/AsmPrinter/WinException.h
+++ b/llvm/lib/CodeGen/AsmPrinter/WinException.h
@@ -80,7 +80,6 @@ class LLVM_LIBRARY_VISIBILITY WinException : public EHStreamer {
const MCExpr *create32bitRef(const MCSymbol *Value);
const MCExpr *create32bitRef(const GlobalValue *GV);
const MCExpr *getLabel(const MCSymbol *Label);
- const MCExpr *getLabelPlusOne(const MCSymbol *Label);
const MCExpr *getOffset(const MCSymbol *OffsetOf, const MCSymbol *OffsetFrom);
const MCExpr *getOffsetPlusOne(const MCSymbol *OffsetOf,
const MCSymbol *OffsetFrom);
diff --git a/llvm/lib/Target/X86/X86AsmPrinter.h b/llvm/lib/Target/X86/X86AsmPrinter.h
index efb951b73532f..6c04f8729f1ff 100644
--- a/llvm/lib/Target/X86/X86AsmPrinter.h
+++ b/llvm/lib/Target/X86/X86AsmPrinter.h
@...
[truncated]
|
|
Updated most of the failing integration tests. There are 2 that merit investigation, which I'm doing now. |
|
Investigated remaining tests and fixed. |
llvm/test/CodeGen/X86/win64_frame.ll
Outdated
| @@ -101,7 +101,8 @@ define void @f5() "frame-pointer"="all" { | |||
| ; CHECK-NEXT: .seh_endprologue | |||
| ; CHECK-NEXT: leaq -92(%rbp), %rcx | |||
| ; CHECK-NEXT: callq external | |||
| ; CHECK-NEXT: nop | |||
| ; UEFI does not have SEH, so does not require NOP | |||
| ; WIN64-NEXT: nop | |||
There was a problem hiding this comment.
For files with a "Assertions have been autogenerated by utils/update_llc_test_checks.py" comment, please make sure you update them using update_llc_test_checks.py . Otherwise the next person to modify the file will be confused trying to figure out what you did.
There was a problem hiding this comment.
Happy to do that -- I'm just not familiar with updating those kind of tests yet. In this case, this test is run 4 times, with different parameters. Will update_llc_test_checks.py know how to do the right thing? The NOP will only be present in 2 out of the 4 tests. What should the ideal patch look like, here?
There was a problem hiding this comment.
update_llc_test_checks.py understands the --check-prefixes flag; it will try to do something reasonable.
There was a problem hiding this comment.
This looks hand-edited; please regenerate with update_llc_test_checks.
| // If there is an EH_LABEL after this CALL, then there is an EH state | ||
| // transition after this CALL. This is exactly the situation which requires | ||
| // NOP padding. | ||
| if (NextMI.isEHLabel()) { |
There was a problem hiding this comment.
As far as I can tell, this triggers on every "invoke"... so if you write try { f(); f(); f(); } catch (...) {}, you get a nop after every call. Not sure how complicated it would be to handle that.
There was a problem hiding this comment.
Could (or should) that be handled by more precise generation of the EH_LABEL pseudo-instructions?
What's really wanted is "what EH state index is this instruction in?", but I could not find a reasonable way to do that without modifying an excessive number of things. Keying on the EH_LABEL pseudo-instructions seemed the most economical, and in my experiments did not result in a significant increase in binary size.
There was a problem hiding this comment.
At the point where we generate the EH_LABEL instructions, the block layout isn't finalized. Actually, nothing computes whether there's a state change until computeIP2StateTable() itself. So maybe the optimization is a little tricky to implement.
| // there are no AMD64 instructions that can be encoded in a single byte and | ||
| // which throw C++ exceptions. Unfortunately, this violates the Windows ABI | ||
| // specification, which requires that the IP2State table entries point to the | ||
| // boundaries between exceptions. |
There was a problem hiding this comment.
Does "violating the ABI specification" have any practical side-effects?
There was a problem hiding this comment.
It prevents our analysis tools, such as our hot-patch generator, from correctly analyzing binaries.
There was a problem hiding this comment.
@efriedma-quic this PR supposedly also fixes #54922 which is the same but for SEH. That can and does happen with single byte instructions, as shown in the issue OP.
| @@ -9,7 +9,6 @@ define i32 @foobar() gc "statepoint-example" personality ptr @__gxx_personality_ | |||
| ; CHECK-NEXT: .seh_endprologue | |||
| ; CHECK-NEXT: callq bar | |||
| ; CHECK-NEXT: .Ltmp0: | |||
| ; CHECK-NEXT: nop | |||
There was a problem hiding this comment.
This makes the call adjacent to the epilog; is that okay?
There was a problem hiding this comment.
Since this is for the Windows GNU target, which does not use SEH, I believe so, but I would really like to hear from someone with experience with the Windows GNU target.
There was a problem hiding this comment.
Is the codegen change here specific to Windows-GNU targets? I don't see any reason MSVC targets would behave differently.
There was a problem hiding this comment.
The Windows GNU target does use SEH for the unwind info, but uses GCC compatible unwind info for the language level things (corresponding to ip2state tables I guess).
There was a problem hiding this comment.
So the conclusion here is this particular case doesn't need the nop? Fine, I guess.
I suspect there's an underlying issue with the algorithm, though. Consider the following C++ function:
[[noreturn]] void g1();
void g2();
struct A { ~A(); };
void f(bool b) {
if (b == false)
g1();
A a;
g2();
}Compile with:
clang -O2 --target=x86_64-windows-msvc -mllvm -opt-bisect-limit=127
Produces:
pushq %rbp
.seh_pushreg %rbp
subq $48, %rsp
.seh_stackalloc 48
leaq 48(%rsp), %rbp
.seh_setframe %rbp, 48
.seh_endprologue
movq $-2, -8(%rbp)
testb %cl, %cl
jne .LBB0_2
jmp .LBB0_1
.LBB0_1: # %if.then
callq "?g1@@YAXXZ"
.LBB0_2: # %if.end
.Ltmp0: # EH_LABEL
callq "?g2@@YAXXZ"
nop
[...]
You end up in the wrong region if g1 unwinds, I think.
(Not sure off the top of my head how to reproduce this without opt-bisect-limit, but I'm sure there's some way.)
There was a problem hiding this comment.
Looking a bit more, if you drop the [[noreturn]] and the opt-bisect-limit flag, you get the same result.
There was a problem hiding this comment.
I've reverted this change, because now I'm handling inserting the NOP (before the SEH epilogue, not for all CALLs) when using GNU EH on Win32.
I'm a little defensive, since I think I came up with the label+1 scheme, but I don't believe there are any documented requirements covering the ip2state table. I do not believe I have ever seen a description of the rules for ip2state tables anywhere in any Microsoft docs. They appear to be implementation details of MSVC EH personality functions, and the requirements are simply that "exception handling works", and relevant EH test suites pass. I'm very willing to accept that this is a requirement, but I would prefer to change the language in the commit message and comments to speak more about the functional reasons why we should increase code size to get instruction-aligned label offsets, or have references to real, existing, public specifications, instead of references to an undocumented, notional "Windows ABI specification". IIRC users of IDA Pro have complained, for example, since it attempts to reconstruct EH blocks using these tables, and that's a perfectly good reason to merge a patch to implement this. I think this can be found in the issue tracker if someone looks closely.
I guess text relocations are cool again. :) |
|
@mk No offense intended, of course. I'm happy to change the commit text / comments. What would you suggest? The Windows ABI is vast enough that a lot of it is poorly-documented, or has deeper invariants that are not documented. The requirement that IP2State boundaries align with instruction boundaries is certainly not emphasized enough publicly, and it just so happens to work with the current version of the unwinder. But there are more tools in the Windows ecosystem than just the NTDLL / RTL unwinder, so this work is intended to align LLVM's codegen with MSVC's, so that LLVM can work better with those other tools. My current focus is on enabling Clang and Rust to work with Microsoft's hot-patching infrastructure. (See #138972 for another PR that is in progress, toward that goal.) This form of hot-patching relies on analysis tools that also read IP2State tables and disassemble instruction streams, and these rely on the entries being correctly aligned to instruction boundaries. It's entirely reasonable to point out that none of this was public, and that that the unwinder worked, so why do this now? Well, again, my goal is to align LLVM with these requirements, and to help make these requirements clear to the public, and to make clear the motivation for doing so. Enabling hot-patching for Rust + Clang on Windows is one of the benefits, but it will also help to enable other Microsoft tools which rely on MSVC's adherence to the (poorly-documented, but real) requirement that IP2State tables be aligned to instruction boundaries. Part of the problem is that MSVC's implementation has been the de facto standard for Windows' ABI. We're trying to change that, by making some of the requirements of the Windows ABI more clear and more explicit. For example, my teammate @dpaoliello has done a fair amount of work on ARM64EC support in LLVM, both in clarifying the ABI and in providing implementation. |
|
While I haven't checked how exactly this PR interacts with SEH, just here because #54922 (comment) , I'd like to point out that https://clang.llvm.org/docs/MSVCCompatibility.html has the following snippet for SEH:
If this patch will remove the +1 offset from the beginning of the call instruction as well, then if that throws it will be caught in the same frame, which is "impossible" (not sure what the practical implications of this are). Call can throw when calling indirect from invalid memory. As it stands, this patch breaks this "functionality", as now these exceptions will be caught in the inner While MSVC never worked this way, this would not fix the whole SEH discrepancy, while effectively "removing" a feature. |
|
Actually, disregard my previous comment. It seems that it's quite a bit more broken than that already and depends on surrounding code. Making it broken in different ways probably won't hurt much. |
|
My team is interested in fixing / completing the SEH support in LLVM, as well. I think this PR is consistent with that, since it aligns LLVM's behavior with that of MSVC. For an SEH exception that occurs within a function due to a CALL (such as a bogus indirect call), the IP should point to the beginning of the CALL instruction, so the IP2State tables should point to the correct region. @namazso , I'd be happy to improve the state of things for SEH, as well as ensuring that this PR doesn't regress support for GNU's EH support on Windows. If GNU's C++ EH support is based on SEH, then I think I will need to change this PR so that a CALL aligned to |
Yes, I was just pointing out that same-frame void test1(void(*a)()) {
__try {
a();
} __except(1) {
printf("throw in call!\n");
}
}
void test2(void(*a)()) {
__try {
test1(a);
} __except (1) {
printf("throw from call!\n");
}
}
void int3() {
__debugbreak();
}
int main() {
test2(&int3);
test2((void(*)())0x8FFFFFFFFFFFFFFF);
return 0;
}Should result in this: Which indeed would probably work like that if the start labels were also offset by 1 all the time. Of course it's way different from MSVC, but it sounded at least consistent even if slightly insane. However when I tested it on the current LLVM release, it was not consistent at all already, so that's why I commented to just disregard this observation - it already didn't conform whatever the docs were saying, it's not going to be any more broken that it currently is.
Sorry, no clue on GNU EH, my observations were purely about the current state of SEH and the docs (incorrectly) describing the "rules" on how exactly it is partially supported. |
|
@rnk - Aside from the comments / text edits, do you have any other suggestions for this PR? |
sivadeilra
force-pushed
the
user/ardavis/upstream-ip2state
branch
from
89f97ae to
d573572
Compare
sivadeilra
force-pushed
the
user/ardavis/upstream-ip2state
branch
2 times, most recently
from
a9ea224 to
541863f
Compare
|
Sorry for the delay, I'm at a conference this week. Regarding wording, I would prefer to rephrase these fragments more precisely in the final commit message: "The Windows ABI requires ..." "... the Windows ABI specification, ..." The Windows ABI is a nebulous concept, and aligning LLVM codegen with MSVC is something we can only chase so far. At some point, we have to tradeoff bug-for-bug MSVC compatibility with other goals like implementation complexity and code size, and it's helpful to motivate the change with more concrete goals. In this case, LLVM has grown support for non-call exceptions, so the assumptions this +1 byte scheme no longer hold, and that, to me, is a very compelling reason to do this nop insertion. IDA pro compat seems like a good supporting reason, and then it seems there is a long tail of internal tools that need to parse these ip2state tools. Hopefully that answers your questions. I expect to be busy over the next month, so please proceed with the review without blocking on my input, I do support this work. :) |
|
@rnk - Thank you, and thanks for your time. I'll work on edits to the text. |
sivadeilra
force-pushed
the
user/ardavis/upstream-ip2state
branch
from
541863f to
205ef34
Compare
|
I've updated the comments and the PR description so that the tone is more neutral, and describes the state of LLVM after the PR, with less emphasis on LLVM's old behavior. |
sivadeilra
force-pushed
the
user/ardavis/upstream-ip2state
branch
from
6be09e3 to
e7ccf87
Compare
I think that's an appropriately conservative place to start, anyway. The uefi-* targets are still nascent and heretofore the Windows targets have been used for producing UEFI binaries. So the approach we've taken is that for all miscellaneous code generation issues, we should start with the UEFI target matching the behavior of the corresponding Windows target exactly. That seems like the right thing to do for now. In the future as the uefi-* targets mature, we can reconsider things that are really only relevant to Windows ABIs and not to the UEFI ABI or the PE-COFF format. The Windows EH ABI is not an aspect of the UEFI ABI, but the support for Windows EH does not interfere with UEFI so I think it's harmless enough to emit all the same stuff. |
There was a problem hiding this comment.
LGTM
The following might warrant a followup at some point:
- generating int3 instead of nop after "always-throw" calls, like stack-coloring-wineh.ll
- elide nops between two calls if they have the same catch handler.
- elide the nop between a call and the epilogue if the call is in a "null" region, i.e. the exception won't be handled. (Not 100% sure about this, but it seems like MSVC does it; https://godbolt.org/z/3fs97Pe4s .)
Please file issues for those so we have a record of that. But I'm happy to land this without those optimizations.
sivadeilra
force-pushed
the
user/ardavis/upstream-ip2state
branch
2 times, most recently
from
2cbe1c0 to
deb477a
Compare
style: revert one change Adjust tests adjust another test fix llvm/test/CodeGen/XCore/exception.ll PR feedback PR feedback update microsoft-abi-eh-async.cpp for tighter NOP handling PR feedback: use INT3 instead of NOP for noreturn update win64_frame.ll Fix bug, plus more accurate NOP optimization Fix win64-seh-epilogue-statepoint.ll
sivadeilra
force-pushed
the
user/ardavis/upstream-ip2state
branch
from
deb477a to
3dec74c
Compare
|
@sivadeilra Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/65/builds/19986 Here is the relevant piece of the build log for the reference |
|
Created issue #150057 for follow-up improvements. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/154/builds/19094 Here is the relevant piece of the build log for the reference |
|
This is causing failures on my macOS machine that uses paths starting with "/Users": |
|
Looking at this now, along with a test failure on AArch64. |
These test failures also appeared on my macOS bot: https://lab.llvm.org/buildbot/#/builders/190/builds/23983 |
|
@jansvoboda11 Can you verify that this change: (inserting |
|
@sivadeilra the tests are still failing at least on my MacOS bot after the fix in 9a7a6b2. Can you take a look? https://lab.llvm.org/buildbot/#/builders/190/builds/23988 |
|
We are still seeing the above failures downstream on Linux, even with the fix in 9a7a6b2 |
|
I had a fix PR going through CI jobs, but 9a7a6b2 was committed before my job finished. It added |
|
#150145 in CI now |
The 2 tests are still failing: https://lab.llvm.org/buildbot/#/builders/190/builds/24008 Any other ideas or should we just revert everything while you try to figure it out? |
Added by #144745. These tests cause Clang -cc1 to generate the option -x86-asm-syntax=intel, which is only available if you have included the x86 target. <<<<<< 1: clang: warning: argument unused during compilation: '-c' [-Wunused-command-line-argument] label:38'0 X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found label:38'1 ? possible intended match 2: clang (LLVM option parsing): Unknown command line argument '-x86-asm-syntax=intel'. Try: 'clang (LLVM option parsing) --help' label:38'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 3: clang (LLVM option parsing): Did you mean '--asan-stack=intel'? label:38'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>>>>
It requires the x86 target, but not in an obvious way, clang's cc1 generates an option that requires the target. Rather than what you see in the RUN line requiring it. Should be fixed now. |
Added by llvm/llvm-project#144745. These tests cause Clang -cc1 to generate the option -x86-asm-syntax=intel, which is only available if you have included the x86 target. <<<<<< 1: clang: warning: argument unused during compilation: '-c' [-Wunused-command-line-argument] label:38'0 X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found label:38'1 ? possible intended match 2: clang (LLVM option parsing): Unknown command line argument '-x86-asm-syntax=intel'. Try: 'clang (LLVM option parsing) --help' label:38'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 3: clang (LLVM option parsing): Did you mean '--asan-stack=intel'? label:38'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>>>>
|
@DavidSpickett Thank you, David! |
This changes how LLVM constructs certain data structures that relate to exception handling (EH) on Windows. Specifically this changes how IP2State tables for functions are constructed. The purpose of this change is to align LLVM to the requires of the Windows AMD64 ABI, which requires that the IP2State table entries point to the boundaries between instructions.
On most Windows platforms (AMD64, ARM64, ARM32, IA64, but not x86-32), exception handling works by looking up instruction pointers in lookup tables. These lookup tables are stored in
.xdatasections in executables. One element of the lookup tables are theIP2Statetables (Instruction Pointer to State).If a function has any instructions that require cleanup during exception unwinding, then it will have an IP2State table. Each entry in the IP2State table describes a range of bytes in the function's instruction stream, and associates an "EH state number" with that range of instructions. A value of -1 means "the null state", which does not require any code to execute. A value other than -1 is an index into the State table.
The entries in the IP2State table contain byte offsets within the instruction stream of the function. The Windows ABI requires that these offsets are aligned to instruction boundaries; they are not permitted to point to a byte that is not the first byte of an instruction.
Unfortunately, CALL instructions present a problem during unwinding. CALL instructions push the address of the instruction after the CALL instruction, so that execution can resume after the CALL. If the CALL is the last instruction within an IP2State region, then the return address (on the stack) points to the next IP2State region. This means that the unwinder will use the wrong cleanup funclet during unwinding.
To fix this problem, compilers should insert a NOP after a CALL instruction, if the CALL instruction is the last instruction within an IP2State region. The NOP is placed within the same IP2State region as the CALL, so that the return address points to the NOP and the unwinder will locate the correct region.
This PR modifies LLVM so that it inserts NOP instructions after CALL instructions, when needed. In performance tests, the NOP has no detectable significance. The NOP is rarely inserted, since it is only inserted when the CALL is the last instruction before an IP2State transition or the CALL is the last instruction before the function epilogue.
NOP padding is only necessary on Windows AMD64 targets. On ARM64 and ARM32, instructions have a fixed size so the unwinder knows how to "back up" by one instruction.
Interaction with Import Call Optimization (ICO):
Import Call Optimization (ICO) is a compiler + OS feature on Windows which improves the performance and security of DLL imports. ICO relies on using a specific CALL idiom that can be replaced by the OS DLL loader. This removes a load and indirect CALL and replaces it with a single direct CALL.
To achieve this, ICO also inserts NOPs after the CALL instruction. If the end of the CALL is aligned with an EH state transition, we also insert a single-byte NOP. Both forms of NOPs must be preserved. They cannot be combined into a single larger NOP; nor can the second NOP be removed.
This is necessary because, if ICO is active and the call site is modified by the loader, the loader will end up overwriting the NOPs that were inserted for ICO. That means that those NOPs cannot be used for the correct termination of the exception handling region (the IP2State transition), so we still need an additional NOP instruction. The NOPs cannot be combined into a longer NOP (which is ordinarily desirable) because then ICO would split one instruction, producing a malformed instruction after the ICO call.