[PATCH] D105968: [libunwind][CET] Support exception handling stack unwind in CET environment

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd added inline comments.


================
Comment at: libunwind/CMakeLists.txt:188
+  add_compile_flags_if_supported(-fcf-protection=full)
+  add_compile_flags_if_supported(-mshstk)
+endif()
----------------
xiongji90 wrote:
> compnerd wrote:
> > I think that we want to test and set:
> > - `/Gy`
> > - `/guard:ehcont`
> > - `/guard:cf`
> > - `/cetcompat`
> > 
> > Or explicitly error with MSVC.
> Hi, @compnerd 
> I suggest to report error with MSVC currently since CET enabling for Windows is still ongoing, once MS claims the enabling work is done, we can support MSVC.
> Thanks very much.
I'm not sure about the statement (I believe that the CET support is available today), but I would be okay with the error stating that libunwind doesn't support CET on Windows yet.  I'm more concerned that someone who builds the library may be under the false impression that CET is enabled when it is not.


================
Comment at: libunwind/src/Registers.hpp:46
 extern "C" void __libunwind_Registers_x86_jumpto(Registers_x86 *);
+#if defined(_LIBUNWIND_CET_ENABLED)
+extern "C" void *__libunwind_get_cet_jump_target() {
----------------
A newline before this line like in the x86_64 case would be nice.


================
Comment at: libunwind/src/UnwindLevel1.c:47
+#if !defined(_LIBUNWIND_CET_ENABLED)
+#define __unw_phase2_resume(cursor, fn) __unw_resume((cursor))
+#elif _LIBUNWIND_CET_ENABLED == 1
----------------
xiongji90 wrote:
> compnerd wrote:
> > Why does this have to be a macro?  Can we just define the function instead?
> Hi, @compnerd 
> In previous version, I defined a function the code looks like following:
> void __unw_phase2_resume(cursor, fn) {
> __LIBUNWIND_POP_CET_SSP(...);
> ....
> ....
> __asm__ volatile(....)
> }
> The stack layout looks like following if compiler doesn't do any optimization to __unw_phase2_resume:
> _____________________
> .....user functions
> ______________________
> __cxx_throw
> _______________________
> _Unwind_RaiseException
> _______________________
> unwind_phase2
> _______________________
> __unw_phase2_resume
> ________________________
> 
> Under such circumstances, the stack frame for __unw_phase2_resume will be skipped too and we must adjust CET shadow stack for it.
> However, as we can see, the __unw_phase2_resume will never return. I am afraid compiler may directly jump to this function instead of "call" it. If so, there will be no stack frame for __unw_phase2_resume and if compiler uses "jmp" to jump to it instead of calling it, we mustn't adjust CET shadow stack for it.
> In our source code, we can't know whether compiler will do such optimization and we can't decide whether we should adjust CET shadow stack for __unw_phase2_resume. So I used macro here to avoid this problem.
> I used inline asm to jump to __libunwind_Registers_x86_64_jumpto for the same purpose.
> Thanks very much.
I wonder if we can get away with some combination of `noinline`, and pragmas for `no-optimize-sibling-calls`.


================
Comment at: libunwind/src/libunwind_ext.h:37
+#if defined(_LIBUNWIND_CET_ENABLED)
+extern void *__unw_cet_get_registers(unw_cursor_t *);
+#endif
----------------
xiongji90 wrote:
> compnerd wrote:
> > Why is this new interface needed?  Who is the expected user?
> Hi, @compnerd 
> Currently, Registers are encapsulated in UnwindCursor class and __libunwind_Registers_x86_jumpto/__libunwind_Registers_x86_64_jumpto need Registers as input parameter. We need to prepare the parameter and then jump to __libunwind_Registers_x86/_64_jumpto function, so we need to get internal Registers from a cursor. This function is added for this purpose and we don't expect other users to use it.
> Do you think we should define it as an internal function instead of a function with "__unw" prefix?
> Thanks very much.
Yes please move this to an internal function.  This header is exported to users, and this is why I was asking specifically about this function.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105968/new/

https://reviews.llvm.org/D105968