[PATCH] D38819: [libunwind] Add support for dwarf unwinding on windows on x86_64

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added inline comments.


================
Comment at: include/unwind.h:125
   uintptr_t private_2; // holds sp that phase1 found for phase2 to use
-#ifndef __LP64__
+#if !defined(__LP64__) && !defined(_WIN64)
   // The implementation of _Unwind_Exception uses an attribute mode on the
----------------
compnerd wrote:
> 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
 public:
-#ifdef __LP64__
+#if defined(__LP64__) || defined(_WIN64)
   typedef uint64_t pint_t;
----------------
compnerd wrote:
> 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
 #
+#if defined(_WIN32)
+# On entry, thread_state pointer is in rcx
----------------
compnerd wrote:
> 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)
----------------
compnerd wrote:
> 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
 DEFINE_LIBUNWIND_FUNCTION(unw_getcontext)
+#if defined(_WIN32)
+  movq  %rax,   (%rcx)
----------------
compnerd wrote:
> Shouldn't this be `_WIN64`?
Same as the otherone; we're within `#ifdef __x86_64__`, but I can change it into `_WIN64` for clarity.


https://reviews.llvm.org/D38819