[PATCH] D50564: Add support for SEH unwinding on Windows.

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: src/Unwind-seh.cpp:163
+#ifdef __x86_64__
+    unw_get_reg(&cursor, UNW_X86_64_RAX, &retval);
+    unw_get_reg(&cursor, UNW_X86_64_RDX, &exc->private_[3]);
----------------
Without understanding the code flow completely - is there a risk that this tries to use an uninitialized cursor, in case we hit `ctx = (struct _Unwind_Context *)ms_exc->ExceptionInformation[1];` above and never initialized the local cursor?


================
Comment at: src/Unwind-seh.cpp:174
+    CONTEXT new_ctx;
+    RtlUnwindEx(frame, (PVOID)target, ms_exc, (PVOID)retval, &new_ctx, disp->HistoryTable);
+    _LIBUNWIND_ABORT("RtlUnwindEx() failed");
----------------
Who will get this new uninitialized CONTEXT here, and will they try to use it for something?


================
Comment at: src/UnwindCursor.hpp:31
+struct UNWIND_INFO {
+  uint8_t Version : 3;
+  uint8_t Flags : 5;
----------------
Hmm, I think this one is supposed to be arch independent. Although it's easy to remove the ifdef once we want to use it for anything else than x86_64...


================
Comment at: src/UnwindCursor.hpp:54
 #include "EHHeaderParser.hpp"
-#include "libunwind.h"
+#include "libunwind_ext.h"
 #include "Registers.hpp"
----------------
This looks like another leftover; there's no `libunwind_ext.h` any longer here.


================
Comment at: src/UnwindCursor.hpp:482
+  int stepWithSEHData() {
+#if !defined(_LIBUNWIND_TARGET_I386)
+    _dispContext.LanguageHandler = RtlVirtualUnwind(UNW_FLAG_UHANDLER,
----------------
Maybe skip the i386 ifdefs here, to keep it concise, as we don't expect to build this with `__SEH__` defined for i386.


================
Comment at: src/UnwindCursor.hpp:625
+  memset(&_histTable, 0, sizeof(_histTable));
+  // FIXME
+  // fill in _registers from thread arg
----------------
If this constructor is unused and unimplemented, I'd omit it altogether.


================
Comment at: src/UnwindCursor.hpp:1157
 
+#if defined(_LIBUNWIND_SUPPORT_SEH_UNWIND)
+  pint_t getLastPC() const { /* FIXME: Implement */ return 0; }
----------------
What does this whole block do here? Isn't this within an !SEH ifdef block? The same methods are declared for the SEH version of this struct further above.


================
Comment at: src/UnwindCursor.hpp:1801
+  if (pc != getLastPC()) {
+    UNWIND_INFO *xdata = reinterpret_cast<UNWIND_INFO *>(base + unwindEntry->UnwindData);
+    if (xdata->Flags & (UNW_FLAG_EHANDLER|UNW_FLAG_UHANDLER)) {
----------------
I can't say I understand all of this yet, but I'm slowly getting there, and I'm trying to compare this to what libgcc does.

libgcc doesn't use any definition of UNWIND_INFO and doesn't need to do the equivalent of `getInfoFromSEH`, used by `step()`, anywhere. `unw_step()` is used in `_Unwind_ForcedUnwind`, which in libgcc is implemented using `RaiseException (STATUS_GCC_FORCED, ...`.

I guess if you happen to have all of the `unw_step` API available, it's easier to just do it like this, in custom code without relying on the NTDLL functions for it, while the libgcc version relies more on the NTDLL API.


Repository:
  rUNW libunwind

https://reviews.llvm.org/D50564