From 150db43e412efba0f95ebde81aabd93e7535b909 Mon Sep 17 00:00:00 2001 From: Med Ismail Bennani Date: Fri, 25 Mar 2022 14:25:23 -0700 Subject: [PATCH] [lldb/Plugin] Sort the ScriptedProcess' thread list before creating threads MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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++ we convert it to a `StructuredData::Dictionary` that uses a `std::map` 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 ScriptedProcess 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 Differential Revision: https://reviews.llvm.org/D122429 Signed-off-by: Med Ismail Bennani --- .../Process/scripted/ScriptedProcess.cpp | 65 ++++++++++++------- 1 file changed, 42 insertions(+), 23 deletions(-) diff --git a/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp b/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp index e36b29db0feb..cd2d4fe4fd23 100644 --- a/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp +++ b/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp @@ -304,35 +304,55 @@ bool ScriptedProcess::DoUpdateThreadList(ThreadList &old_thread_list, 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( LLVM_PRETTY_FUNCTION, "Couldn't fetch thread list from Scripted Process.", error); + // Because `StructuredData::Dictionary` uses a `std::map` 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 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( + 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( - LLVM_PRETTY_FUNCTION, "Invalid thread info object", error); + [this, &error, &new_thread_list]( + const std::pair 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( - 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( @@ -345,8 +365,7 @@ bool ScriptedProcess::DoUpdateThreadList(ThreadList &old_thread_list, if (!reg_ctx_sp) return ScriptedInterface::ErrorWithMessage( 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 @@ bool ScriptedProcess::DoUpdateThreadList(ThreadList &old_thread_list, return true; }; - thread_info_sp->ForEach(create_scripted_thread); + llvm::for_each(sorted_threads, create_scripted_thread); return new_thread_list.GetSize(false) > 0; } -- 2.34.1