[lldb] [Process] Add proper support for NetBSD core files with threads

[mgorny]

Summary

Improve the support for processing NetBSD cores. Fix reading process
identifier, thread information and associating the terminating signal
with the correct thread.

Includes test cases for single-threaded program receiving SIGSEGV,
and two dual-threaded programs: one where thread receives the signal,
and the other one when the whole process is signalled.

[krytarowski]

Demo:

http://netbsd.org/~kamil/lldb/firefox-core.typescript

Replay:

script -p ./firefox-core.typescript

BSD script(1) is incompatible with the GNU one, so I prepared a quick port to Linux:

http://netbsd.org/~kamil/lldb/nbscript.c

gcc nbscript.c -lutil -o nbscript

[labath]

A test would infinitely times more valuable then a demo script. What is the tiniest core file you can produce on NetBSD? (on linux we've gotten them down to about 20K) Then we could check that in and write a test for it...

[krytarowski]

In D32149#731887, @labath wrote:

A test would infinitely times more valuable then a demo script. What is the tiniest core file you can produce on NetBSD? (on linux we've gotten them down to about 20K) Then we could check that in and write a test for it...

This is something I wanted to bring to the dev mailing list.

I wanted to prepare at least three tests, if possible four:

• one thread (if possible two variations: signal to one particular thread + signal to all threads)
• multiple threads (signal to one particular thread + signal to all threads)

And this in combination of all supported targets (x86_64, i386, etc).

Emitting SIGABRT for such program gives core of size 97kilobytes:

int main(){for(;;);}

I will write assembly programs for the above cases, without libc.

[kettenis]

Generally looks reasonable to me. A few comments inline.

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

1374 ↗

(On Diff #95522)

By making this change you risk losing recognition of "normal" NetBSD ELF files, i.e. executables and shared libraries.

source/Plugins/Process/elf-core/ProcessElfCore.cpp

563 ↗

(On Diff #95522)

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.

815 ↗

(On Diff #95522)

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.

[kettenis]

In D32149#731887, @labath wrote:

A test would infinitely times more valuable then a demo script. What is the tiniest core file you can produce on NetBSD? (on linux we've gotten them down to about 20K) Then we could check that in and write a test for it...

The smalles core dumps I managed to create for OpenBSD so far are a bit over 4M.

[krytarowski]

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

1374 ↗

(On Diff #95522)

Hmm, do you mean these notes:

$ readelf -n /bin/cat                                                                                                                                 

Displaying notes found at file offset 0x00000214 with length 0x00000018:
  Owner                 Data size	Description
  NetBSD		0x00000004	IDENT 799005900 (7.99.59)

Displaying notes found at file offset 0x0000022c with length 0x00000014:
  Owner                 Data size	Description
  NetBSD		0x00000004	PaX <>

source/Plugins/Process/elf-core/ProcessElfCore.cpp

563 ↗

(On Diff #95522)

Can we call it: Type1 core (as of now: NetBSD) and Type2 core (Linux, Android, FreeBSD, OpenBSD)?

I don't care which would be 1 and which 2.

815 ↗

(On Diff #95522)

Sounds good.

[kettenis]

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

1374 ↗

(On Diff #95522)

In particular the first one of these. On OpenBSD I just have a generic check on the name which catches both these and the core file notes. But on NetBSD those use different names.

source/Plugins/Process/elf-core/ProcessElfCore.cpp

563 ↗

(On Diff #95522)

I'd just drop word "specific" from the comment. And OpenBSD will obviously be handled in a similar way as NetBSD in the near future.

[krytarowski]

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

1374 ↗

(On Diff #95522)

I will test it and add switch to support both types of ELF files: core(5) and executables.

source/Plugins/Process/elf-core/ProcessElfCore.cpp

563 ↗

(On Diff #95522)

Assuming that NetBSD and OpenBSD will share the same function - we can go pickup appropriate name now.

As far as I can tell FreeBSD shares similar concept to Linux.

A thread specific data is defined by multiplication of the following notes:

FreeBSD       0x000000e0      NT_PRSTATUS (prstatus structure)
FreeBSD       0x00000200      NT_FPREGSET (floating point registers)
FreeBSD       0x00000018      NT_THRMISC (thrmisc structure)

[labath]

In D32149#731920, @krytarowski wrote:

In D32149#731887, @labath wrote:

A test would infinitely times more valuable then a demo script. What is the tiniest core file you can produce on NetBSD? (on linux we've gotten them down to about 20K) Then we could check that in and write a test for it...

This is something I wanted to bring to the dev mailing list.

I wanted to prepare at least three tests, if possible four:

• one thread (if possible two variations: signal to one particular thread + signal to all threads)
• multiple threads (signal to one particular thread + signal to all threads)

And this in combination of all supported targets (x86_64, i386, etc).

Emitting SIGABRT for such program gives core of size 97kilobytes:

int main(){for(;;);}

I will write assembly programs for the above cases, without libc.

Cool, I am looking forward to the results.

source/Plugins/Process/elf-core/ProcessElfCore.cpp

795 ↗

(On Diff #95522)

How about StringRef name = note.n_name;

803 ↗

(On Diff #95522)

Then this can be

else if (name.consume_front("NetBSD-CORE@")) {
  ...
  if (name.getAsInteger(0, tid))
    error...

848 ↗

(On Diff #95522)

for (auto &data: m_thread_data) data.signo = signo seems shorter, more understandable, and consistent with other usages in this file.

856 ↗

(On Diff #95522)

This could also be a range-based for.

[krytarowski]

Fix handling executable and shared library triple detection.

Apply changes from review.

Update revision summary.

[krytarowski]

ping?

[joerg]

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

55 ↗

(On Diff #96272)

Not your fault, but is there a reason why this are all pointers to strings and not just const char[] ?

302 ↗

(On Diff #96272)

Unrelated cosmetic change.

412 ↗

(On Diff #96272)

Unrelated cosmetic change.

775 ↗

(On Diff #96272)

Dito.

1488 ↗

(On Diff #96272)

Unrelated.

3042 ↗

(On Diff #96272)

Unrelated.

3066 ↗

(On Diff #96272)

Unrelated.

source/Plugins/Process/elf-core/ProcessElfCore.cpp

378 ↗

(On Diff #96272)

Unrelated.

463 ↗

(On Diff #96272)

Either sort them by value or by name, but not randomly

523 ↗

(On Diff #96272)

Can you define a constant for the offset here below instead of a magic number?

685 ↗

(On Diff #96272)

Unrelated.

844 ↗

(On Diff #96272)

Typo

854 ↗

(On Diff #96272)

Move the else to the } and the comment after the {

[krytarowski]

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

302 ↗

(On Diff #96272)

I let clang-format to go and alter minor things. I can run clang-format over original files - commit, and add my diff again.

source/Plugins/Process/elf-core/ProcessElfCore.cpp

463 ↗

(On Diff #96272)

I will split this enum{} into two enums.

523 ↗

(On Diff #96272)

I will try to get something to define aliases for these magic numbers.

844 ↗

(On Diff #96272)

OK

854 ↗

(On Diff #96272)

OK

[labath]

In D32149#738250, @krytarowski wrote:

ping?

Sorry, I wasn't responding partially because I was waiting to see how the discussion on https://reviews.llvm.org/D32434 settles, as I think it may have effect on the test strategy. I'll write more tomorrow. :)

[krytarowski]

In D32149#739727, @labath wrote:

In D32149#738250, @krytarowski wrote:

ping?

Sorry, I wasn't responding partially because I was waiting to see how the discussion on https://reviews.llvm.org/D32434 settles, as I think it may have effect on the test strategy. I'll write more tomorrow. :)

Thanks for working on it. I will upload soon changes to address noted by Joerg.

[krytarowski]

Strip non-functional style improvements in unrelated code-parts.

Apply changes to address comments from Joerg.

[krytarowski]

Remove another unrelated style improvement.

[krytarowski]

Rebase to HEAD.
Apply "Error" -> "Status" rename.

[krytarowski]

Can we go with this change?

[labath]

What was your decision on the core files? I was under the impression you were gonna add the zip files as well. If so, then they should go in at the same time.

[krytarowski]

In D32149#763008, @labath wrote:

What was your decision on the core files? I was under the impression you were gonna add the zip files as well. If so, then they should go in at the same time.

In the same commit? If so I will try to amend the patch.

[labath]

Affirmative. Tests should go in together with the feature they are testing.

[krytarowski]

Suspended, I need to resurrect tracing of 1 thread and fix kernel bugs for multiple threads first.

[mgorny]

I've attempted to rebase Kamil's patch against the current sources. Please note that this is not the final version; I'll add a test case and PID reading later.

[mgorny]

Updated to read PID and make unknown notes non-fatal.

[mgorny]

Added two tests, more pending. Sadly, I wasn't able to reuse even the test cases since -nostdlib doesn't really work on NetBSD (resulting executables are not recognized as valid executables).

[mgorny]

I've added one more test case, and added stop reason/signal checking for both threads. I think it's ready for review now.

[krytarowski]

Looks good to me.

[labath]

The code looks good, though I think we should really start figuring out how to write elf core tests without checking in binaries.

lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp

static void ParseFreeBSDPrStatus(ThreadData &thread_data,
   thread_data.tid = data.GetU32(&offset);   // pr_pid
   if (lp64)
     offset += 4;
 
   size_t len = data.GetByteSize() - offset;
   thread_data.gpregset = DataExtractor(data, offset, len);
 }
 
-static void ParseNetBSDProcInfo(ThreadData &thread_data,
-                                const DataExtractor &data) {
+static llvm::Error ParseNetBSDProcInfo(const DataExtractor &data,
+                                       uint32_t &cpi_nlwps,
+                                       uint32_t &cpi_signo,
+                                       uint32_t &cpi_siglwp) {
   lldb::offset_t offset = 0;
 
-  int version = data.GetU32(&offset);
+  uint32_t version = data.GetU32(&offset);
   if (version != 1)
-    return;
+    return llvm::make_error<llvm::StringError>(
+        "Error parsing NetBSD core(5) notes: Unsupported procinfo version",
+        llvm::inconvertibleErrorCode());
 
-  offset += 4;
-  thread_data.signo = data.GetU32(&offset);
+  uint32_t cpisize = data.GetU32(&offset);
+  if (cpisize != NETBSD::NT_PROCINFO_SIZE)
+    return llvm::make_error<llvm::StringError>(
+        "Error parsing NetBSD core(5) notes: Unsupported procinfo size",
+        llvm::inconvertibleErrorCode());
+
+  cpi_signo = data.GetU32(&offset); /* killing signal */
+
+  offset += NETBSD::NT_PROCINFO_CPI_SIGCODE_SIZE;
+  offset += NETBSD::NT_PROCINFO_CPI_SIGPEND_SIZE;
+  offset += NETBSD::NT_PROCINFO_CPI_SIGMASK_SIZE;
+  offset += NETBSD::NT_PROCINFO_CPI_SIGIGNORE_SIZE;
+  offset += NETBSD::NT_PROCINFO_CPI_SIGCATCH_SIZE;
+  offset += NETBSD::NT_PROCINFO_CPI_PID_SIZE;
+  offset += NETBSD::NT_PROCINFO_CPI_PPID_SIZE;
+  offset += NETBSD::NT_PROCINFO_CPI_PGRP_SIZE;
+  offset += NETBSD::NT_PROCINFO_CPI_SID_SIZE;
+  offset += NETBSD::NT_PROCINFO_CPI_RUID_SIZE;
+  offset += NETBSD::NT_PROCINFO_CPI_EUID_SIZE;
+  offset += NETBSD::NT_PROCINFO_CPI_SVUID_SIZE;
+  offset += NETBSD::NT_PROCINFO_CPI_RGID_SIZE;
+  offset += NETBSD::NT_PROCINFO_CPI_EGID_SIZE;
+  offset += NETBSD::NT_PROCINFO_CPI_SVGID_SIZE;
+  cpi_nlwps = data.GetU32(&offset); /* number of LWPs */
+
+  offset += NETBSD::NT_PROCINFO_CPI_NAME_SIZE;
+  cpi_siglwp = data.GetU32(&offset); /* LWP target of killing signal */
+
+  return llvm::Error::success();
 }
 
 static void ParseOpenBSDProcInfo(ThreadData &thread_data,
                                  const DataExtractor &data) {
   lldb::offset_t offset = 0;
 
   int version = data.GetU32(&offset);
   if (version != 1)
if (!have_prstatus) {
     return llvm::make_error<llvm::StringError>(
         "Could not find NT_PRSTATUS note in core file.",
         llvm::inconvertibleErrorCode());
   }
   m_thread_data.push_back(thread_data);
   return llvm::Error::success();
 }
 
+/// NetBSD specific Thread context from PT_NOTE segment
+///
+/// NetBSD ELF core files use notes to provide information about
+/// the process's state.  The note name is "NetBSD-CORE" for
+/// information that is global to the process, and "NetBSD-CORE@nn",
+/// where "nn" is the lwpid of the LWP that the information belongs
+/// to (such as register state).
+///
+/// NetBSD uses the following note identifiers:
+///
+///      ELF_NOTE_NETBSD_CORE_PROCINFO (value 1)
+///             Note is a "netbsd_elfcore_procinfo" structure.
+///      ELF_NOTE_NETBSD_CORE_AUXV     (value 2; since NetBSD 8.0)
+///             Note is an array of AuxInfo structures.
+///
+/// NetBSD also uses ptrace(2) request numbers (the ones that exist in
+/// machine-dependent space) to identify register info notes.  The
+/// info in such notes is in the same format that ptrace(2) would
+/// export that information.
+///
+/// For more information see /usr/include/sys/exec_elf.h
+///
 llvm::Error ProcessElfCore::parseNetBSDNotes(llvm::ArrayRef<CoreNote> notes) {
   ThreadData thread_data;
+  bool had_nt_regs = false;
+
+  // To be extracted from struct netbsd_elfcore_procinfo
+  // Used to sanity check of the LWPs of the process
+  uint32_t nlwps = 0;
+  uint32_t signo;  // killing signal
+  uint32_t siglwp; // LWP target of killing signal
+
   for (const auto &note : notes) {
-    // NetBSD per-thread information is stored in notes named "NetBSD-CORE@nnn"
-    // so match on the initial part of the string.
-    if (!llvm::StringRef(note.info.n_name).startswith("NetBSD-CORE"))
-      continue;
+    llvm::StringRef name = note.info.n_name;
 
-    switch (note.info.n_type) {
-    case NETBSD::NT_PROCINFO:
-      ParseNetBSDProcInfo(thread_data, note.data);
-      break;
-    case NETBSD::NT_AUXV:
+    if (name == "NetBSD-CORE") {
+      if (note.info.n_type == NETBSD::NT_PROCINFO) {
+        llvm::Error error = ParseNetBSDProcInfo(note.data, nlwps, signo, siglwp);
+        if (error)
+          return error;
+      } else if (note.info.n_type == NETBSD::NT_AUXV) {
         m_auxv = note.data;
-      break;
+      }
+    } else if (name.consume_front("NetBSD-CORE@")) {
+      lldb::tid_t tid;
+      if (name.getAsInteger(10, tid))
+        return llvm::make_error<llvm::StringError>(
+            "Error parsing NetBSD core(5) notes: Cannot convert LWP ID "
+            "to integer",
+            llvm::inconvertibleErrorCode());
+
+      switch (GetArchitecture().GetMachine()) {
+      case llvm::Triple::x86_64: {
+        // Assume order PT_GETREGS, PT_GETFPREGS
+        if (note.info.n_type == NETBSD::AMD64::NT_REGS) {
+          // If this is the next thread, push the previous one first.
+          if (had_nt_regs) {
+            m_thread_data.push_back(thread_data);
+            thread_data = ThreadData();
+            had_nt_regs = false;
+          }
 
-    case NETBSD::NT_AMD64_REGS:
-      if (GetArchitecture().GetMachine() == llvm::Triple::x86_64)
           thread_data.gpregset = note.data;
-      break;
-    default:
+          thread_data.tid = tid;
+          if (thread_data.gpregset.GetByteSize() == 0)
+            return llvm::make_error<llvm::StringError>(
+                "Could not find general purpose registers note in core file.",
+                llvm::inconvertibleErrorCode());
+          had_nt_regs = true;
+        } else if (note.info.n_type == NETBSD::AMD64::NT_FPREGS) {
+          if (!had_nt_regs || tid != thread_data.tid)
+            return llvm::make_error<llvm::StringError>(
+                "Error parsing NetBSD core(5) notes: Unexpected order "
+                "of NOTEs PT_GETFPREG before PT_GETREG",
+                llvm::inconvertibleErrorCode());
           thread_data.notes.push_back(note);
-      break;
+        } else {
+          return llvm::make_error<llvm::StringError>(
+              "Error parsing NetBSD core(5) notes: Unsupported AMD64 NOTE",
+              llvm::inconvertibleErrorCode());
         }
+      } break;
+      default:
+        return llvm::make_error<llvm::StringError>(
+            "Error parsing NetBSD core(5) notes: Unsupported architecture",
+            llvm::inconvertibleErrorCode());
       }
-  if (thread_data.gpregset.GetByteSize() == 0) {
+    } else {
       return llvm::make_error<llvm::StringError>(
-        "Could not find general purpose registers note in core file.",
+          "Error parsing NetBSD core(5) notes: Unrecognized note",
           llvm::inconvertibleErrorCode());
     }
+  }
+
+  // Push the last thread.
+  if (had_nt_regs)
     m_thread_data.push_back(thread_data);
+
+  if (m_thread_data.empty())
+    return llvm::make_error<llvm::StringError>(
+        "Error parsing NetBSD core(5) notes: No threads information "
+        "specified in notes",
+        llvm::inconvertibleErrorCode());
+
+  if (m_thread_data.size() != nlwps)
+    return llvm::make_error<llvm::StringError>(
+        "Error parsing NetBSD core(5) notes: Mismatch between the number "
+        "of LWPs in netbsd_elfcore_procinfo and the number of LWPs specified "
+        "by MD notes",
+        llvm::inconvertibleErrorCode());
+
+  // Signal targeted at the whole process.
+  if (siglwp == 0) {
+    for (auto &data : m_thread_data)
+      data.signo = signo;
+  }
+  // Signal destined for a particular LWP.
+  else {
+    bool passed = false;
+
+    for (auto &data : m_thread_data) {
+      if (data.tid == siglwp) {
+        data.signo = signo;
+        passed = true;
+        break;
+      }
+    }
+
+    if (!passed)
+      return llvm::make_error<llvm::StringError>(
+          "Error parsing NetBSD core(5) notes: Signal passed to unknown LWP",
+          llvm::inconvertibleErrorCode());
+  }
+
   return llvm::Error::success();
 }
 
 llvm::Error ProcessElfCore::parseOpenBSDNotes(llvm::ArrayRef<CoreNote> notes) {
   ThreadData thread_data;
   for (const auto &note : notes) {
     // OpenBSD per-thread information is stored in notes named "OpenBSD@nnn" so
     // match on the initial part of the string.

lldb/source/Plugins/Process/elf-core/RegisterUtilities.h

enum {
   NT_PRPSINFO,
   NT_THRMISC = 7,
   NT_PROCSTAT_AUXV = 16,
   NT_PPC_VMX = 0x100
 };
 }
 
 namespace NETBSD {
-enum { NT_PROCINFO = 1, NT_AUXV, NT_AMD64_REGS = 33, NT_AMD64_FPREGS = 35 };
+enum { NT_PROCINFO = 1, NT_AUXV = 2 };
+
+/* Size in bytes */
+enum { NT_PROCINFO_SIZE = 160 };
+
+/* Size in bytes */
+enum {
+  NT_PROCINFO_CPI_VERSION_SIZE = 4,
+  NT_PROCINFO_CPI_CPISIZE_SIZE = 4,
+  NT_PROCINFO_CPI_SIGNO_SIZE = 4,
+  NT_PROCINFO_CPI_SIGCODE_SIZE = 4,
+  NT_PROCINFO_CPI_SIGPEND_SIZE = 16,
+  NT_PROCINFO_CPI_SIGMASK_SIZE = 16,
+  NT_PROCINFO_CPI_SIGIGNORE_SIZE = 16,
+  NT_PROCINFO_CPI_SIGCATCH_SIZE = 16,
+  NT_PROCINFO_CPI_PID_SIZE = 4,
+  NT_PROCINFO_CPI_PPID_SIZE = 4,
+  NT_PROCINFO_CPI_PGRP_SIZE = 4,
+  NT_PROCINFO_CPI_SID_SIZE = 4,
+  NT_PROCINFO_CPI_RUID_SIZE = 4,
+  NT_PROCINFO_CPI_EUID_SIZE = 4,
+  NT_PROCINFO_CPI_SVUID_SIZE = 4,
+  NT_PROCINFO_CPI_RGID_SIZE = 4,
+  NT_PROCINFO_CPI_EGID_SIZE = 4,
+  NT_PROCINFO_CPI_SVGID_SIZE = 4,
+  NT_PROCINFO_CPI_NLWPS_SIZE = 4,
+  NT_PROCINFO_CPI_NAME_SIZE = 32,
+  NT_PROCINFO_CPI_SIGLWP_SIZE = 4,
+};
+
+namespace AMD64 {
+enum { NT_REGS = 33, NT_FPREGS = 35 };
 }
 
+} // namespace NETBSD
+
 namespace OPENBSD {
 enum {
   NT_PROCINFO = 10,
   NT_AUXV = 11,
   NT_REGS = 20,
   NT_FPREGS = 21,
 };
 }

 
 constexpr RegsetDesc FPR_Desc[] = {
     {llvm::Triple::FreeBSD, llvm::Triple::UnknownArch, FREEBSD::NT_FPREGSET},
     // In a i386 core file NT_FPREGSET is present, but it's not the result
     // of the FXSAVE instruction like in 64 bit files.
     // The result from FXSAVE is in NT_PRXFPREG for i386 core files
     {llvm::Triple::Linux, llvm::Triple::x86, LINUX::NT_PRXFPREG},
     {llvm::Triple::Linux, llvm::Triple::UnknownArch, LINUX::NT_FPREGSET},
-    {llvm::Triple::NetBSD, llvm::Triple::x86_64, NETBSD::NT_AMD64_FPREGS},
+    {llvm::Triple::NetBSD, llvm::Triple::x86_64, NETBSD::AMD64::NT_FPREGS},
     {llvm::Triple::OpenBSD, llvm::Triple::UnknownArch, OPENBSD::NT_FPREGS},
 };
 
 constexpr RegsetDesc PPC_VMX_Desc[] = {
     {llvm::Triple::FreeBSD, llvm::Triple::UnknownArch, FREEBSD::NT_PPC_VMX},
     {llvm::Triple::Linux, llvm::Triple::UnknownArch, LINUX::NT_PPC_VMX},
 };
 
 constexpr RegsetDesc PPC_VSX_Desc[] = {
     {llvm::Triple::Linux, llvm::Triple::UnknownArch, LINUX::NT_PPC_VSX},
 };
 
 } // namespace lldb_private
 
 #endif // #ifndef LLDB_REGISTERUTILITIES_H