Page MenuHomePhabricator

[libunwind] Add support for dwarf unwinding on windows on x86_64
ClosedPublic

Authored by mstorsjo on Oct 11 2017, 1:15 PM.

Details

Summary

Clang doesn't currently support building for windows/x86_64 with dwarf by setting command line parameters, but if manually modified to use dwarf, we can make libunwind work in this configuration as well.

Diff Detail

Repository
rL LLVM

Event Timeline

mstorsjo created this revision.Oct 11 2017, 1:15 PM

As further explanation/justification - libcxxabi and libunwind don't support SEH exceptions yet.

compnerd added inline comments.Oct 14 2017, 5:30 PM
include/unwind.h
125 ↗(On Diff #118678)

I think I would prefer that we do this generically as:

#if __POINTER_WIDTH__ == 32
src/AddressSpace.hpp
145 ↗(On Diff #118678)

I think I prefer the generic:

#if __POINTER_WIDTH__ == 64
197 ↗(On Diff #118678)

Same.

src/UnwindRegistersRestore.S
68 ↗(On Diff #118678)

This is confusing. Why is this _WIN32? Shouldn't this be _WIN64?

72 ↗(On Diff #118678)

Hmm, why is this $16? The $rsp was adjusted by $8 in the setjmp.

src/UnwindRegistersSave.S
66 ↗(On Diff #118678)

Shouldn't this be _WIN64?

mstorsjo added inline comments.Oct 15 2017, 3:34 AM
include/unwind.h
125 ↗(On Diff #118678)

It seems like GCC doesn't set __POINTER_WIDTH__, but both GCC and clang set __SIZEOF_POINTER__, so I can use that.

src/AddressSpace.hpp
145 ↗(On Diff #118678)

This one probably could be simplified even further by just using intptr_t and uintptr_t, right?

src/UnwindRegistersRestore.S
68 ↗(On Diff #118678)

I guess I could use that as well. I just used _WIN32 out of habit as generic identifier for windows-in-whatever-form; both _WIN32 and _WIN64 are defined, and we're within an #ifdef __x86_64__ anyway. I can change it into _WIN64 for clarity.

72 ↗(On Diff #118678)

This is the exact same thing as is done right now for x86_64 on unixy systems already, just with different registers.

The adjustment by 16 bytes is because we store rcx and rip on the stack here to restore them slightly differently than the others. See the store new rcx/rip on new stack, restore rcx later and rcx was saved here earlier and rip was saved here comments below.

src/UnwindRegistersSave.S
66 ↗(On Diff #118678)

Same as the otherone; we're within #ifdef __x86_64__, but I can change it into _WIN64 for clarity.

mstorsjo updated this revision to Diff 119068.Oct 15 2017, 3:47 AM

Use __SIZEOF_POINTER__ instead of checking __LP64__ together with _WIN64. Changed _WIN32 into _WIN64 within the assembly sources for consistency/clarity. Removed one type definition ifdef by just using (u)intptr_t instead.

martell added inline comments.
docs/index.rst
55 ↗(On Diff #119068)

Based on your comments above I assume this supports GCC also ?
If so, replace
Windows i386, x86_64 Clang DWARF CFI
with
Windows i386, x86_64 Clang, GCC DWARF CFI

This is exciting stuff Martin :)

mstorsjo added inline comments.Oct 15 2017, 10:54 AM
docs/index.rst
55 ↗(On Diff #119068)

I'm not sure if gcc supports dwarf exceptions on x86_64 or how much hacking is needed to enable it; the comment about gcc for __SIZEOF_POINTER__ was because it was in generic code that could be used by gcc in one of the existing cases where gcc actually is supported.

compnerd added inline comments.Oct 22 2017, 2:22 PM
src/UnwindRegistersRestore.S
98 ↗(On Diff #119068)

Doesn't Win64 ABI require some of the MMX registers be saved/restored too?

mstorsjo added inline comments.Oct 22 2017, 2:30 PM
src/UnwindRegistersRestore.S
98 ↗(On Diff #119068)

Right, yes, xmm6-xmm15 should be backed up and restored. I'll try to amend this with such a change.

mstorsjo added inline comments.Oct 23 2017, 3:08 AM
src/UnwindRegistersRestore.S
98 ↗(On Diff #119068)

Actually, such a change doesn't necessarily make much sense on its own.

As long as the dwarf encoding itself doesn't describe how to restore those registers (and on unix platforms you don't need to, so it probably isn't even specified), you'd just end up backing up the xmm registers on entry when throwing the exception, and restoring the exactly same ones again - it only guards against changes within libcxxabi/libunwind and the unwinding machinery itself, not against changes further down in the call stack between the thrower and catcher of the exception.

So with that, I guess this patch is futile unless planning to extend the x86_64 dwarf handling in llvm to include those registers as well - and that's a little out of scope of what I intended to do here...

rnk added inline comments.Oct 23 2017, 2:25 PM
src/UnwindRegistersRestore.S
98 ↗(On Diff #119068)

If we have XMM values in the register context, we might as well reload them here. I assume libunwind will switch away from DWARF and over to UNWIND_INFO opcodes in the near future, and that will give us an accurate register context.

mstorsjo added inline comments.Oct 23 2017, 2:34 PM
src/UnwindRegistersRestore.S
98 ↗(On Diff #119068)

Yeah, although to do anything useful with the XMM registers, the llvm backend would have to output dwarf opcodes for callee saved xmm registers, which it currently doesn't. Otherwise it'll just reload them back to the same values as they already are. I've managed to hack LLVM to do that (without fully understanding what I'm doing though), and I'll see if I manage to update libunwind to use that as well.

mstorsjo updated this revision to Diff 120215.Oct 25 2017, 2:27 AM
mstorsjo edited the summary of this revision. (Show Details)

Updated to take care of saving/restoring XMM registers as well.

aprantl removed a subscriber: aprantl.Oct 25 2017, 9:01 AM
mstorsjo added inline comments.Oct 25 2017, 2:13 PM
src/UnwindRegistersRestore.S
72 ↗(On Diff #118678)

Instead of duplicating the asm for the win64 and unix calling convention, I can also pretty easily template it using the cpp, using a define for whatever register holds the context pointer from the first function argument. Would that be better, or is it clearer just to have it typed out in both forms literally?

mstorsjo updated this revision to Diff 120380.Oct 26 2017, 3:19 AM

Simplified the assembly by sharing much more code between the unix and win64 paths, added a padding element to ensure more stable struct packing.

rnk accepted this revision.Oct 26 2017, 9:49 AM

lgtm

This revision is now accepted and ready to land.Oct 26 2017, 9:49 AM
This revision was automatically updated to reflect the committed changes.