mstorsjo added a comment.

> I've tested this implementation on x86-64 to ensure that it works. All 
> libc++abi tests pass, as do all libc++ exception-related tests.

I tested it now as well, and seems to work for my simple testcase.

> ARM still remains to be implemented (@compnerd?).

LLVM doesn't generate SEH unwind data for ARM at all yet. ARM64 implementation 
is ongoing at the moment though, see https://reviews.llvm.org/D50166 and 
https://reviews.llvm.org/D50288.

> Special thanks to KJK::Hyperion for his excellent series of articles on how 
> EH works on x86-64 Windows. (Seriously, check it out. It's awesome.)

Can you give some links to it? A brief googling didn't turn up much else than 
the PSEH library.

> I'm actually not sure if this should go in as is. I particularly don't like 
> that I duplicated the UnwindCursor class for this special case.

As I'm not totally familiar with how it works, I can only give cursory comments 
and compare to the libgcc implementation when trying to wrap my head around it, 
but this does seem way, way more complex than the libgcc version of the same.

As for the duplicated UnwindCursor, I'm thinking if it'd become more 
straightforward by avoiding touching those parts altogether, and just 
reimplementing all the other public functions, `_Unwind_G/SetGR/IP`, 
`_Unwind_Backtrace` etc, directly in Unwind-seh.cpp, and not care about using 
the libunwind.h APIs (`unw_*`) inbetween altogether? Or perhaps my libgcc 
comparison is making me biased.



================
Comment at: src/Unwind-seh.cpp:53
+
+/// Exception cleanup routine used by \c __libunwind_frame_consolidate to
+/// regain control after handling an SEH exception.
----------------
I don't see any `__libunwind_frame_consolidate` anywhere, is this comment 
outdated?


================
Comment at: src/UnwindLevel1.c:35
 
+#if !_LIBUNWIND_SUPPORT_SEH_UNWIND
+
----------------
This probably works, but isn't `#if !defined(_LIBUNWIND_SUPPORT_SEH_UNWIND)` 
more of the common form?


================
Comment at: src/libunwind_ext.h:43
+  #if defined(__x86_64__) && !defined(__MINGW64__)
+typedef struct _DISPATCHER_CONTEXT {
+  ULONG64 ControlPc;
----------------
What's this all about? winnt.h (from both MSVC and mingw-w64) should define 
this struct, no?


================
Comment at: src/libunwind_ext.h:73
+  #endif
+extern int _unw_init_seh(unw_cursor_t *cursor, CONTEXT *ctx);
+extern DISPATCHER_CONTEXT *_unw_seh_get_disp_ctx(unw_cursor_t *cursor);
----------------
These are all both defined and called from Unwind-seh.cpp, so couldn't they 
just be static functions within there?


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

Reply via email to