[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:185
+if (LIBUNWIND_ENABLE_CET)
+  add_compile_flags_if_supported(-fcf-protection=full)
+  add_compile_flags_if_supported(-mshstk)
----------------
MaskRay wrote:
> add_compile_flags.
> 
> When LIBUNWIND_ENABLE_CET=on, it is an error if the compiler does not support the options.
Hi, @MaskRay 
I update the patch to check "LIBUNWIND_SUPPORTS_FCF_PROTECTION_EQ_FULL_FLAG" and "LIBUNWIND_SUPPORTS_MSHSTK_FLAG", if either flag is not set, error will be reported.
Thanks very much.


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


================
Comment at: libunwind/src/UnwindLevel1.c:55
+    __asm__ volatile("push %%edi\n\t"                                          \
+                     "sub $4, %%esp\n\t"                                       \
+                     "jmp *%%edx\n\t" ::"D"(cetRegContext),                    \
----------------
MaskRay wrote:
> 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.
Hi, @maskray
I also think using C++ keyword "asm" is better but libunwind source has already used "__asm__": https://github.com/llvm/llvm-project/blob/main/libunwind/src/config.h#L79  So I aligned with this style.
Thanks very much.


================
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__)
----------------
MaskRay wrote:
> on Linux x86 platforms.
Fixed.
Thanks very much.


================
Comment at: libunwind/test/lit.site.cfg.in:30
 config.cxx_ext_threads          = @LIBUNWIND_BUILD_EXTERNAL_THREAD_LIBRARY@
+config.cet_on                   = @LIBUNWIND_ENABLE_CET@
 
----------------
MaskRay wrote:
> perhaps `config.x86_cet`
Fixed.
Thanks very much.


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

https://reviews.llvm.org/D105968