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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
