[lldb/Process] Populate queues in Scripted Process

[mib]

Summary

This patch enhances queue support in Scripted Processes.

Scripted Threads could already report their queue name if they had one,
but this information was only surfaced when getting the process and
thread status.

However, no queue was create and added to the scripted process queue
list. This patch improves that by creating a queue from the scripted
thread queue name. For now, it uses an invalid queue id, since the
scripted thread doesn't expose this capability yet, but this could
easily be supported if the queue id information is available.

rdar://98844004

Signed-off-by: Med Ismail Bennani <[e-mail]>

[JDevlieghere]

lldb/source/Plugins/SystemRuntime/MacOSX/SystemRuntimeMacOSX.cpp

757 ↗

(On Diff #482163)

Comparing the plugin name defeats the abstraction a plugin is meant to provide. While we have other instances of LLDB breaking these abstractions, I don't recall other places where we compare the plugin name. The way we normally deal with this is extend the plugins capability (by adding a method) and implementing it accordingly for all the plugins (or have a sane default).

Based on the description of the patch it's not clear to me why this is special for scripted processes. If we need to special case this I'd like to see a comment explaining why.

[mib]

lldb/source/Plugins/SystemRuntime/MacOSX/SystemRuntimeMacOSX.cpp

757 ↗

(On Diff #482163)

I agree, but in this case, Process::UpdateQueueListIfNeeded calls the System Runtime plugin PopulateQueueList method and since queues are only supported on macOS, it does make sense in my opinion.

I think we definitely shouldn't add another system runtime for scripted processes just to support that case, so adding a special case for scripted processes seems to be the less intrusive way to implement it.

[labath]

lldb/source/Plugins/SystemRuntime/MacOSX/SystemRuntimeMacOSX.cpp

757 ↗

(On Diff #482163)

I'm with Jonas here. Even though these Queue concepts are a big mystery do me, I would like to understand why the scripted processes do can not go through the same code path as "regular" ones.

[mib]

lldb/source/Plugins/SystemRuntime/MacOSX/SystemRuntimeMacOSX.cpp

757 ↗

(On Diff #482163)

The reason here why this is not implemented at the Scripted Process plugin level, is because UpdateQueueListIfNeeded is a base method of the Process base class that's not expected to be redefined in the process plugin.

I guess we could either make UpdateQueueListIfNeeded virtual and keep the base class implementation the default and override it in the Scripted Process plugin class or we can have a virtual void Process::DoUpdateQueueList() {} method that would be called in Process::UpdateQueueListIfNeeded.

[mib]

Make use of the process plugin model to avoid adding a special case on the system runtime plugin.

[labath]

That definitely looks much better. As I understand it, the scripted thread interface is more high-level that our internal thread interface, and that means it is computing its queue by itself, instead of delegating the work to a separate plugin. That makes sense to me.

[mib]

In D139853#3992058, @labath wrote:

That definitely looks much better. As I understand it, the scripted thread interface is more high-level that our internal thread interface, and that means it is computing its queue by itself, instead of delegating the work to a separate plugin. That makes sense to me.

Correct, Scripted Thread will report "pre-computed" queues to lldb, so it won't need to rely on the system runtime to resolve them in lldb.

[JDevlieghere]

This looks like what I had in mind. LGTM.

lldb/include/lldb/Target/Process.h

 
   // Given a thread_id, it will assign a more reasonable index id for display
   // to the user. If the thread_id has previously been assigned, the same index
   // id will be used.
   uint32_t AssignIndexIDToThread(uint64_t thread_id);
 
   // Queue Queries
 
-  void UpdateQueueListIfNeeded();
+  virtual void UpdateQueueListIfNeeded();
 
   QueueList &GetQueueList() {
     UpdateQueueListIfNeeded();
     return m_queue_list;
   }
 
   QueueList::QueueIterable Queues() {
     UpdateQueueListIfNeeded();

lldb/source/Plugins/Process/scripted/ScriptedProcess.h

public:
 
   bool GetProcessInfo(ProcessInstanceInfo &info) override;
 
   lldb_private::StructuredData::ObjectSP
   GetLoadedDynamicLibrariesInfos() override;
 
   lldb_private::StructuredData::DictionarySP GetMetadata() override;
 
+  void UpdateQueueListIfNeeded() override;
+
 protected:
   ScriptedProcess(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp,
                   const ScriptedMetadata &scripted_metadata, Status &error);
 
   Status DoStop();
 
   void Clear();
 

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

 #include "lldb/Host/OptionParser.h"
 #include "lldb/Host/ThreadLauncher.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/OptionArgParser.h"
 #include "lldb/Interpreter/OptionGroupBoolean.h"
 #include "lldb/Interpreter/ScriptInterpreter.h"
 #include "lldb/Interpreter/ScriptedMetadata.h"
 #include "lldb/Target/MemoryRegionInfo.h"
+#include "lldb/Target/Queue.h"
 #include "lldb/Target/RegisterContext.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/State.h"
 
 #include <mutex>
 
 LLDB_PLUGIN_DEFINE(ScriptedProcess)
 
lldb_private::StructuredData::DictionarySP ScriptedProcess::GetMetadata() {
   Status error;
   if (!metadata_sp || !metadata_sp->GetSize())
     return ScriptedInterface::ErrorWithMessage<StructuredData::DictionarySP>(
         LLVM_PRETTY_FUNCTION, "No metadata.", error);
 
   return metadata_sp;
 }
 
+void ScriptedProcess::UpdateQueueListIfNeeded() {
+  CheckInterpreterAndScriptObject();
+  for (ThreadSP thread_sp : Threads()) {
+    if (const char *queue_name = thread_sp->GetQueueName()) {
+      QueueSP queue_sp = std::make_shared<Queue>(
+          m_process->shared_from_this(), thread_sp->GetQueueID(), queue_name);
+      m_queue_list.AddQueue(queue_sp);
+    }
+  }
+}
+
 ScriptedProcessInterface &ScriptedProcess::GetInterface() const {
   return m_interpreter->GetScriptedProcessInterface();
 }