[Lldb-commits] [PATCH] D32149: Correct handling NetBSD core(5) files with threads

[Mark Kettenis via Phabricator via lldb-commits]

kettenis added a comment.

Generally looks reasonable to me.  A few comments inline.



================
Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:1374
+    else if ((note.n_name == LLDB_NT_OWNER_NETBSDCORE) &&
+             (note.n_type == LLDB_NT_NETBSD_NT_PROCINFO)) {
       // Set the elf OS version to NetBSD.  Also clear the vendor.
----------------
By making this change you risk losing recognition of "normal" NetBSD ELF files, i.e. executables and shared libraries.


================
Comment at: source/Plugins/Process/elf-core/ProcessElfCore.cpp:563
+
+/// Generic (Linux, ...) specific Thread context from PT_NOTE segment
+/// 1) A Thread Context in a core file usually described by 3 NOTE entries.
----------------
A bit strange to call something both generic and specific...

I think some of these note types originated on SVR4/Solaris, but I'm not sure how much Linux deviated from that design through the years.


================
Comment at: source/Plugins/Process/elf-core/ProcessElfCore.cpp:815
+            return Error("Error parsing NetBSD core(5) notes: Cannot convert "
+                         "LWP ID to integer");
+        } else if (note.n_type == NETBSD::AMD64::NT_FPREGS) {
----------------
Wouldn't it make sense to move the parsing of the LWP ID before the switch. Otherwise you'll have to duplicate the code for every NetBSD architecture you'll add.


Repository:
  rL LLVM

https://reviews.llvm.org/D32149