[PATCH] D38819: [libunwind] Add support for dwarf unwinding on windows on x86_64
Martin Storsjö via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Sun Oct 15 03:34:49 PDT 2017
mstorsjo added inline comments.
Comment at: include/unwind.h:125
uintptr_t private_2; // holds sp that phase1 found for phase2 to use
+#if !defined(__LP64__) && !defined(_WIN64)
// The implementation of _Unwind_Exception uses an attribute mode on the
> I think I would prefer that we do this generically as:
> #if __POINTER_WIDTH__ == 32
It seems like GCC doesn't set `__POINTER_WIDTH__`, but both GCC and clang set `__SIZEOF_POINTER__`, so I can use that.
Comment at: src/AddressSpace.hpp:145
+#if defined(__LP64__) || defined(_WIN64)
typedef uint64_t pint_t;
> I think I prefer the generic:
> #if __POINTER_WIDTH__ == 64
This one probably could be simplified even further by just using `intptr_t` and `uintptr_t`, right?
Comment at: src/UnwindRegistersRestore.S:68
+# On entry, thread_state pointer is in rcx
> This is confusing. Why is this `_WIN32`? Shouldn't this be `_WIN64`?
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.
Comment at: src/UnwindRegistersRestore.S:72
+ movq 56(%rcx), %rax # rax holds new stack pointer
+ subq $16, %rax
+ movq %rax, 56(%rcx)
> Hmm, why is this `$16`? The `$rsp` was adjusted by `$8` in the `setjmp`.
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.
Comment at: src/UnwindRegistersSave.S:66
+ movq %rax, (%rcx)
> Shouldn't this be `_WIN64`?
Same as the otherone; we're within `#ifdef __x86_64__`, but I can change it into `_WIN64` for clarity.
More information about the cfe-commits