[lldb] Add a "thread extrainfo" LC_NOTE for Mach-O corefiles, to store the thread IDs of the threads

[jasonmolenda]

Summary

This adds a new LC_NOTE to ObjectFileMachO to represent additional metadata about threads in the corefile. Mach-O corefiles have each thread represented by an LC_THREAD which can only contain an array of register context "flavors" (pre-defined register sets by the kernel). This LC_NOTE has a JSON dictionary with metadata to add to each thread.

struct thread_extrainfo {
   char json_dictionary[];  // must be nul '\0' terminated c-string
};

The JSON dictionary must have a threads key, and the value is an array. The array must have the same number of elements as there are LC_THREADs in the corefile, even if the entries are empty. Initially, only the thread_id key is supported. If a thread's dictionary does not specify a thread_id, lldb will make up a value for it.

An example:

{"threads":[{"thread_id":18427013},{"thread_id":18427037},{"thread_id":18427038}]}

which then looks like

(lldb) thr li
Process 0 stopped
  thread #1: tid = 0x1192c85, 0x0000000185b19ea8 libsystem_kernel.dylib`__semwait_signal + 8
* thread #2: tid = 0x1192c9d, 0x0000000100000d6c a.out`crash_worker(in=0x0000000000000000) at main.cpp:15:18, stop reason = ESR_EC_DABORT_EL0 (fault address: 0x0)
  thread #3: tid = 0x1192c9e, 0x0000000185b19ea8 libsystem_kernel.dylib`__semwait_signal + 8

in lldb.

This patch adds support to ObjectFileMachO's process save-core to emit this metadata, and support to ObjectFileMachO, ProcessMachCore, ThreadMachCore to read & use those thread IDs when provided.

I expect to be adding additional thread attributes in the near future -- name, queue name, maybe a stop reason? -- they will be simple additions this format, but I'm hoping to get feedback on the format itself now. This is the first time we've had an LC_NOTE that did not use a strict binary representation with required format - by putting JSON in there, we do not have strict definitions of what is allowed to be included in the dictionary, but I think there are enough different use cases that this is the right choice for this one.

[jasonmolenda]

Or possibly we should just make "thread extrainfo" a JSON blob with a dictionary for each thread, which would make it easier to extend, instead of a binary format. I'm not sure I have a strong opinion.

[jasonmolenda]

As soon as I started thinking about thread names and queue names and mach exception data, all variable length things, a binary format doesn't seem ideal. We do already send a lot of thread information about processes in JSON in gdb remote serial protocol from debugserver today. It's the first time we'd use JSON in a Mach-O LC_NOTE though, so I'm interested to hear other people's thoughts.

[jasonmolenda]

After thinking about this more, and talking with @jingham I rewrote the patch so LC_NOTE "thread extrainfo" is a JSON dictionary with a key threads that has an array. The number of entries in the array must match the number of LC_THREADs in the Mach-O corefile. Each array entry may have a thread_id key with a thread id for that LC_THREAD, or lldb will create a thread_id for it. I expect we will add more per-thread keys in the future.

The payload looks like

{"threads":[{"thread_id":18368681},{"thread_id":18368703},{"thread_id":18368704}]}

and JSON is stored as a c-string, requires a nul byte '\0' at the end.

I've treated the LC_NOTEs in Mach-O as strictly defined binary data until now, so producers and consumers both had a reference to work with. But I think in the case of per-thread information in a corefile, we will have different people wanting to add different fields -- some fixed width, some variable length -- and specifying a JSON format that can be extended more easily is probably the correct way to go here.

[jasonmolenda]

One interesting question about the basic design of this LC_NOTE. If I call it "thread extrainfo" and it is a JSON dictionary with a threads array -- how long before someone wants to stick some non-thread specific piece of data in here. Should it be a proc metadata with a threads array, and it doesn't become confusing as it is extended over years.

[bulbazord]

The idea itself looks good to me. It would be nice if we could maintain a schema for this format in documentation or elsewhere so future code spelunkers have an idea of what the parsing and saving code are doing while they read it.

lldb/test/API/macosx/corefile-exception-reason/TestCorefileExceptionReason.py

54–57

nit

[JDevlieghere]

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

5682–5693

There's a *lot* of duplication between reading LC_NOTEs. At some point we should extract the common code instead of copy-pasting it every time.

5723–5724

StructuredData::Array::GetSize returns a size_t. No point in turning this into a uint32_t.

lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp

603

Capitalize + period.

608–619

This code is (presumably on purpose) trying to find gaps in the available tids: for example if I had [1, 2, ? ,4, ?, 6] this is going to turn that into [1, 2, (3), 4, (5), 6]. If that property matters, it probably should be part of the comment. If it doesn't, you could exploit the fact that the set is sorted and just take the last value (6 for the previous example) and return [1, 2, 4, 6, 7, 8], but again, I assume that's what you're trying to avoid here.

628–629

make_shared (avoid "naked new")

[jasonmolenda]

Thanks for the feedback @bulbazord & @JDevlieghere , I will address those points.

I am still trying to decide on the scope of this JSON LC_NOTE. Right now it's "thread extrainfo" and a dictionary with a "threads" key by definition. But I have a feeling it should be "process metadata", and currently it may contain a "threads" key which must be an array with the same number of elements as corefile has LC_THREADs, for future extension use.

[jasonmolenda]

Thanks for the feedback Alex & Jonas. Jonas' comment that we have a lot of different methods manually stepping over load commands to find their LC_NOTEs was something I'd been meaning to deal with some day, but hadn't done yet. I shouldn't complicate this patch by also doing that but ... I did. Updating the patch in a minute.

lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp

608–619

I wasn't trying to maintain the order of the created tids, you're right I shouldn't bother being so fancy, just grab the highest tid number seen and increment past it. I create the set by iterating over the vector, I'll just watch for the highest tid_t value in that loop instead.

