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

[Fangrui Song via Phabricator via libcxx-commits]

MaskRay added a comment.

Mostly looks good.



================
Comment at: libunwind/CMakeLists.txt:185
+if (LIBUNWIND_ENABLE_CET)
+  add_compile_flags_if_supported(-fcf-protection=full)
+  add_compile_flags_if_supported(-mshstk)
----------------
add_compile_flags.

When LIBUNWIND_ENABLE_CET=on, it is an error if the compiler does not support the options.


================
Comment at: libunwind/src/UnwindLevel1.c:173
 
+  // The first stack frame walked is unwind_phase2.
+  unsigned framesWalked = 1;
----------------
`git diff -U0 --no-color 'HEAD^' | clang/tools/clang-format/clang-format-diff.py -i -p1` reformat as needed.


================
Comment at: libunwind/src/UnwindLevel1.c:55
+    __asm__ volatile("push %%edi\n\t"                                          \
+                     "sub $4, %%esp\n\t"                                       \
+                     "jmp *%%edx\n\t" ::"D"(cetRegContext),                    \
----------------
xiongji90 wrote:
> MaskRay wrote:
> > Is the calling convention here wrong?
> > 
> > 
> Hi, @MaskRay 
> The code here is to emulate x86 calling convention. As we know, x86 uses stack to pass input parameter and store return address.
> We will jump to "__libunwind_Registers_x86_jumpto" instead of calling it to avoid any change to CET shadow stack.
> In libunwind, "__libunwind_Registers_x86_jumpto" is a pure x86 assembly function, the implementation expects the stack frame to be a standard x86 function call stack frame like following:
> 
> +______________+
> + input param   +
> +______________+
> + return addr    +
> +___________ __+
> .....
> __libunwind_Registers_x86_jumpto will never return, so the return address stored in stack will not be used. However, the implementation does rely on the stack layout, following code is from __libunwind_Registers_x86_jumpto:
> 
> DEFINE_LIBUNWIND_FUNCTION(__libunwind_Registers_x86_jumpto)
> #
> # extern "C" void __libunwind_Registers_x86_jumpto(Registers_x86 *);
> #
> # On entry:
> #  +                       +
> #  +-----------------------+
> #  + thread_state pointer  +
> #  +-----------------------+
> #  + return address        +
> #  +-----------------------+   <-- SP
> #  +                       +
>   _LIBUNWIND_CET_ENDBR
>   movl   4(%esp), %eax
>   # set up eax and ret on new stack location
>   movl  28(%eax), %edx # edx holds new stack pointer
>   subl  $8,%edx
>   movl  %edx, 28(%eax)
>   movl  0(%eax), %ebx
> ......
> As we can see, we must ensure the input parameter is placed in 4(%esp), 0(%esp) stores dummy return address which will never be used.
> So, after pushing input parameter for __libunwind_Registers_x86_jumpto into stack, we have to "sub $4, %%esp" in order to emulate pushing return address into stack, the pushing is aimed to make stack layout to meet __libunwind_Registers_x86_jumpto's requirement.
> Thanks very much.
`sub $4, %%esp` is for the dummay return address. Haven't used x86-32 assembly for a while and forgot this. Sorry!

You may just write `asm` instead of `__asm__`. Other places use `asm` directly as well.


================
Comment at: libunwind/src/cet_unwind.h:17
+
+// Currently, CET is implemented in x86/x86_64 Linux platform.
+#if defined(_LIBUNWIND_TARGET_LINUX) && defined(__CET__) && defined(__SHSTK__)
----------------
on Linux x86 platforms.


================
Comment at: libunwind/test/lit.site.cfg.in:30
 config.cxx_ext_threads          = @LIBUNWIND_BUILD_EXTERNAL_THREAD_LIBRARY@
+config.cet_on                   = @LIBUNWIND_ENABLE_CET@
 
----------------
perhaps `config.x86_cet`


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

https://reviews.llvm.org/D105968