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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:5682-5693
+  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];
----------------
There's a *lot* of duplication between reading `LC_NOTE`s. At some point we should extract the common code instead of copy-pasting it every time. 


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:5723-5724
+        }
+        const uint32_t num_threads = threads->GetSize();
+        for (uint32_t i = 0; i < num_threads; i++) {
+          StructuredData::Dictionary *thread;
----------------
`StructuredData::Array::GetSize` returns a `size_t`. No point in turning this into a `uint32_t`. 


================
Comment at: lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp:603
+
+        // populate used_tids
+        for (uint32_t i = 0; i < num_threads; i++) {
----------------
Capitalize + period. 


================
Comment at: lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp:608-619
+        // If any threads have an unspecified thread id,
+        // find an unused number, use it instead.
+        tid_t current_unused_tid = 0;
+        for (uint32_t i = 0; i < num_threads; i++) {
+          if (tids[i] == LLDB_INVALID_THREAD_ID) {
+            while (used_tids.find(current_unused_tid) != used_tids.end()) {
+              current_unused_tid++;
----------------
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. 


================
Comment at: lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp:628-629
+      for (uint32_t i = 0; i < num_threads; i++) {
+        ThreadSP thread_sp(new ThreadMachCore(*this, tids[i], i));
         new_thread_list.AddThread(thread_sp);
       }
----------------
`make_shared` (avoid "naked new")


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158785/new/

https://reviews.llvm.org/D158785