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@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
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