[jasonmolenda]

Address feedback from Alex and Jonas. Most significantly, add a ObjectFileMachO::FindLC_NOTEByName method which all the LC_NOTE readers in ObjectFileMachO call. One of them specifically needs to iterate over multiple LC_NOTEs with the same name ("load binary"), so it returns a vector of offset & sizes of matching LC_NOTEs which looks a little unusual for the "there will only be one of these" LC_NOTEs. I also simplified the tid_t creation for missing tid_t values in ProcessMachCore when only some threads were given specified tid_t values.

[jasonmolenda]

Oh, and I did change the LC_NOTE name that I'm adding to "process metadata" and specified that it *may* contain a threads key, instead of *shall* contain a threads key.

[jasonmolenda]

All of the feedback is addressed at this point. I had one decision I didn't like - the LC_NOTE has an explicit size in the load command, but I said that the JSON string needed to be nul byte ('\0') terminated, which is redundant. I've changed the implementation to (1) in the producer side, don't add a nul byte at the end of the string, and (2) in the consumer side, add the nul byte to the read buffer, before treating it as a c-string. To help emphasize that the size of the LC_NOTE is sufficient.

[JDevlieghere]

I really dislike all the manual memory management that's going on in this file, but I realize that's not the result of this patch, but rather because of the code that was factored out. I'd love if someone would replace the mallo'c buffers with SmallString<0>s but that's orthogonal to this patch. LGTM.

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

73

I don't think this is needed (anymore)?

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h

124

Thanks for factoring this out!

[jasonmolenda]

Update patch to not use malloc/free for a temporary heap allocation; a std::string works fine. One caveat is that the c-strings we read in may have a nul byte terminator, and left alone the std::string will consider that part of the string; that can be preserved when using a string_view etc representation.

lldb/include/lldb/Symbol/ObjectFile.h

virtual bool GetCorefileMainBinaryInfo(lldb::addr_t &value,
                                          bool &value_is_offset, UUID &uuid,
                                          ObjectFile::BinaryType &type) {
     value = LLDB_INVALID_ADDRESS;
     value_is_offset = false;
     uuid.Clear();
     return false;
   }
 
+  /// Get metadata about threads from the corefile.
+  ///
+  /// The corefile may have metadata (e.g. a Mach-O "thread extrainfo"
+  /// LC_NOTE) which for the threads in the process; this method tries
+  /// to retrieve them.
+  ///
+  /// \param[out] tids
+  ///     Filled in with a vector of tid_t's that matches the number
+  ///     of threads in the corefile (ObjectFile::GetNumThreadContexts).
+  ///     If a tid is not specified for one of the corefile threads,
+  ///     that entry in the vector will have LLDB_INVALID_THREAD_ID and
+  ///     the caller should assign a tid to the thread that does not
+  ///     conflict with the ones provided in this array.
+  ///     As additional metadata are added, this method may return a
+  ///     \a tids vector with no thread id's specified at all; the
+  ///     corefile may only specify one of the other metadata.
+  ///
+  /// \return
+  ///     Returns true if thread metadata was found in this corefile.
+  ///
+  virtual bool GetCorefileThreadExtraInfos(std::vector<lldb::tid_t> &tids) {
+    return false;
+  }
+
   virtual lldb::RegisterContextSP
   GetThreadContextAtIndex(uint32_t idx, lldb_private::Thread &thread) {
     return lldb::RegisterContextSP();
   }
 
   /// The object file should be able to calculate its type by looking at its
   /// file header and possibly the sections or other data in the object file.
   /// The file type is used in the debugger to help select the correct plug-

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h

public:
   }
 
   lldb_private::Address GetEntryPointAddress() override;
 
   lldb_private::Address GetBaseAddress() override;
 
   uint32_t GetNumThreadContexts() override;
 
+  std::vector<std::tuple<lldb::offset_t, lldb::offset_t>>
+  FindLC_NOTEByName(std::string name);
+
   std::string GetIdentifierString() override;
 
   lldb_private::AddressableBits GetAddressableBits() override;
 
   bool GetCorefileMainBinaryInfo(lldb::addr_t &value, bool &value_is_offset,
                                  lldb_private::UUID &uuid,
                                  ObjectFile::BinaryType &type) override;
 
+  bool GetCorefileThreadExtraInfos(std::vector<lldb::tid_t> &tids) override;
+
   bool LoadCoreFileImages(lldb_private::Process &process) override;
 
   lldb::RegisterContextSP
   GetThreadContextAtIndex(uint32_t idx, lldb_private::Thread &thread) override;
 
   ObjectFile::Type CalculateType() override;
 
   ObjectFile::Strata CalculateStrata() override;

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

 #include "lldb/Utility/AppleUuidCompatibility.h"
 #else
 #include <uuid/uuid.h>
 #endif
 
 #include <bitset>
 #include <memory>
 #include <optional>
 
 // Unfortunately the signpost header pulls in the system MachO header, too.
 #ifdef CPU_TYPE_ARM
 #undef CPU_TYPE_ARM
 #endif
 #ifdef CPU_TYPE_ARM64
 #undef CPU_TYPE_ARM64
 #endif
 #ifdef CPU_TYPE_ARM64_32
if (!m_thread_context_offsets_valid) {
         }
         offset = cmd_offset + thread_cmd.cmdsize;
       }
     }
   }
   return m_thread_context_offsets.GetSize();
 }
 
