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

[xiongji90 via Phabricator via libcxx-commits]

xiongji90 added inline comments.


================
Comment at: libunwind/CMakeLists.txt:188
+  add_compile_flags_if_supported(-fcf-protection=full)
+  add_compile_flags_if_supported(-mshstk)
+endif()
----------------
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.


================
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
----------------
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.


================
Comment at: libunwind/src/UnwindLevel1.c:42
+    _LIBUNWIND_POP_CET_SSP(1);                                                 \
+    __libunwind_cet_jumpto((cursor), (frames_skipped));                        \
+  } while (0)
----------------
hjl.tools wrote:
> xiongji90 wrote:
> > hjl.tools wrote:
> > > Why is this needed?  Can you unwind the shadow stack and jump to the landing pad as usual?
> > Hi, @hjl.tools 
> > I have updated the patch to remove some unnecessary code, could you help review again?
> > Thanks very much.
> __unw_phase2_resume looks quite fragile.   Can you call the function pointer,  cetJumpAddress, in C directly?
Hi, @hjl.tools 
I am afraid calling a function pointer in C is risky. The cetJumpAddress is address of __libunwind_Registers_x86/x86_64_jumpto and these functions will never return, if we call this function or call the function pointer, compiler codegen may generate following code:
"
mov __libunwind_Registers_x86_64_jumpto, %rax
call *%rax 
"
Under such circumstance, we must adjust CET SSP by 1 in __libunwind_Registers_x86_64_jumpto since the call instruction will push CET shadow stack.

However, if compiler codegen use "jmp" instead of "call", for example:
"
jmp  xxxxx (the addr of __libunwind_Registers_x86_64_jumpto)
"
we shouldn't adjust CET shadow stack. It is difficult for us to decide whether or not should we adjust CET shadow stack since we can't know what code compiler will generate for the function/function pointer call in C source level.

Thanks very much.


================
Comment at: libunwind/src/assembly.h:18
 
+#if (defined(__i386__) || defined(__x86_64__)) && defined(__linux__)
+#include <cet.h>
----------------
compnerd wrote:
> Do you need to consider MinGW or cywin?
Hi, @compnerd 
I suggest to ignore MinGW and cywin currently.
Hi, @hjl.tools 
Do you think we can ignore MinGW and cywin?

Thanks very much.


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

https://reviews.llvm.org/D105968