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

[Charles Davis via Phabricator via cfe-commits]

cdavis5x marked 2 inline comments as done.
cdavis5x 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]);
----------------
mstorsjo wrote:
> 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?
We should only hit this point if we're in phase 2 (i.e. `IS_UNWINDING(exc->ExceptionFlags)` is true). But you've now got me worried about the potential for a rogue personality function surreptitiously returning this to exploit libunwind. I've added a check here.


================
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");
----------------
mstorsjo wrote:
> Who will get this new uninitialized CONTEXT here, and will they try to use it for something?
It won't be uninitialized. `RtlUnwindEx()` always starts unwinding from the current frame; it fills in the passed in `CONTEXT` and passes its address to any handlers it encounters along the way. In other words, it's there so `RtlUnwindEx()` has a place to keep track of the unwind context.


================
Comment at: src/UnwindCursor.hpp:1157
 
+#if defined(_LIBUNWIND_SUPPORT_SEH_UNWIND)
+  pint_t getLastPC() const { /* FIXME: Implement */ return 0; }
----------------
mstorsjo wrote:
> 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.
Right now, it doesn't do anything. I added it so @kristina has someplace to put the code for unwinding with SEH data when we're //not// on Windows and we //don't// have the `RtlUnwindEx()` function available. You'll note that the '!SEH' `ifdef` now says:

```lang=c++
#if defined(_LIBUNWIND_SUPPORT_SEH_UNWIND) && defined(_WIN32)
```


================
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)) {
----------------
kristina wrote:
> mstorsjo wrote:
> > 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.
> This primarily deals with the SEH exceptions re-purposed as a C++ exception mechanism on x86_64 (if I understood this right), it's possible to set a custom filter using a runtime call so I suspect GCC does that or defines a translation function (also via a runtime call) which acts as a filter for "true" SEH exceptions behind the scenes deep within the runtime. Typially "true" SEH exceptions don't, outside of runtime magic, play nicely with C++ exceptions, with the `__C_specific_handler` ones being a completely different paradigm that falls far outside the scope of libunwind (those ones being the "true"/explicit SEH exceptions).
> 
> (Don't take my word for it, it's been a while and I only implemented the "true" variation for 64-bit Linux by reserving some RT signals and using that to invoke additional runtime glue that would then do the unwinding, completely ignoring DWARF since CFI exceptions and SEH exceptions really don't mix especially on platforms that are not Windows-like)
Actually, it's just to implement the lower-level `libunwind` APIs--specifically, `unw_get_info()`. The code @kristina is talking about is all in Unwind-seh.cpp.


Repository:
  rUNW libunwind

https://reviews.llvm.org/D50564