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

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.


================
Comment at: lldb/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp:1094
+  if (arch_type == llvm::Triple::x86_64 &&
+      os_type != llvm::Triple::OSType::Win32) {
     return ABISP(new ABISysV_x86_64(process_sp));
----------------
This really isn't correct.  There is no reason to assume that everything on x86_64 uses the SysV ABI.  Can you convert this to a switch over the OS type and return it from the known OSes (Linux, BSD, and Darwin do conform to SysV, the others can return `ABISP()`.


================
Comment at: lldb/source/Plugins/ABI/Windows-x86_64/ABIWindows_x86_64.cpp:1078
+  return g_register_infos;
+}
+
----------------
Ugh, this really feels like copy and paste.  Perhaps we should have a `.inc` file that we can use to replicate the reigster names across the targets.


================
Comment at: lldb/source/Plugins/ABI/Windows-x86_64/ABIWindows_x86_64.cpp:1104
+                                        addr_t func_addr, addr_t return_addr,
+                                        llvm::ArrayRef<addr_t> args) const {
+  Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
----------------
Nit: the indentation seems off.


================
Comment at: lldb/source/Plugins/ABI/Windows-x86_64/ABIWindows_x86_64.cpp:1203
+  }
+  uint32_t byte_size = (bit_width + (8 - 1)) / 8;
+  Status error;
----------------
Mind using `CHAR_BIT` instead of `8`?


================
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(
----------------
FP80 is not supported on Windows, this should be a hard error.


================
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...
----------------
`long double` is the same as `double` on Windows.


================
Comment at: lldb/source/Plugins/ABI/Windows-x86_64/ABIWindows_x86_64.cpp:1492
+    if (byte_size && *byte_size > 0) {
+      const RegisterInfo *altivec_reg =
+          reg_ctx->GetRegisterInfoByName("xmm0", 0);
----------------
lmao, is this a copy paste thing?  I think that `xmm_reg` is better than `altivec_reg` as ALTIVEC is PPC specific.


================
Comment at: lldb/source/Plugins/ABI/Windows-x86_64/ABIWindows_x86_64.cpp:1573
+    uint32_t field_byte_width = field_bit_width / 8;
+    uint32_t field_byte_offset = field_bit_offset / 8 + data_byte_offset;
+
----------------
Mind using `CHAR_BIT` instead of `8` please?


================
Comment at: lldb/source/Plugins/ABI/Windows-x86_64/ABIWindows_x86_64.h:55
+  // in other environments there can be a large number of different functions
+  // involved in async traps.
+  bool CallFrameAddressIsValid(lldb::addr_t cfa) override {
----------------
This comment doesn't make sense on Windows - signal handlers don't really apply to Windows.


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


================
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;
+  }
----------------
Can we add an additional test please?  In particular, leaf frames are permitted to have unaligned stacks.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D62213