[Lldb-commits] [PATCH] D122429: [lldb/Plugin] Sort the ScriptedProcess' thread list before creating threads

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 418325.
mib added a comment.

Implement @JDevlieghere suggestions:

- Use a `std::map<size_t, ObjectSP>` to sort the thread info dictionary while preventing potential overflow


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

https://reviews.llvm.org/D122429

Files:
  lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp


Index: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
===================================================================
--- lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
+++ lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
@@ -304,35 +304,55 @@
 
   StructuredData::DictionarySP thread_info_sp = GetInterface().GetThreadsInfo();
 
-  // FIXME: Need to sort the dictionary otherwise the thread ids won't match the
-  // thread indices.
-
   if (!thread_info_sp)
     return ScriptedInterface::ErrorWithMessage<bool>(
         LLVM_PRETTY_FUNCTION,
         "Couldn't fetch thread list from Scripted Process.", error);
 
+  // Because `StructuredData::Dictionary` uses a `std::map<ConstString,
+  // ObjectSP>` for storage, each item is sorted based on the key alphabetical
+  // order. Since `GetThreadsInfo` provides thread indices as the key element,
+  // thread info comes ordered alphabetically, instead of numerically, so we
+  // need to sort the thread indices before creating thread.
+
+  StructuredData::ArraySP keys = thread_info_sp->GetKeys();
+
+  std::map<size_t, StructuredData::ObjectSP> sorted_threads;
+  auto sort_keys = [&sorted_threads,
+                    &thread_info_sp](StructuredData::Object *item) -> bool {
+    if (!item)
+      return false;
+
+    llvm::StringRef key = item->GetStringValue();
+    size_t idx = 0;
+
+    // Make sure the provided index is actually an integer
+    if (!llvm::to_integer(key, idx))
+      return false;
+
+    sorted_threads[idx] = thread_info_sp->GetValueForKey(key);
+    return true;
+  };
+
+  size_t thread_count = thread_info_sp->GetSize();
+
+  if (!keys->ForEach(sort_keys) || sorted_threads.size() != thread_count)
+    // Might be worth showing the unsorted thread list instead of return early.
+    return ScriptedInterface::ErrorWithMessage<bool>(
+        LLVM_PRETTY_FUNCTION, "Couldn't sort thread list.", error);
+
   auto create_scripted_thread =
-      [this, &old_thread_list, &error,
-       &new_thread_list](ConstString key, StructuredData::Object *val) -> bool {
-    if (!val)
-      return ScriptedInterface::ErrorWithMessage<bool>(
-          LLVM_PRETTY_FUNCTION, "Invalid thread info object", error);
+      [this, &error, &new_thread_list](
+          const std::pair<size_t, StructuredData::ObjectSP> pair) -> bool {
+    size_t idx = pair.first;
+    StructuredData::ObjectSP object_sp = pair.second;
 
-    lldb::tid_t tid = LLDB_INVALID_THREAD_ID;
-    if (!llvm::to_integer(key.AsCString(), tid))
+    if (!object_sp)
       return ScriptedInterface::ErrorWithMessage<bool>(
-          LLVM_PRETTY_FUNCTION, "Invalid thread id", error);
-
-    if (ThreadSP thread_sp =
-            old_thread_list.FindThreadByID(tid, false /*=can_update*/)) {
-      // If the thread was already in the old_thread_list,
-      // just add it back to the new_thread_list.
-      new_thread_list.AddThread(thread_sp);
-      return true;
-    }
+          LLVM_PRETTY_FUNCTION, "Invalid thread info object", error);
 
-    auto thread_or_error = ScriptedThread::Create(*this, val->GetAsGeneric());
+    auto thread_or_error =
+        ScriptedThread::Create(*this, object_sp->GetAsGeneric());
 
     if (!thread_or_error)
       return ScriptedInterface::ErrorWithMessage<bool>(
@@ -345,8 +365,7 @@
     if (!reg_ctx_sp)
       return ScriptedInterface::ErrorWithMessage<bool>(
           LLVM_PRETTY_FUNCTION,
-          llvm::Twine("Invalid Register Context for thread " +
-                      llvm::Twine(key.AsCString()))
+          llvm::Twine("Invalid Register Context for thread " + llvm::Twine(idx))
               .str(),
           error);
 
@@ -355,7 +374,7 @@
     return true;
   };
 
-  thread_info_sp->ForEach(create_scripted_thread);
+  llvm::for_each(sorted_threads, create_scripted_thread);
 
   return new_thread_list.GetSize(false) > 0;
 }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D122429.418325.patch
Type: text/x-patch
Size: 3873 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20220325/04f3e660/attachment.bin>