[Lldb-commits] [PATCH] D62213: [ABI] Implement Windows ABI for x86_64

[Wanyi Ye via Phabricator via lldb-commits]

kusmour marked 16 inline comments as done.
kusmour added inline comments.


================
Comment at: lldb/source/Plugins/ABI/Windows-x86_64/ABIWindows_x86_64.cpp:1257-1259
+
+    // We currently only support extracting values with Clang QualTypes. Do we
+    // care about others?
----------------
xiaobai wrote:
> I don't see any references to clang in the below code. Is this still accurate?
not relevant, deleted


================
Comment at: lldb/source/Plugins/ABI/Windows-x86_64/ABIWindows_x86_64.cpp:1359
+      } else {
+        // FIXME - don't know how to do 80 bit long doubles yet.
+        error.SetErrorString(
----------------
compnerd wrote:
> FP80 is not supported on Windows, this should be a hard error.
I changed it to provide a correct error message. Is it better to put an assertion or return here?


================
Comment at: lldb/source/Plugins/ABI/Windows-x86_64/ABIWindows_x86_64.cpp:1466
+              } else if (*byte_size == sizeof(long double)) {
+                // Don't handle long double since that can be encoded as 80 bit
+                // floats...
----------------
compnerd wrote:
> `long double` is the same as `double` on Windows.
got it thanks


================
Comment at: lldb/source/Plugins/ABI/Windows-x86_64/ABIWindows_x86_64.cpp:1811
+
+// Windows doesn't use rbp
+// Let this return false
----------------
xiaobai wrote:
> nit: Windows-x86_64 doesn't use rbp
thanks


================
Comment at: lldb/source/Plugins/ABI/Windows-x86_64/ABIWindows_x86_64.h:59
+
+	  if (cfa & (8ull - 1ull))
+      return false; // Not 8 byte aligned
----------------
compnerd wrote:
> This should be made strict as per the Windows ABI.
windows is 16-byte aligned, updated.


================
Comment at: lldb/source/Plugins/ABI/Windows-x86_64/ABIWindows_x86_64.h:63
+      return false; // Zero is not a valid stack address
+    return true;
+  }
----------------
compnerd wrote:
> Can we add an additional test please?  In particular, leaf frames are permitted to have unaligned stacks.
can you provide more information about this? On windows a leaf function doesn't require a stack frame. But a frame function that doesn't call other function is not required to align the stack


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62213/new/

https://reviews.llvm.org/D62213