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

[Saleem Abdulrasool via Phabricator via libcxx-commits]

compnerd accepted this revision.
compnerd added a comment.

Minor comments about the implementation, but I think that this is reasonable.



================
Comment at: libunwind/CMakeLists.txt:188
+  if (NOT LIBUNWIND_SUPPORTS_FCF_PROTECTION_EQ_FULL_FLAG)
+    message(FATAL_ERROR "Compiler doesn't support CET -fcf-protection option!")
+  endif()
----------------
I would recommend `SEND_ERROR` here instead of `FATAL_ERROR`.  This will allow getting subsequent errors as well.


================
Comment at: libunwind/CMakeLists.txt:191
+  if (NOT LIBUNWIND_SUPPORTS_MSHSTK_FLAG)
+    message(FATAL_ERROR "Compiler doesn't support CET -mshstk option!")
+  endif()
----------------
I would recommend `SEND_ERROR` here instead of `FATAL_ERROR`.


================
Comment at: libunwind/src/UnwindCursor.hpp:456
+  virtual void *get_registers() {
+    _LIBUNWIND_ABORT("cet_jumpto not implemented");
+  }
----------------
Most of the warnings indicate the name.  I would suggest we follow the same convention to help quickly identify the function which is unimplemented.


================
Comment at: libunwind/src/UnwindLevel1.c:56
+                     "sub $4, %%esp\n\t"                                       \
+                     "jmp *%%edx\n\t" ::"D"(cetRegContext),                    \
+                     "d"(cetJumpAddress)                                       \
----------------



================
Comment at: libunwind/src/UnwindLevel1.c:57-58
+                     "jmp *%%edx\n\t" ::"D"(cetRegContext),                    \
+                     "d"(cetJumpAddress)                                       \
+                     :);                                                       \
+  } while (0)
----------------



================
Comment at: libunwind/src/UnwindLevel1.c:66
+    void *cetJumpAddress = __libunwind_cet_get_jump_target();                  \
+    __asm__ volatile("jmpq *%%rdx\n\t" ::"D"(cetRegContext),                   \
+                     "d"(cetJumpAddress)                                       \
----------------



================
Comment at: libunwind/src/UnwindLevel1.c:67-68
+    __asm__ volatile("jmpq *%%rdx\n\t" ::"D"(cetRegContext),                   \
+                     "d"(cetJumpAddress)                                       \
+                     :);                                                       \
+  } while (0)
----------------



================
Comment at: libunwind/src/cet_unwind.h:10-11
+
+#ifndef _CET_UNWIND_H_
+#define _CET_UNWIND_H_
+
----------------



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



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

https://reviews.llvm.org/D105968