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

[Med Ismail Bennani via Phabricator via lldb-commits]

mib created this revision.
mib added reviewers: JDevlieghere, jingham.
mib added a project: LLDB.
Herald added a project: All.
mib requested review of this revision.
Herald added a subscriber: lldb-commits.

With Scripted Processes, in order to create scripted threads, the blueprint
provides a dictionary that have each thread index as the key with the respective
thread instance as the pair value.

In Python, this is fine because a dictionary key can be of any type including
integer types:

  >>> {1: "one", 2: "two", 10: "ten"}
  {1: 'one', 2: 'two', 10: 'ten'}

However, when the python dictionary gets bridged to C++ with convert it to a
`StructuredData::Dictionary` that uses a `std::map<ConstString, ObjectSP>`
for storage.

Because `std::map` is an ordered container and ours uses the `ConstString`
type for keys, the thread indices gets converted to strings which makes the
dictionary sorted alphabetically, instead of numerically.

If the ScriptedProcesse has 10 threads or more, it causes thread “10”
(and higher) to be after thread “1”, but before thread “2”.

In order to solve this, this sorts the thread info dictionary keys
numerically, before iterating over them to create ScriptedThreads.

rdar://90327854

Signed-off-by: Med Ismail Bennani <medismail.bennani at gmail.com>


Repository:
  rG LLVM Github Monorepo

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,33 +304,53 @@
 
   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);
 
-  auto create_scripted_thread =
-      [this, &old_thread_list, &error,
-       &new_thread_list](ConstString key, StructuredData::Object *val) -> bool {
+  // 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::vector<llvm::StringRef> sorted_keys(keys->GetSize());
+  auto sort_keys = [&sorted_keys](StructuredData::Object *item) -> bool {
+    if (!item)
+      return false;
+
+    llvm::StringRef value = item->GetStringValue();
+    size_t idx = 0;
+
+    // Make sure the provided index is actually an integer
+    if (!llvm::to_integer(value, idx))
+      return false;
+
+    sorted_keys[idx] = value;
+    return true;
+  };
+
+  size_t thread_count = thread_info_sp->GetSize();
+
+  if (!keys->ForEach(sort_keys) || sorted_keys.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);
+
+  for (size_t idx = 0; idx < thread_count; idx++) {
+    llvm::StringRef key = sorted_keys[idx];
+    StructuredData::ObjectSP val = thread_info_sp->GetValueForKey(key);
     if (!val)
       return ScriptedInterface::ErrorWithMessage<bool>(
           LLVM_PRETTY_FUNCTION, "Invalid thread info object", error);
 
-    lldb::tid_t tid = LLDB_INVALID_THREAD_ID;
-    if (!llvm::to_integer(key.AsCString(), tid))
+    uint32_t thread_idx = UINT32_MAX;
+    if (!llvm::to_integer(key, thread_idx))
       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 index", error);
 
     auto thread_or_error = ScriptedThread::Create(*this, val->GetAsGeneric());
 
@@ -346,16 +366,12 @@
       return ScriptedInterface::ErrorWithMessage<bool>(
           LLVM_PRETTY_FUNCTION,
           llvm::Twine("Invalid Register Context for thread " +
-                      llvm::Twine(key.AsCString()))
+                      llvm::Twine(key.data()))
               .str(),
           error);
 
     new_thread_list.AddThread(thread_sp);
-
-    return true;
-  };
-
-  thread_info_sp->ForEach(create_scripted_thread);
+  }
 
   return new_thread_list.GetSize(false) > 0;
 }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D122429.418026.patch
Type: text/x-patch
Size: 3563 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20220324/43d6479c/attachment.bin>