This revision was automatically updated to reflect the committed changes.
Closed by commit rL341125: Add support for SEH unwinding on Windows. (authored
by cdavis, committed by ).
Repository:
rL LLVM
https://reviews.llvm.org/D50564
Files:
libunwind/trunk/include/__libunwind_config.h
rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.
In https://reviews.llvm.org/D50564#1211477, @mstorsjo wrote:
> Not much more comments from me. The implementation seems reasonable, and
> works for one simple test I did (with an earlier revision
cdavis5x added a comment.
Ping.
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
cdavis5x updated this revision to Diff 162509.
cdavis5x added a comment.
- Move `DISPATCHER_CONTEXT` definition into UnwindCursor.hpp.
Repository:
rUNW libunwind
https://reviews.llvm.org/D50564
Files:
include/__libunwind_config.h
include/unwind.h
src/AddressSpace.hpp
cdavis5x updated this revision to Diff 162441.
cdavis5x marked 8 inline comments as done.
cdavis5x added a comment.
- Remove case we'll never realistically hit.
- Add back include I removed earlier.
- Add a comment clarifying that some environments use SEH without Windows
runtime support.
kristina added a comment.
I'd say LGTM since it's an introduction of any sort of **runtime** within the
LLVM project scope that deals with SEH specifically. So far all the published
code is pretty much exclusively related to Clang/LLVM IR and MC support for
codegen of SEH related code, but
mstorsjo added a comment.
Not much more comments from me. The implementation seems reasonable, and works
for one simple test I did (with an earlier revision of the patch at least), and
further refinement can happen in-tree I guess.
I'd like to have someone else (@rnk @compnerd?) give it a more
cdavis5x marked 2 inline comments as done.
cdavis5x added inline comments.
Comment at: src/Unwind-seh.cpp:163
+#ifdef __x86_64__
+unw_get_reg(, UNW_X86_64_RAX, );
+unw_get_reg(, UNW_X86_64_RDX, >private_[3]);
mstorsjo wrote:
> Without understanding the
cdavis5x updated this revision to Diff 162250.
cdavis5x marked 7 inline comments as done.
cdavis5x added a comment.
- Remove unnecessary code.
- Guard against rogue personality functions returning wrong values.
- Add a comment clarifying the purpose of the `new_ctx` variable.
Repository:
rUNW
kristina added inline comments.
Comment at: src/UnwindCursor.hpp:1801
+ if (pc != getLastPC()) {
+UNWIND_INFO *xdata = reinterpret_cast(base +
unwindEntry->UnwindData);
+if (xdata->Flags & (UNW_FLAG_EHANDLER|UNW_FLAG_UHANDLER)) {
mstorsjo wrote:
> I
mstorsjo added inline comments.
Comment at: src/Unwind-seh.cpp:163
+#ifdef __x86_64__
+unw_get_reg(, UNW_X86_64_RAX, );
+unw_get_reg(, UNW_X86_64_RDX, >private_[3]);
Without understanding the code flow completely - is there a risk that this
tries to use
kristina added a comment.
In https://reviews.llvm.org/D50564#1208169, @cdavis5x wrote:
> OK, I've removed some of the dependencies on Windows-specific types. The code
> in `Unwind-seh.cpp` is very Windows-specific, though, so I left it alone.
Much appreciated, thank you for your work. LGTM in
cdavis5x added a comment.
OK, I've removed some of the dependencies on Windows-specific types. The code
in `Unwind-seh.cpp` is very Windows-specific, though, so I left it alone.
Repository:
rUNW libunwind
https://reviews.llvm.org/D50564
___
cdavis5x updated this revision to Diff 161807.
cdavis5x added a comment.
- Make a bit more friendly to non-Windows.
- Fix bits that depend on https://reviews.llvm.org/D50413.
Repository:
rUNW libunwind
https://reviews.llvm.org/D50564
Files:
include/__libunwind_config.h
include/unwind.h
cdavis5x added a comment.
In https://reviews.llvm.org/D50564#1207493, @kristina wrote:
> In https://reviews.llvm.org/D50564#1206393, @mstorsjo wrote:
>
> > In https://reviews.llvm.org/D50564#1206370, @cdavis5x wrote:
> >
> > > In https://reviews.llvm.org/D50564#1206302, @kristina wrote:
> > >
>
kristina added a comment.
In https://reviews.llvm.org/D50564#1206393, @mstorsjo wrote:
> In https://reviews.llvm.org/D50564#1206370, @cdavis5x wrote:
>
> > In https://reviews.llvm.org/D50564#1206302, @kristina wrote:
> >
> > > I'm all for this change except the core issue is that you're using
>
mstorsjo added a comment.
In https://reviews.llvm.org/D50564#1206370, @cdavis5x wrote:
> In https://reviews.llvm.org/D50564#1206302, @kristina wrote:
>
> > I'm all for this change except the core issue is that you're using
> > libunwind as a shim around the actual unwinding API provided by
cdavis5x added a comment.
In https://reviews.llvm.org/D50564#1206302, @kristina wrote:
> I'm all for this change except the core issue is that you're using libunwind
> as a shim around the actual unwinding API provided by Windows. It would be
> nice to have something that did not have to do
kristina added a comment.
I'm all for this change except the core issue is that you're using libunwind as
a shim around the actual unwinding API provided by Windows. It would be nice to
have something that did not have to do that and was capable of performing
unwinding of SEH-style exceptions
cdavis5x added a comment.
Ping...
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
cdavis5x added a comment.
Ping.
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
cdavis5x updated this revision to Diff 160660.
cdavis5x added a comment.
- Get rid of DISPATCHER_CONTEXT def for ARM. We only support ARM on Win8+
anyway.
Repository:
rUNW libunwind
https://reviews.llvm.org/D50564
Files:
include/__libunwind_config.h
src/AddressSpace.hpp
cdavis5x added a comment.
...And it turns out Phab marked the comments as done when I uploaded the new
change. I didn't know it would do that. That's useful to know.
Repository:
rUNW libunwind
https://reviews.llvm.org/D50564
___
cfe-commits
cdavis5x added a comment.
Marked some comments as done. (Phab won't let me post an empty comment, so...)
Repository:
rUNW libunwind
https://reviews.llvm.org/D50564
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
cdavis5x updated this revision to Diff 160638.
cdavis5x marked 3 inline comments as done.
cdavis5x edited the summary of this revision.
cdavis5x added a comment.
- Update checks for DISPATCHER_CONTEXT definition.
- Add link to KJK::Hyperion's articles on SEH.
Repository:
rUNW libunwind
cdavis5x added a comment.
In https://reviews.llvm.org/D50564#1195985, @cdavis5x wrote:
> In https://reviews.llvm.org/D50564#1195794, @mstorsjo wrote:
>
> > > Special thanks to KJK::Hyperion for his excellent series of articles on
> > > how EH works on x86-64 Windows. (Seriously, check it out.
mstorsjo added a comment.
In https://reviews.llvm.org/D50564#1199285, @rnk wrote:
> In https://reviews.llvm.org/D50564#1198996, @cdavis5x wrote:
>
> > Could somebody verify that the `DISPATCHER_CONTEXT` struct is defined in
> > `` for the Win8 and Win10 SDKs? I can't install them right now.
>
>
zturner added a comment.
In https://reviews.llvm.org/D50564#1199285, @rnk wrote:
> In https://reviews.llvm.org/D50564#1198996, @cdavis5x wrote:
>
> > Could somebody verify that the `DISPATCHER_CONTEXT` struct is defined in
> > `` for the Win8 and Win10 SDKs? I can't install them right now.
>
>
rnk added a comment.
In https://reviews.llvm.org/D50564#1198996, @cdavis5x wrote:
> Could somebody verify that the `DISPATCHER_CONTEXT` struct is defined in
> `` for the Win8 and Win10 SDKs? I can't install them right now.
I checked both, and they are available, so I think we should guard the
cdavis5x added a comment.
Could somebody verify that the `DISPATCHER_CONTEXT` struct is defined in
`` for the Win8 and Win10 SDKs? I can't install them right now.
Repository:
rUNW libunwind
https://reviews.llvm.org/D50564
___
cfe-commits
rnk added inline comments.
Comment at: src/libunwind_ext.h:43
+ #if defined(__x86_64__) && !defined(__MINGW64__)
+typedef struct _DISPATCHER_CONTEXT {
+ ULONG64 ControlPc;
cdavis5x wrote:
> mstorsjo wrote:
> > What's this all about? winnt.h (from both MSVC and
cdavis5x added a comment.
In https://reviews.llvm.org/D50564#1195794, @mstorsjo wrote:
> > 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
cdavis5x updated this revision to Diff 160195.
cdavis5x added a comment.
- Fix outdated comment.
- Make preprocessor conditional more consistent.
- Make some private functions used only in a single file static.
Repository:
rUNW libunwind
https://reviews.llvm.org/D50564
Files:
cdavis5x added inline comments.
Comment at: src/Unwind-seh.cpp:53
+
+/// Exception cleanup routine used by \c __libunwind_frame_consolidate to
+/// regain control after handling an SEH exception.
mstorsjo wrote:
> I don't see any `__libunwind_frame_consolidate`
cdavis5x added inline comments.
Comment at: src/libunwind_ext.h:43
+ #if defined(__x86_64__) && !defined(__MINGW64__)
+typedef struct _DISPATCHER_CONTEXT {
+ ULONG64 ControlPc;
mstorsjo wrote:
> What's this all about? winnt.h (from both MSVC and mingw-w64)
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
cdavis5x created this revision.
cdavis5x added reviewers: mstorsjo, rnk, compnerd, smeenai.
Herald added subscribers: cfe-commits, chrib, christof, kristof.beyls, mgorny.
Herald added a reviewer: javed.absar.
I've tested this implementation on x86-64 to ensure that it works. All
`libc++abi` tests
37 matches
Mail list logo
