[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/CMakeLists.txt:188
+  add_compile_flags_if_supported(-fcf-protection=full)
+  add_compile_flags_if_supported(-mshstk)
+endif()
----------------
compnerd wrote:
> 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.
Hi, @compnerd 
I will update the patch to report error for MSVC + CET. All components including OS, toolchain, system libraries need to enable CET so that CET feature can be fully enabled in a platform. I know MS is working on CET but I am not sure whether they have finished the work.
Hi, @hjl.tools  
Could you please correct me if anything is wrong about CET enabling status for Windows and MSVC.
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:
> > > 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.
> One feature of __builtin_eh_return is to perform the special tail call.   Can clang provide a way to do that?
Hi, @hjl.tools 
I think clang is compatible with GCC for __builtin_eh_return and could you share some sample code or info about this special feature?
If it can work, we can get rid of the inline asm.
Thanks very much.


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

https://reviews.llvm.org/D105968