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

Have you checked `check-cxxabi` `test/forced_unwind[12].pass.cpp`? Do they pass with CET?



================
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
----------------
xiongji90 wrote:
> xiongji90 wrote:
> > compnerd wrote:
> > > xiongji90 wrote:
> > > > 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.
> > > I wonder if we can get away with some combination of `noinline`, and pragmas for `no-optimize-sibling-calls`.
> > Hi, @compnerd 
> > I think using combination of following:
> > 1. using __attribute__((noinline)) to decorate target functions
> > 2. building source file containing target functions with -fno-optimize-sibling-calls
> > can help us to get away with the issue mentioned above. I only found gcc/clang compiling options for "-fno-optimize-sibling-calls" and didn't find corresponding pragma or attribute, so if we choose to use "-fno-optimize-sibling-calls", tail call optimization for other functions will be disabled too which seems to be overshooting.
> > And if choosing the combination of "noinline" and "-fno-optimize-sibling-calls", we are relying on gcc/clang's implementation which is a implicit dependency, using macro here should be safe to all compilers.
> > 
> > Thanks very much.
> Hi, @compnerd 
> If you have concern about the macro "unw_phase_resume", I can try to update the patch to use combination of "noinline" and "-fno-optimize-sibling-calls". And do we have any way to use "no-optimize-sibling-calls" to decorate a function? I only found compiling option "-fno-optimize-sibling-calls".
> Thanks very much. 
I think using a macro is a right call here, more robust than dancing with -noinline, no-optimize-sibling-calls and using functions and computing how many additional frames are skipped.


================
Comment at: libunwind/src/UnwindLevel1.c:51
+  do {                                                                         \
+    _LIBUNWIND_POP_CET_SSP(1 + (fn));                                          \
+    void *cetRegContext = __libunwind_cet_get_registers((cursor));             \
----------------
Question: why is this `1 + (fn)`?


================
Comment at: libunwind/src/UnwindLevel1.c:55
+    __asm__ volatile("push %%edi\n\t"                                          \
+                     "sub $4, %%esp\n\t"                                       \
+                     "jmp *%%edx\n\t" ::"D"(cetRegContext),                    \
----------------
Is the calling convention here wrong?




================
Comment at: libunwind/src/UnwindLevel1.c:173
 
+  unsigned int framesWalked = 0;
   // Walk each frame until we reach where search phase said to stop.
----------------



================
Comment at: libunwind/src/UnwindLevel1.c:223
 
+    framesWalked++;
     // If there is a personality routine, tell it we are unwinding.
----------------
https://llvm.org/docs/CodingStandards.html#prefer-preincrement


================
Comment at: libunwind/src/UnwindLevel1.c:287
 
+  unsigned int framesWalked = 0;
   // Walk each frame until we reach where search phase said to stop
----------------



================
Comment at: libunwind/src/cet_unwind.h:20
+    defined(__SHSTK__) &&                                                      \
+    (defined(_LIBUNWIND_TARGET_I386) || defined(_LIBUNWIND_TARGET_X86_64))
+#define _LIBUNWIND_CET_ENABLED 1
----------------
Since CET is x86 only, `(defined(_LIBUNWIND_TARGET_I386) || defined(_LIBUNWIND_TARGET_X86_64))` can be omitted.


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

https://reviews.llvm.org/D105968