LGTM, but I'd feel better if someone else signed off on this, too.

Ping.

• Detect libgcc.

Ping.

Ping.

Consensus seems clear to me. Patch abandoned.

Ping.

• Move DISPATCHER_CONTEXT definition into UnwindCursor.hpp.

• 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.

• Remove unnecessary code.
• Guard against rogue personality functions returning wrong values.
• Add a comment clarifying the purpose of the new_ctx variable.

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.

• Make a bit more friendly to non-Windows.
• Fix bits that depend on D50413.

In D50564#1207493, @kristina wrote:

In D50564#1206393, @mstorsjo wrote:

In D50564#1206370, @cdavis5x wrote:

In 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 that and was capable of performing unwinding of SEH-style exceptions without needing additional runtime support.

It would be nice, but that would require extra work. We'd have to implement reading and interpreting unwind codes, and calling any handlers present at each frame (which all have a different calling convention from Itanium handlers), and handling chained unwind info... Or we could use the implementation that MS provided to us for free--and which gets loaded into every process anyway by virtue of being in NTDLL, and which is extremely well tested. Given all that, I'm wondering what implementing all that ourselves would gain us. I suppose we could eventually do all that, but for now, I think this is outside the scope of my change.

+1. I guess such a library would be very nice to have, but from the point of view of implementing exception handling, using the underlying APIs probably is the way to go. The other question, as posted before, is whether we want to wrap the whole CONTEXT structs in the UnwindCursor class and expose it via the unw_* set of APIs. It gives quite a significant amount of extra code here compared to libgcc's unwind-seh.c which is <500 loc altogether, providing only the _Unwind_* API, implementing it directly with the Windows Rtl* APIs from ntdll. That would give only a partial libunwind, with only the higher level API available, but that's the only part used for exception handling at least.

That still makes the underlying assumption that SEH is exclusive to Windows (and by that virtue PE/COFF) which doesn't necessarily hold true. There is also the issue of generic names like _LIBUNWIND_SUPPORT_SEH_UNWIND to guard includes of Windows specific headers which ties into my previous point. Would it be possible to somehow abstract away the Windows runtime function call and Windows specific header includes, even if it's via CMake options.

I'm raising this issue as there are other implementations of SEH that share the same (or extremely similar, depending on SDK versions) structures, but would not have the Windows headers available when compiling libunwind, and while the runtime function call to the Rtl* API is easy to change later on, separating rest of it from Windows headers is slightly messier.

Would it, at very least, be possible to decouple most of it from a dependency on Windows headers and if possible use things like void* in place of PVOID or anything that's stdint.h/inttypes.h friendly? It's an excellent patch regardless but loosening the Windows SDK dependencies a bit would make it a lot better.

Would be much appreciated, as I'm going through a backlog of patches I'd like to put up for review one of which includes SEH for x86_64 Linux (ELF) implemented using RT signals and additional compiler runtime glue code, currently residing within compiler-rt builtins (one of the issues I want to address before). It's quite a wonky ABI since I tried to keep as much compatible with 64-bit Windows SEH (.xdata variation) in terms of frame lowering etc, but run-time mechanisms differ vastly for obvious reasons. Having it interop with libunwind smoothly without rewriting much of existing code in this patch would be nice, or even worse resorting to a million preprocessor guards to cover all the cases.

In 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 that and was capable of performing unwinding of SEH-style exceptions without needing additional runtime support.

Ping...

Are y'all waiting for me to do something?

Ping.

Ping.

• Get rid of DISPATCHER_CONTEXT def for ARM. We only support ARM on Win8+ anyway.

...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.

