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

[xiongji90 via Phabricator via llvm-commits]

xiongji90 added inline comments.


================
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:
> > > 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.
> GCC uses an undocumented intrinsic, __builtin_eh_return, to avoid such issue.
Hi, @hjl.tools 
The undocumented GCC intrinsic __builtin_eh_return are used by GCC unwinder and its semantic are strongly related to GCC unwind's implementation.
For example, GCC's unwinder will install target context and then get target landing pad address, after that GCC unwinder will use __builtin_eh_return to adjust the SP value and jump to landing pad, __builtin_eh_return assumes that target context has been prepared.
In our implementation, we need to adjust CET SSP and then jump to __libunwind_Registers_X86/_64_jumpto(asm code), this asm function will prepare target context (restore registers), adjust SP and jump to final landing pad. Our requirement here is just directly jump to __libunwind_Registers_X86/_64_jumpto function to avoid pushing CET shadow stack again.  So, I think __builtin_eh_return can't help much in libunwind.

Thanks very much.


================
Comment at: libunwind/src/assembly.h:18
 
+#if (defined(__i386__) || defined(__x86_64__)) && defined(__linux__)
+#include <cet.h>
----------------
hjl.tools wrote:
> xiongji90 wrote:
> > 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.
> If MinGW and cywin support CET, libunwind should support MinGW and cywin.
Hi, @hjl.tools and @compnerd 
I didn't find any info of CET support for MinGW and cywin after some google search, so I suggest we can ignore MinGW and cywin currently.

Thanks very much.


================
Comment at: libunwind/src/libunwind_ext.h:37
+#if defined(_LIBUNWIND_CET_ENABLED)
+extern void *__unw_cet_get_registers(unw_cursor_t *);
+#endif
----------------
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.


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

https://reviews.llvm.org/D105968