[Lldb-commits] [PATCH] D31131: [LLDB] OpenBSD support

[Mark Kettenis via Phabricator via lldb-commits]

kettenis marked 2 inline comments as done.
kettenis added a comment.

Will revise the diff based on your comments. Thanks!



================
Comment at: include/lldb/Host/Config.h:35
+
+#include "lldb/Host/openbsd/Config.h"
 
----------------
krytarowski wrote:
> krytarowski wrote:
> > Missing in patch?
> I think that Config.h should go away. It's almost empty. But it's beyond the scope of this patch.
> 
> While there I would sort this list of includes again.
Oops yes; missed adding a directory.


================
Comment at: include/lldb/Host/HostInfo.h:55
+#elif defined(__OpenBSD__)
+#include "lldb/Host/openbsd/HostInfoOpenBSD.h"
+#define HOST_INFO_TYPE HostInfoOpenBSD
----------------
krytarowski wrote:
> I would sort includes here.
Do you want me to sort the entire list?


================
Comment at: source/Host/openbsd/Host.cpp:223
+
+#if 0
+lldb::DataBufferSP Host::GetAuxvData(lldb_private::Process *process) {
----------------
labath wrote:
> krytarowski wrote:
> > Wasn't it already gone from there?
> Yep, this should be deleted completely.
Gone.


================
Comment at: source/Plugins/Platform/OpenBSD/PlatformOpenBSD.cpp:60
+
+#if defined(__FreeBSD__) || defined(__OpenBSD__)
+    // Only accept "unknown" for the OS if the host is BSD and
----------------
krytarowski wrote:
> Please delete `__FreeBSD__` here and from the FreeBSD platform switch for `__OpenBSD__`, but I don't insist on doing the FreeBSD tweak within the same commit.
Will do.


================
Comment at: source/Plugins/Platform/OpenBSD/PlatformOpenBSD.cpp:158
+    case 1:
+      triple.setArchName("i386");
+      break;
----------------
labath wrote:
> As far as I can tell, you're only adding x86_64 support, so you should probably remove the others.
True.  I trimmed the list that FreeBSD had.  My intention is to submit support for all these architectures. But I can leave the currently unsupported ones out.


================
Comment at: source/Plugins/Process/elf-core/ProcessElfCore.cpp:588
+      }
+    } else if (strncmp(note.n_name.c_str(), "OpenBSD", 7) == 0) {
+      m_os = llvm::Triple::OpenBSD;
----------------
krytarowski wrote:
> `note.n_name == "OpenBSD"` ?
That wouldn't work. The notes describing the registers have the thread ID attached to this name, i.e. "OpenBSD at 250037".  I'll need to parse that string to make core dumps of multi-threaded processes work later.


================
Comment at: source/Plugins/Process/elf-core/ThreadElfCore.cpp:114
+
+    case llvm::Triple::OpenBSD: {
+      switch (arch.GetMachine()) {
----------------
krytarowski wrote:
> I would consider to keep it sorted - after Linux.
Will do.


================
Comment at: source/Plugins/Process/elf-core/ThreadElfCore.cpp:117
+      case llvm::Triple::aarch64:
+        reg_interface = new RegisterInfoPOSIX_arm64(arch);
+        break;
----------------
labath wrote:
> Have you checked that the arm64 case would work here? (at least whether the register context have roughly the same layout ?)
I'm actually working on making the layout the same (OpenBSD/arm64 is still under heavy development). I'll make sure it works before this diff lands ;)


Repository:
  rL LLVM

https://reviews.llvm.org/D31131