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

[Joerg Sonnenberger via Phabricator via lldb-commits]

joerg added inline comments.


================
Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:55
 const char *const LLDB_NT_OWNER_NETBSD = "NetBSD";
+const char *const LLDB_NT_OWNER_NETBSDCORE = "NetBSD-CORE";
 const char *const LLDB_NT_OWNER_OPENBSD = "OpenBSD";
----------------
Not your fault, but is there a reason why this are all pointers to strings and not just const char[] ?


================
Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:302
+  // default 32 or 64 bit arch (without any architecture revision) based on
+  // object file's class.
   if (header.e_type == ET_CORE) {
----------------
Unrelated cosmetic change.


================
Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:412
+    data_sp = DataBufferLLVM::CreateSliceFromPath(file->GetPath(), length,
+                                                  file_offset);
     if (!data_sp)
----------------
Unrelated cosmetic change.


================
Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:775
+                data_sp = DataBufferLLVM::CreateSliceFromPath(file.GetPath(),
+                                                              -1, file_offset);
                 if (data_sp) {
----------------
Dito.


================
Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:1488
           llvm::StringRef path(cstr);
-          if (path.contains("/lib/x86_64-linux-gnu") || path.contains("/lib/i386-linux-gnu")) {
+          if (path.contains("/lib/x86_64-linux-gnu") ||
+              path.contains("/lib/i386-linux-gnu")) {
----------------
Unrelated.


================
Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3042
+      [this, symbol_table, section_list,
+       &new_symbols](lldb::addr_t file_addr, uint32_t size, dw_offset_t) {
+        Symbol *symbol = symbol_table->FindSymbolAtFileAddress(file_addr);
----------------
Unrelated.


================
Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3066
+                section_sp, // Section in which this symbol is defined or null.
+                offset,     // Offset in section or symbol value.
+                0,     // Size:          Don't specify the size as an FDE can
----------------
Unrelated.


================
Comment at: source/Plugins/Process/elf-core/ProcessElfCore.cpp:378
+  // Don't proceed if core file doesn't contain the actual data for this address
+  // range.
   if (file_start == file_end)
----------------
Unrelated.


================
Comment at: source/Plugins/Process/elf-core/ProcessElfCore.cpp:463
 
-enum { NT_PROCINFO = 1, NT_AUXV, NT_AMD64_REGS = 33, NT_AMD64_FPREGS = 35 };
+enum { NT_PROCINFO = 1, NT_PROCINFO_SIZE = 160, NT_AUXV = 2 };
+
----------------
Either sort them by value or by name, but not randomly


================
Comment at: source/Plugins/Process/elf-core/ProcessElfCore.cpp:523
+
+  offset += 108;
+  cpi_nlwps = data.GetU32(&offset); /* number of LWPs */
----------------
Can you define a constant for the offset here below instead of a magic number?


================
Comment at: source/Plugins/Process/elf-core/ProcessElfCore.cpp:685
           thread_data->fpregset = note_data;
-        else if(arch.IsMIPS())
+        else if (arch.IsMIPS())
           thread_data->fpregset = note_data;
----------------
Unrelated.


================
Comment at: source/Plugins/Process/elf-core/ProcessElfCore.cpp:844
+  if (m_thread_data.size() != nlwps)
+    return Error("rror parsing NetBSD core(5) notes: Mismatch between the "
+                 "number of LWPs in netbsd_elfcore_procinfo and the number of "
----------------
Typo


================
Comment at: source/Plugins/Process/elf-core/ProcessElfCore.cpp:854
+  /* Signal destinated for a particular LWP */
+  else {
+    bool passed = false;
----------------
Move the else to the } and the comment after the {


Repository:
  rL LLVM

https://reviews.llvm.org/D32149