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

[Saleem Abdulrasool via Phabricator via libcxx-commits]

compnerd added inline comments.


================
Comment at: libunwind/CMakeLists.txt:55
 option(LIBUNWIND_BUILD_32_BITS "Build 32 bit libunwind" ${LLVM_BUILD_32_BITS})
+option(LIBUNWIND_BUILD_CET_ENABLE "Build libunwind with CET enabled" ${LLVM_BUILD_CET_ENABLE})
 option(LIBUNWIND_ENABLE_ASSERTIONS "Enable assertions independent of build mode." ON)
----------------
Please name this `LIBUNWIND_ENABLE_CET`


================
Comment at: libunwind/CMakeLists.txt:146
+  set(LIBUNWIND_CET_ON False)
+endif()
+
----------------
I don't think that this is needed.  You should be able to directly use `LIBUNWIND_BUILD_CET_ENABLE` at the sites using this local variable.


================
Comment at: libunwind/CMakeLists.txt:188
+  add_compile_flags_if_supported(-fcf-protection=full)
+  add_compile_flags_if_supported(-mshstk)
+endif()
----------------
I think that we want to test and set:
- `/Gy`
- `/guard:ehcont`
- `/guard:cf`
- `/cetcompat`

Or explicitly error with MSVC.


================
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
----------------
Why does this have to be a macro?  Can we just define the function instead?


================
Comment at: libunwind/src/UnwindLevel1.c:48
+#define __unw_phase2_resume(cursor, fn) __unw_resume((cursor))
+#elif _LIBUNWIND_CET_ENABLED == 1
+#define __unw_phase2_resume(cursor, fn)                                        \
----------------
Can we avoid the value here and use `#if defined(__x86_64__) || defined(_M_ARM64)` instead to select between the inline assembly?


================
Comment at: libunwind/src/assembly.h:18
 
+#if (defined(__i386__) || defined(__x86_64__)) && defined(__linux__)
+#include <cet.h>
----------------
Do you need to consider MinGW or cywin?


================
Comment at: libunwind/src/cet_unwind.h:12
+#define _CET_UNWIND_H_
+#include "libunwind.h"
+#include <cet.h>
----------------
Please add a newline above this line.


================
Comment at: libunwind/src/libunwind_ext.h:37
+#if defined(_LIBUNWIND_CET_ENABLED)
+extern void *__unw_cet_get_registers(unw_cursor_t *);
+#endif
----------------
Why is this new interface needed?  Who is the expected user?


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

https://reviews.llvm.org/D105968