-std::string ObjectFileMachO::GetIdentifierString() {
-  std::string result;
-  Log *log(
-      GetLog(LLDBLog::Symbols | LLDBLog::Process | LLDBLog::DynamicLoader));
+std::vector<std::tuple<offset_t, offset_t>>
+ObjectFileMachO::FindLC_NOTEByName(std::string name) {
+  std::vector<std::tuple<offset_t, offset_t>> results;
   ModuleSP module_sp(GetModule());
   if (module_sp) {
     std::lock_guard<std::recursive_mutex> guard(module_sp->GetMutex());
 
-    // First, look over the load commands for an LC_NOTE load command with
-    // data_owner string "kern ver str" & use that if found.
-    lldb::offset_t offset = MachHeaderSizeFromMagic(m_header.magic);
+    offset_t offset = MachHeaderSizeFromMagic(m_header.magic);
     for (uint32_t i = 0; i < m_header.ncmds; ++i) {
       const uint32_t cmd_offset = offset;
       llvm::MachO::load_command lc = {};
       if (m_data.GetU32(&offset, &lc.cmd, 2) == nullptr)
         break;
       if (lc.cmd == LC_NOTE) {
         char data_owner[17];
         m_data.CopyData(offset, 16, data_owner);
         data_owner[16] = '\0';
         offset += 16;
-        uint64_t fileoff = m_data.GetU64_unchecked(&offset);
-        uint64_t size = m_data.GetU64_unchecked(&offset);
 
-        // "kern ver str" has a uint32_t version and then a nul terminated
-        // c-string.
-        if (strcmp("kern ver str", data_owner) == 0) {
-          offset = fileoff;
-          uint32_t version;
-          if (m_data.GetU32(&offset, &version, 1) != nullptr) {
-            if (version == 1) {
-              uint32_t strsize = size - sizeof(uint32_t);
-              char *buf = (char *)malloc(strsize);
-              if (buf) {
-                m_data.CopyData(offset, strsize, buf);
-                buf[strsize - 1] = '\0';
-                result = buf;
-                if (buf)
-                  free(buf);
-                LLDB_LOGF(log, "LC_NOTE 'kern ver str' found with text '%s'",
-                          result.c_str());
-                return result;
-              }
-            }
-          }
-        }
-      }
-      offset = cmd_offset + lc.cmdsize;
-    }
+        if (name == data_owner) {
+          offset_t payload_offset = m_data.GetU64_unchecked(&offset);
+          offset_t payload_size = m_data.GetU64_unchecked(&offset);
+          results.push_back({payload_offset, payload_size});
+        }
+      }
+      offset = cmd_offset + lc.cmdsize;
+    }
+  }
+  return results;
+}
+
+std::string ObjectFileMachO::GetIdentifierString() {
+  Log *log(
+      GetLog(LLDBLog::Symbols | LLDBLog::Process | LLDBLog::DynamicLoader));
+  ModuleSP module_sp(GetModule());
+  if (module_sp) {
+    std::lock_guard<std::recursive_mutex> guard(module_sp->GetMutex());
+
+    auto lc_notes = FindLC_NOTEByName("kern ver str");
+    for (auto lc_note : lc_notes) {
+      offset_t payload_offset = std::get<0>(lc_note);
+      offset_t payload_size = std::get<1>(lc_note);
+      if (m_data.GetU32(&payload_offset, &version, 1) != nullptr) {
+          uint32_t strsize = payload_size - sizeof(uint32_t);
+          std::string result(strsize, '\0');
+          m_data.CopyData(payload_offset, strsize, result.data());
+          while (result.back() == '\0')
+            result.resize(result.size() - 1);
 
     // Second, make a pass over the load commands looking for an obsolete
     // LC_IDENT load command.
-    offset = MachHeaderSizeFromMagic(m_header.magic);
+    offset_t offset = MachHeaderSizeFromMagic(m_header.magic);
     for (uint32_t i = 0; i < m_header.ncmds; ++i) {
       const uint32_t cmd_offset = offset;
       llvm::MachO::ident_command ident_command;
       if (m_data.GetU32(&offset, &ident_command, 2) == nullptr)
         break;
       if (ident_command.cmd == LC_IDENT && ident_command.cmdsize != 0) {
-        char *buf = (char *)malloc(ident_command.cmdsize);
-        if (buf != nullptr && m_data.CopyData(offset, ident_command.cmdsize,
-                                              buf) == ident_command.cmdsize) {
-          buf[ident_command.cmdsize - 1] = '\0';
-          result = buf;
+        std::string result(ident_command.cmdsize, '\0');
+        if (m_data.CopyData(offset, ident_command.cmdsize, result.data()) ==
+            ident_command.cmdsize) {
+          while (result.back() == '\0')
+            result.resize(result.size() - 1);
           LLDB_LOGF(log, "LC_IDENT found with text '%s'", result.c_str());
+          return result;
         }
-        if (buf)
-          free(buf);
       }
       offset = cmd_offset + ident_command.cmdsize;
     }
   }
-  return result;
+  return {};
 }
 
 AddressableBits ObjectFileMachO::GetAddressableBits() {
   AddressableBits addressable_bits;
 
   Log *log(GetLog(LLDBLog::Process));
   ModuleSP module_sp(GetModule());
   if (module_sp) {
     std::lock_guard<std::recursive_mutex> guard(module_sp->GetMutex());
-    lldb::offset_t offset = MachHeaderSizeFromMagic(m_header.magic);
-    for (uint32_t i = 0; i < m_header.ncmds; ++i) {
-      const uint32_t cmd_offset = offset;
-      llvm::MachO::load_command lc = {};
-      if (m_data.GetU32(&offset, &lc.cmd, 2) == nullptr)
-        break;
-      if (lc.cmd == LC_NOTE) {
-        char data_owner[17];
-        m_data.CopyData(offset, 16, data_owner);
-        data_owner[16] = '\0';
-        offset += 16;
-        uint64_t fileoff = m_data.GetU64_unchecked(&offset);
-
-        // "addrable bits" has a uint32_t version and a uint32_t
-        // number of bits used in addressing.
-        if (strcmp("addrable bits", data_owner) == 0) {
-          offset = fileoff;
-          uint32_t version;
-          if (m_data.GetU32(&offset, &version, 1) != nullptr) {
-            if (version == 3) {
-              uint32_t num_addr_bits = m_data.GetU32_unchecked(&offset);
-              addressable_bits.SetAddressableBits(num_addr_bits);
-              LLDB_LOGF(log,
-                        "LC_NOTE 'addrable bits' v3 found, value %d "
-                        "bits",
-                        num_addr_bits);
-              break;
-            }
-            if (version == 4) {
-              uint32_t lo_addr_bits = m_data.GetU32_unchecked(&offset);
-              uint32_t hi_addr_bits = m_data.GetU32_unchecked(&offset);
+    auto lc_notes = FindLC_NOTEByName("addrable bits");
+    for (auto lc_note : lc_notes) {
+      offset_t payload_offset = std::get<0>(lc_note);
+      if (m_data.GetU32(&payload_offset, &version, 1) != nullptr) {
+          uint32_t num_addr_bits = m_data.GetU32_unchecked(&payload_offset);
+          uint32_t lo_addr_bits = m_data.GetU32_unchecked(&payload_offset);
+          uint32_t hi_addr_bits = m_data.GetU32_unchecked(&payload_offset);
 
-              if (lo_addr_bits == hi_addr_bits)
-                addressable_bits.SetAddressableBits(lo_addr_bits);
-              else
-                addressable_bits.SetAddressableBits(lo_addr_bits, hi_addr_bits);
-              LLDB_LOGF(log,
-                        "LC_NOTE 'addrable bits' v4 found, value %d & %d bits",
-                        lo_addr_bits, hi_addr_bits);
-
-              break;
-            }
-          }
-        }
-      }
-      offset = cmd_offset + lc.cmdsize;
-    }
+          LLDB_LOGF(log, "LC_NOTE 'addrable bits' v4 found, value %d & %d bits",
   }
   return addressable_bits;
 }
 
 bool ObjectFileMachO::GetCorefileMainBinaryInfo(addr_t &value,
                                                 bool &value_is_offset,
                                                 UUID &uuid,
                                                 ObjectFile::BinaryType &type) {
   Log *log(
       GetLog(LLDBLog::Symbols | LLDBLog::Process | LLDBLog::DynamicLoader));
   value = LLDB_INVALID_ADDRESS;
   value_is_offset = false;
   uuid.Clear();
   uint32_t log2_pagesize = 0; // not currently passed up to caller
   uint32_t platform = 0;      // not currently passed up to caller
   ModuleSP module_sp(GetModule());
   if (module_sp) {
     std::lock_guard<std::recursive_mutex> guard(module_sp->GetMutex());
-    lldb::offset_t offset = MachHeaderSizeFromMagic(m_header.magic);
-    for (uint32_t i = 0; i < m_header.ncmds; ++i) {
-      const uint32_t cmd_offset = offset;
-      llvm::MachO::load_command lc = {};
-      if (m_data.GetU32(&offset, &lc.cmd, 2) == nullptr)
-        break;
-      if (lc.cmd == LC_NOTE) {
-        char data_owner[17];
-        memset(data_owner, 0, sizeof(data_owner));
-        m_data.CopyData(offset, 16, data_owner);
-        offset += 16;
-        uint64_t fileoff = m_data.GetU64_unchecked(&offset);
-        uint64_t size = m_data.GetU64_unchecked(&offset);
+
+    auto lc_notes = FindLC_NOTEByName("main bin spec");
+    for (auto lc_note : lc_notes) {
+      offset_t payload_offset = std::get<0>(lc_note);
 
-        // struct main_bin_spec
-        // {
-        //     uint32_t version;       // currently 2
-        //     uint32_t type;          // 0 == unspecified, 1 == kernel,
-        //                             // 2 == user process,
-        //                             // 3 == standalone binary
-        //     uint64_t address;       // UINT64_MAX if address not specified
-        //     uint64_t slide;         // slide, UINT64_MAX if unspecified
-        //                             // 0 if no slide needs to be applied to
-        //                             // file address
-        //     uuid_t   uuid;          // all zero's if uuid not specified
-        //     uint32_t log2_pagesize; // process page size in log base 2,
-        //                             // e.g. 4k pages are 12.
-        //                             // 0 for unspecified
-        //     uint32_t platform;      // The Mach-O platform for this corefile.
-        //                             // 0 for unspecified.
-        //                             // The values are defined in
-        //                             // <mach-o/loader.h>, PLATFORM_*.
-        // } __attribute((packed));
 
-        // "main bin spec" (main binary specification) data payload is
-        // formatted:
-        //    uint32_t version       [currently 1]
-        //    uint32_t type          [0 == unspecified, 1 == kernel,
-        //                            2 == user process, 3 == firmware ]
-        //    uint64_t address       [ UINT64_MAX if address not specified ]
-        //    uuid_t   uuid          [ all zero's if uuid not specified ]
-        //    uint32_t log2_pagesize [ process page size in log base
-        //                             2, e.g. 4k pages are 12.
-        //                             0 for unspecified ]
-        //    uint32_t unused        [ for alignment ]
 
-        if (strcmp("main bin spec", data_owner) == 0 && size >= 32) {
-          offset = fileoff;
-          uint32_t version;
-          if (m_data.GetU32(&offset, &version, 1) != nullptr && version <= 2) {
-            uint32_t binspec_type = 0;
-            uuid_t raw_uuid;
-            memset(raw_uuid, 0, sizeof(uuid_t));
+      if (m_data.GetU32(&payload_offset, &version, 1) != nullptr &&
+          version <= 2) {
 
-            if (!m_data.GetU32(&offset, &binspec_type, 1))
-              return false;
-            if (!m_data.GetU64(&offset, &value, 1))
-              return false;
-            uint64_t slide = LLDB_INVALID_ADDRESS;
-            if (version > 1 && !m_data.GetU64(&offset, &slide, 1))
-              return false;
-            if (value == LLDB_INVALID_ADDRESS &&
-                slide != LLDB_INVALID_ADDRESS) {
-              value = slide;
-              value_is_offset = true;
-            }
+        if (!m_data.GetU32(&payload_offset, &binspec_type, 1))
+        if (!m_data.GetU64(&payload_offset, &value, 1))
+        if (version > 1 && !m_data.GetU64(&payload_offset, &slide, 1))
+        if (value == LLDB_INVALID_ADDRESS && slide != LLDB_INVALID_ADDRESS) {
 
-            if (m_data.CopyData(offset, sizeof(uuid_t), raw_uuid) != 0) {
-              uuid = UUID(raw_uuid, sizeof(uuid_t));
-              // convert the "main bin spec" type into our
-              // ObjectFile::BinaryType enum
-              const char *typestr = "unrecognized type";
-              switch (binspec_type) {
-              case 0:
-                type = eBinaryTypeUnknown;
-                typestr = "uknown";
-                break;
-              case 1:
-                type = eBinaryTypeKernel;
-                typestr = "xnu kernel";
-                break;
-              case 2:
-                type = eBinaryTypeUser;
-                typestr = "userland dyld";
-                break;
-              case 3:
-                type = eBinaryTypeStandalone;
-                typestr = "standalone";
-                break;
-              }
-              LLDB_LOGF(log,
-                        "LC_NOTE 'main bin spec' found, version %d type %d "
-                        "(%s), value 0x%" PRIx64 " value-is-slide==%s uuid %s",
-                        version, type, typestr, value,
-                        value_is_offset ? "true" : "false",
-                        uuid.GetAsString().c_str());
-              if (!m_data.GetU32(&offset, &log2_pagesize, 1))
-                return false;
-              if (version > 1 && !m_data.GetU32(&offset, &platform, 1))
-                return false;
-              return true;
-            }
-          }
-        }
-      }
-      offset = cmd_offset + lc.cmdsize;
+        if (m_data.CopyData(payload_offset, sizeof(uuid_t), raw_uuid) != 0) {
+          if (!m_data.GetU32(&payload_offset, &log2_pagesize, 1))
+          if (version > 1 && !m_data.GetU32(&payload_offset, &platform, 1))
+  return false;
+}
+
+bool ObjectFileMachO::GetCorefileThreadExtraInfos(std::vector<tid_t> &tids) {
+  tids.clear();
+  ModuleSP module_sp(GetModule());
+  if (module_sp) {
+    std::lock_guard<std::recursive_mutex> guard(module_sp->GetMutex());
+
+    Log *log(GetLog(LLDBLog::Object | LLDBLog::Process | LLDBLog::Thread));
+    auto lc_notes = FindLC_NOTEByName("process metadata");
+    for (auto lc_note : lc_notes) {
+      offset_t payload_offset = std::get<0>(lc_note);
+      offset_t strsize = std::get<1>(lc_note);
+      std::string buf(strsize, '\0');
+      if (m_data.CopyData(payload_offset, strsize, buf.data()) != strsize) {
+        LLDB_LOGF(log,
+                  "Unable to read %" PRIu64
+                  " bytes of 'process metadata' LC_NOTE JSON contents",
+                  strsize);
+        return false;
+      }
+      while (buf.back() == '\0')
+        buf.resize(buf.size() - 1);
+      StructuredData::ObjectSP object_sp = StructuredData::ParseJSON(buf);
+      StructuredData::Dictionary *dict = object_sp->GetAsDictionary();
+      if (!dict) {
+        LLDB_LOGF(log, "Unable to read 'process metadata' LC_NOTE, did not "
+                       "get a dictionary.");
+        return false;
+      }
+      StructuredData::Array *threads;
+      if (!dict->GetValueForKeyAsArray("threads", threads) || !threads) {
+        LLDB_LOGF(log,
+                  "'process metadata' LC_NOTE does not have a 'threads' key");
+        return false;
+      }
+      if (threads->GetSize() != GetNumThreadContexts()) {
+        LLDB_LOGF(log, "Unable to read 'process metadata' LC_NOTE, number of "
+                       "threads does not match number of LC_THREADS.");
+        return false;
+      }
+      const size_t num_threads = threads->GetSize();
+      for (size_t i = 0; i < num_threads; i++) {
+        StructuredData::Dictionary *thread;
+        if (!threads->GetItemAtIndexAsDictionary(i, thread) || !thread) {
+          LLDB_LOGF(log,
+                    "Unable to read 'process metadata' LC_NOTE, threads "
+                    "array does not have a dictionary at index %zu.",
+                    i);
+          return false;
+        }
+        tid_t tid = LLDB_INVALID_THREAD_ID;
+        if (thread->GetValueForKeyAsInteger<tid_t>("thread_id", tid))
+          if (tid == 0)
+            tid = LLDB_INVALID_THREAD_ID;
+        tids.push_back(tid);
+      }
+
+      if (log) {
+        StreamString logmsg;
+        logmsg.Printf("LC_NOTE 'process metadata' found: ");
+        dict->Dump(logmsg, /* pretty_print */ false);
+        LLDB_LOGF(log, "%s", logmsg.GetData());
+      }
+      return true;
     }
   }
   return false;
 }
 
 lldb::RegisterContextSP
 ObjectFileMachO::GetThreadContextAtIndex(uint32_t idx,
                                          lldb_private::Thread &thread) {
   lldb::RegisterContextSP reg_ctx_sp;
 
   ModuleSP module_sp(GetModule());
   if (module_sp) {
     std::lock_guard<std::recursive_mutex> guard(module_sp->GetMutex());
     if (!m_thread_context_offsets_valid)
       GetNumThreadContexts();
 
     const FileRangeArray::Entry *thread_context_file_range =
         m_thread_context_offsets.GetEntryAtIndex(idx);
     if (thread_context_file_range) {
 
       DataExtractor data(m_data, thread_context_file_range->GetRangeBase(),
                          thread_context_file_range->GetByteSize());
 
if (make_core) {
         // addressing in this process or 0 for unknown.
         uint64_t address_mask = process_sp->GetCodeAddressMask();
         if (address_mask != 0) {
           // LC_NOTE "addrable bits"
           mach_header.ncmds++;
           mach_header.sizeofcmds += sizeof(llvm::MachO::note_command);
         }
 
+        // LC_NOTE "process metadata"
+        mach_header.ncmds++;
+        mach_header.sizeofcmds += sizeof(llvm::MachO::note_command);
+
         // LC_NOTE "all image infos"
         mach_header.ncmds++;
         mach_header.sizeofcmds += sizeof(llvm::MachO::note_command);
 
         // Write the mach header
         buffer.PutHex32(mach_header.magic);
         buffer.PutHex32(mach_header.cputype);
         buffer.PutHex32(mach_header.cpusubtype);
if (make_core) {
               bits); // # of bits used for high addresses
           addrable_bits_lcnote_up->payload.PutHex32(0); // reserved
 
           file_offset += addrable_bits_lcnote_up->payload.GetSize();
 
           lc_notes.push_back(std::move(addrable_bits_lcnote_up));
         }
 
+        // Add "process metadata" LC_NOTE
+        std::unique_ptr<LCNoteEntry> thread_extrainfo_lcnote_up(
+            new LCNoteEntry(addr_byte_size, byte_order));
+        thread_extrainfo_lcnote_up->name = "process metadata";
+        thread_extrainfo_lcnote_up->payload_file_offset = file_offset;
+
+        StructuredData::DictionarySP dict(
+            std::make_shared<StructuredData::Dictionary>());
+        StructuredData::ArraySP threads(
+            std::make_shared<StructuredData::Array>());
+        for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) {
+          ThreadSP thread_sp(thread_list.GetThreadAtIndex(thread_idx));
+          StructuredData::DictionarySP thread(
+              std::make_shared<StructuredData::Dictionary>());
+          thread->AddIntegerItem("thread_id", thread_sp->GetID());
+          threads->AddItem(thread);
+        }
+        dict->AddItem("threads", threads);
+        StreamString strm;
+        dict->Dump(strm, /* pretty */ false);
+        thread_extrainfo_lcnote_up->payload.PutRawBytes(strm.GetData(),
+                                                        strm.GetSize());
+
+        file_offset += thread_extrainfo_lcnote_up->payload.GetSize();
+        file_offset = llvm::alignTo(file_offset, 16);
+        lc_notes.push_back(std::move(thread_extrainfo_lcnote_up));
+
         // Add "all image infos" LC_NOTE
         std::unique_ptr<LCNoteEntry> all_image_infos_lcnote_up(
             new LCNoteEntry(addr_byte_size, byte_order));
         all_image_infos_lcnote_up->name = "all image infos";
         all_image_infos_lcnote_up->payload_file_offset = file_offset;
         file_offset = CreateAllImageInfosPayload(
             process_sp, file_offset, all_image_infos_lcnote_up->payload,
             core_style);
return true; // This is the right plug to handle saving core files for
                  // this process
   }
   return false;
 }
 
 ObjectFileMachO::MachOCorefileAllImageInfos
 ObjectFileMachO::GetCorefileAllImageInfos() {
   MachOCorefileAllImageInfos image_infos;
-  Log *log(
-      GetLog(LLDBLog::Symbols | LLDBLog::Process | LLDBLog::DynamicLoader));
-
-  // Look for an "all image infos" LC_NOTE.
-  lldb::offset_t offset = MachHeaderSizeFromMagic(m_header.magic);
-  for (uint32_t i = 0; i < m_header.ncmds; ++i) {
-    const uint32_t cmd_offset = offset;
-    llvm::MachO::load_command lc = {};
-    if (m_data.GetU32(&offset, &lc.cmd, 2) == nullptr)
-      break;
-    if (lc.cmd == LC_NOTE) {
-      char data_owner[17];
-      m_data.CopyData(offset, 16, data_owner);
-      data_owner[16] = '\0';
-      offset += 16;
-      uint64_t fileoff = m_data.GetU64_unchecked(&offset);
-      offset += 4; /* size unused */
+  Log *log(GetLog(LLDBLog::Object | LLDBLog::Symbols | LLDBLog::Process |
+                  LLDBLog::DynamicLoader));
 
-      if (strcmp("all image infos", data_owner) == 0) {
-        offset = fileoff;
-        // Read the struct all_image_infos_header.
-        uint32_t version = m_data.GetU32(&offset);
-        if (version != 1) {
-          return image_infos;
-        }
-        uint32_t imgcount = m_data.GetU32(&offset);
-        uint64_t entries_fileoff = m_data.GetU64(&offset);
-        // 'entries_size' is not used, nor is the 'unused' entry.
-        //  offset += 4; // uint32_t entries_size;
-        //  offset += 4; // uint32_t unused;
+  auto lc_notes = FindLC_NOTEByName("all image infos");
+  for (auto lc_note : lc_notes) {
+    offset_t payload_offset = std::get<0>(lc_note);
+    uint32_t version = m_data.GetU32(&payload_offset);
+    uint32_t imgcount = m_data.GetU32(&payload_offset);
+    uint64_t entries_fileoff = m_data.GetU64(&payload_offset);
 
-        LLDB_LOGF(log,
-                  "LC_NOTE 'all image infos' found version %d with %d images",
-                  version, imgcount);
-        offset = entries_fileoff;
-        for (uint32_t i = 0; i < imgcount; i++) {
-          // Read the struct image_entry.
-          offset_t filepath_offset = m_data.GetU64(&offset);
-          uuid_t uuid;
-          memcpy(&uuid, m_data.GetData(&offset, sizeof(uuid_t)),
-                 sizeof(uuid_t));
-          uint64_t load_address = m_data.GetU64(&offset);
-          offset_t seg_addrs_offset = m_data.GetU64(&offset);
-          uint32_t segment_count = m_data.GetU32(&offset);
-          uint32_t currently_executing = m_data.GetU32(&offset);
+    LLDB_LOGF(log, "LC_NOTE 'all image infos' found version %d with %d images",
+    payload_offset = entries_fileoff;
+      offset_t filepath_offset = m_data.GetU64(&payload_offset);
+      memcpy(&uuid, m_data.GetData(&payload_offset, sizeof(uuid_t)),
+      uint64_t load_address = m_data.GetU64(&payload_offset);
+      offset_t seg_addrs_offset = m_data.GetU64(&payload_offset);
+      uint32_t segment_count = m_data.GetU32(&payload_offset);
+      uint32_t currently_executing = m_data.GetU32(&payload_offset);
 
-          MachOCorefileImageEntry image_entry;
-          image_entry.filename = (const char *)m_data.GetCStr(&filepath_offset);
-          image_entry.uuid = UUID(uuid, sizeof(uuid_t));
-          image_entry.load_address = load_address;
-          image_entry.currently_executing = currently_executing;
 
-          offset_t seg_vmaddrs_offset = seg_addrs_offset;
-          for (uint32_t j = 0; j < segment_count; j++) {
-            char segname[17];
-            m_data.CopyData(seg_vmaddrs_offset, 16, segname);
-            segname[16] = '\0';
-            seg_vmaddrs_offset += 16;
-            uint64_t vmaddr = m_data.GetU64(&seg_vmaddrs_offset);
-            seg_vmaddrs_offset += 8; /* unused */
 
-            std::tuple<ConstString, addr_t> new_seg{ConstString(segname),
-                                                    vmaddr};
-            image_entry.segment_load_addresses.push_back(new_seg);
-          }
-          LLDB_LOGF(
-              log, "  image entry: %s %s 0x%" PRIx64 " %s",
+        std::tuple<ConstString, addr_t> new_seg{ConstString(segname), vmaddr};
+      LLDB_LOGF(log, "  image entry: %s %s 0x%" PRIx64 " %s",
                 image_entry.filename.c_str(),
-              image_entry.uuid.GetAsString().c_str(), image_entry.load_address,
+                image_entry.uuid.GetAsString().c_str(),
+                image_entry.load_address,
                 image_entry.currently_executing ? "currently executing"
                                                 : "not currently executing");
-          image_infos.all_image_infos.push_back(image_entry);
-        }
-      } else if (strcmp("load binary", data_owner) == 0) {
-        uint32_t version = m_data.GetU32(&fileoff);
-        if (version == 1) {
-          uuid_t uuid;
-          memcpy(&uuid, m_data.GetData(&fileoff, sizeof(uuid_t)),
-                 sizeof(uuid_t));
-          uint64_t load_address = m_data.GetU64(&fileoff);
-          uint64_t slide = m_data.GetU64(&fileoff);
-          std::string filename = m_data.GetCStr(&fileoff);
+  }
+
+  lc_notes = FindLC_NOTEByName("load binary");
+  for (auto lc_note : lc_notes) {
+    offset_t payload_offset = std::get<0>(lc_note);
+    uint32_t version = m_data.GetU32(&payload_offset);
+      memcpy(&uuid, m_data.GetData(&payload_offset, sizeof(uuid_t)),
+      uint64_t load_address = m_data.GetU64(&payload_offset);
+      uint64_t slide = m_data.GetU64(&payload_offset);
+      std::string filename = m_data.GetCStr(&payload_offset);
 
-          MachOCorefileImageEntry image_entry;
-          image_entry.filename = filename;
-          image_entry.uuid = UUID(uuid, sizeof(uuid_t));
-          image_entry.load_address = load_address;
-          image_entry.slide = slide;
-          image_entry.currently_executing = true;
-          image_infos.all_image_infos.push_back(image_entry);
-          LLDB_LOGF(log,
-                    "LC_NOTE 'load binary' found, filename %s uuid %s load "
-                    "address 0x%" PRIx64 " slide 0x%" PRIx64,
-                    filename.c_str(),
-                    image_entry.uuid.IsValid()
-                        ? image_entry.uuid.GetAsString().c_str()
-                        : "00000000-0000-0000-0000-000000000000",
-                    load_address, slide);
-        }
-      }
-    }
-    offset = cmd_offset + lc.cmdsize;
-  }
 
   return image_infos;
 }
 
 bool ObjectFileMachO::LoadCoreFileImages(lldb_private::Process &process) {
   MachOCorefileAllImageInfos image_infos = GetCorefileAllImageInfos();
-  Log *log = GetLog(LLDBLog::DynamicLoader);
+  Log *log = GetLog(LLDBLog::Object | LLDBLog::DynamicLoader);
   Status error;
 
   bool found_platform_binary = false;
   ModuleList added_modules;
   for (MachOCorefileImageEntry &image : image_infos.all_image_infos) {
     ModuleSP module_sp, local_filesystem_module_sp;
 
     // If this is a platform binary, it has been loaded (or registered with

lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp

 bool ProcessMachCore::DoUpdateThreadList(ThreadList &old_thread_list,
                                          ThreadList &new_thread_list) {
   if (old_thread_list.GetSize(false) == 0) {
     // Make up the thread the first time this is called so we can setup our one
     // and only core thread state.
     ObjectFile *core_objfile = m_core_module_sp->GetObjectFile();
 
     if (core_objfile) {
+      std::set<tid_t> used_tids;
       const uint32_t num_threads = core_objfile->GetNumThreadContexts();
-      for (lldb::tid_t tid = 0; tid < num_threads; ++tid) {
-        ThreadSP thread_sp(new ThreadMachCore(*this, tid));
+      std::vector<tid_t> tids;
+      if (core_objfile->GetCorefileThreadExtraInfos(tids)) {
+        assert(tids.size() == num_threads);
+
+        // Find highest tid value.
+        tid_t highest_tid = 0;
+        for (uint32_t i = 0; i < num_threads; i++) {
+          if (tids[i] != LLDB_INVALID_THREAD_ID && tids[i] > highest_tid)
+            highest_tid = tids[i];
+        }
+        tid_t current_unused_tid = highest_tid + 1;
+        for (uint32_t i = 0; i < num_threads; i++) {
+          if (tids[i] == LLDB_INVALID_THREAD_ID) {
+            tids[i] = current_unused_tid++;
+          }
+        }
+      } else {
+        // No metadata, insert numbers sequentially from 0.
+        for (uint32_t i = 0; i < num_threads; i++) {
+          tids.push_back(i);
+        }
+      }
+
+      for (uint32_t i = 0; i < num_threads; i++) {
+        ThreadSP thread_sp =
+            std::make_shared<ThreadMachCore>(*this, tids[i], i);
         new_thread_list.AddThread(thread_sp);
       }
     }
   } else {
     const uint32_t num_threads = old_thread_list.GetSize(false);
     for (uint32_t i = 0; i < num_threads; ++i)
       new_thread_list.AddThread(old_thread_list.GetThreadAtIndex(i, false));
   }
   return new_thread_list.GetSize(false) > 0;
 }
 
 void ProcessMachCore::RefreshStateAfterStop() {
   // Let all threads recover from stopping and do any clean up based on the

lldb/source/Plugins/Process/mach-core/ThreadMachCore.h

 #include <string>
 
 #include "lldb/Target/Thread.h"
 
 class ProcessMachCore;
 
 class ThreadMachCore : public lldb_private::Thread {
 public:
-  ThreadMachCore(lldb_private::Process &process, lldb::tid_t tid);
+  ThreadMachCore(lldb_private::Process &process, lldb::tid_t tid,
+                 uint32_t objfile_lc_thread_idx);
 
   ~ThreadMachCore() override;
 
   void RefreshStateAfterStop() override;
 
   const char *GetName() override;
 
   lldb::RegisterContextSP GetRegisterContext() override;

 protected:
   friend class ProcessMachCore;
 
   // Member variables.
   std::string m_thread_name;
   std::string m_dispatch_queue_name;
   lldb::addr_t m_thread_dispatch_qaddr;
   lldb::RegisterContextSP m_thread_reg_ctx_sp;
+  uint32_t m_objfile_lc_thread_idx;
 
   // Protected member functions.
   bool CalculateStopInfo() override;
 };
 
 #endif // LLDB_SOURCE_PLUGINS_PROCESS_MACH_CORE_THREADMACHCORE_H

lldb/source/Plugins/Process/mach-core/ThreadMachCore.cpp

 //#include "RegisterContextKDP_i386.h"
 //#include "RegisterContextKDP_x86_64.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 // Thread Registers
 
-ThreadMachCore::ThreadMachCore(Process &process, lldb::tid_t tid)
+ThreadMachCore::ThreadMachCore(Process &process, lldb::tid_t tid,
+                               uint32_t objfile_lc_thread_idx)
     : Thread(process, tid), m_thread_name(), m_dispatch_queue_name(),
-      m_thread_dispatch_qaddr(LLDB_INVALID_ADDRESS), m_thread_reg_ctx_sp() {}
+      m_thread_dispatch_qaddr(LLDB_INVALID_ADDRESS), m_thread_reg_ctx_sp(),
+      m_objfile_lc_thread_idx(objfile_lc_thread_idx) {}
 
 ThreadMachCore::~ThreadMachCore() { DestroyThread(); }
 
 const char *ThreadMachCore::GetName() {
   if (m_thread_name.empty())
     return nullptr;
   return m_thread_name.c_str();
 }
ThreadMachCore::CreateRegisterContextForFrame(StackFrame *frame) {
 
   if (concrete_frame_idx == 0) {
     if (!m_thread_reg_ctx_sp) {
       ProcessSP process_sp(GetProcess());
 
       ObjectFile *core_objfile =
           static_cast<ProcessMachCore *>(process_sp.get())->GetCoreObjectFile();
       if (core_objfile)
-        m_thread_reg_ctx_sp =
-            core_objfile->GetThreadContextAtIndex(GetID(), *this);
+        m_thread_reg_ctx_sp = core_objfile->GetThreadContextAtIndex(
+            m_objfile_lc_thread_idx, *this);
     }
     reg_ctx_sp = m_thread_reg_ctx_sp;
   } else {
     reg_ctx_sp = GetUnwinder().CreateRegisterContextForFrame(frame);
   }
   return reg_ctx_sp;
 }
 

lldb/test/API/macosx/corefile-exception-reason/TestCorefileExceptionReason.py

def test(self):
         self.build()
         (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(
             self, "// break here", lldb.SBFileSpec("main.cpp")
         )
 
         self.runCmd("continue")
 
         self.runCmd("process save-core -s stack " + corefile)
+        live_tids = []
+        if self.TraceOn():
+            self.runCmd("thread list")
+        for t in process.threads:
+            live_tids.append(t.GetThreadID())
         process.Kill()
         self.dbg.DeleteTarget(target)
 
         # Now load the corefile
         target = self.dbg.CreateTarget("")
         process = target.LoadCore(corefile)
         thread = process.GetSelectedThread()
         self.assertTrue(process.GetSelectedThread().IsValid())
         if self.TraceOn():
             self.runCmd("image list")
             self.runCmd("bt")
             self.runCmd("fr v")
 
         self.assertEqual(
             thread.GetStopDescription(256), "ESR_EC_DABORT_EL0 (fault address: 0x0)"
         )
+
+        if self.TraceOn():
+            self.runCmd("thread list")
+        for i in range(process.GetNumThreads()):
+            t = process.GetThreadAtIndex(i)
+            self.assertEqual(t.GetThreadID(), live_tids[i])