Marked some comments as done. (Phab won't let me post an empty comment, so...)

• Update checks for DISPATCHER_CONTEXT definition.
• Add link to KJK::Hyperion's articles on SEH.

In D50564#1195985, @cdavis5x wrote:

In 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 didn't turn up much else than the PSEH library.

I can't seem to find it either. I wonder if it's vanished from the Internet. Maybe the IA will have something.

Could somebody verify that the DISPATCHER_CONTEXT struct is defined in <winnt.h> for the Win8 and Win10 SDKs? I can't install them right now.

Ping.

In 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 didn't turn up much else than the PSEH library.

• Fix outdated comment.
• Make preprocessor conditional more consistent.
• Make some private functions used only in a single file static.

In D50413#1195101, @mstorsjo wrote:

@cdavis5x I presume the fact that this one turned out tricky is blocking submitting the SEH unwinding patch.

Would it be worth to rework that patch to just use the basic types just like libunwind does today, e.g. having _Unwind_GetRegionStart return plain uintptr_t instead of _Unwind_Ptr, which also would be consistent with the existing entry points in Unwind-EHABI.cpp, Unwind-sjlj.c and UnwindLevel1.c, in order to be able to submit it before this one gets sorted out?

In D50413#1191726, @krytarowski wrote:

NetBSD uses typedef void *_Unwind_Ptr; unconditionally in its <unwind.h>... if that has to be matched.

Done.

Additionally: typedef long _Unwind_Word;.

Done.

• Add NetBSD-specific definitions.
• Pull out common declaration of __personality_routine.

• Remove unneeded preprocessor condition.

In D50414#1191834, @mstorsjo wrote:

Should we maybe add the same declaration of _GCC_specific_handler to clang's unwind.h? That would allow removing the forward declaration in libcxxabi from D49638.

Probably. I believe it's present in GCC's <unwind.h>, too.

Do you plan on implementing these SEH specific bits in libunwind,

Already have. Patch forthcoming.

Rebasing against HEAD.

Are you sure this shouldn't also define _UNICODE? looks Hmm... GCC doesn't define it. I wonder if we should anyway. A user who set -municode may also be using <tchar.h>, and thus may want those macros set to their Unicode counterparts.

Extremely belated ping.

Fix the build error we saw in trunk.

I think I've figured out why the build broke when I landed this. (Sorry that took so long.)

Update for merge conflicts.

Merge with a more recent revision.

Unfortunately, this patch needs revision due to a merge conflict. :\

@sanjoy: Ping.

Ping.

Factor emission of the patchable instruction into a TTI hook.

For now, I've disallowed it with naked and always_inline/__forceinline attributes. Do any other attributes affect prologue generation in a way that might interfere with ms_hook_prologue? AFAICT, GCC only disallows ms_hook_prologue on a) nested functions and b) functions compiled with -mfentry (neither of which we support, AFAIK).

• Add Sema tests for the ms_hook_prologue attribute.
• Disallow ms_hook_prologue on architectures that Windows doesn't support.
• Disallow ms_hook_prologue with the naked and always_inline attributes.

• Clarify that x86-64 doesn't use a special magic no-op.
• Don't emit patch space with MSVC. The linker adds it for us there.
• Don't force 64-bit functions to use a stack frame. That's only needed for 32-bit.
• Test that, when we do emit the patch space, we emit it before the alignment directive.

• Use Sanjoy's patchable-function attribute.
• Add documentation for this new attribute.

• Use Sanjoy's "patchable-function" attribute.
• Override EmitConstantPool() instead of EmitFunctionEntryLabel().
• Clarify why we're emitting 0xcc bytes (int3 instructions) instead of NOPs.

Ping^3.

Ping...

Ping.

Ping...

Address review comments.

• Rebase onto rL247238.
• Run clang-format on this patch.
• Push va_arg emission down into the ABIInfo hierarchy.

Address @rsmith's comments.

• Rebase onto rL246348.
• Register the __builtin_ms_va_list predef decl. Fixes a nasty interaction with modules.
• Mark __builtin_ms_va_start builtin as manually type-checked.

Closing manually because Phabricator for some reason didn't close it for me...

Abandoning since I gutted the assert(3) in D1622.

Just call X86Subtarget::isCallingConvWin64 in LowerVAARG.

Gut target OS assertion from LowerVAARG.

Big belated update:

• Rebase onto rL243284.
• Go back to checking if the containing function uses the Win64 ABI. Opaque pointer types make checking the va_list type difficult.
• Don't do the "don't save XMM registers" dance in a variadic Win64 ABI function on a SysV platform. (This was something I missed last time.)