[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

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

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

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

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/source/Plugins/SystemRuntime/MacOSX/SystemRuntimeMacOSX.cpp

void SystemRuntimeMacOSX::PopulateQueueList(
 
   // We either didn't have libBacktraceRecording (and need to create the queues
   // list based on threads) or we did get the queues list from
   // libBacktraceRecording but some special queues may not be included in its
   // information.  This is needed because libBacktraceRecording will only list
   // queues with pending or running items by default - but the magic com.apple
   // .main-thread queue on thread 1 is always around.
 
+  bool is_scripted_process = m_process->GetPluginName() == "ScriptedProcess";
   for (ThreadSP thread_sp : m_process->Threads()) {
     if (thread_sp->GetAssociatedWithLibdispatchQueue() != eLazyBoolNo) {
       if (thread_sp->GetQueueID() != LLDB_INVALID_QUEUE_ID) {
         if (queue_list.FindQueueByID(thread_sp->GetQueueID()).get() ==
             nullptr) {
           QueueSP queue_sp(new Queue(m_process->shared_from_this(),
                                      thread_sp->GetQueueID(),
                                      thread_sp->GetQueueName()));
           if (thread_sp->ThreadHasQueueInformation()) {
             queue_sp->SetKind(thread_sp->GetQueueKind());
             queue_sp->SetLibdispatchQueueAddress(
                 thread_sp->GetQueueLibdispatchQueueAddress());
             queue_list.AddQueue(queue_sp);
           } else {
             queue_sp->SetKind(
                 GetQueueKind(thread_sp->GetQueueLibdispatchQueueAddress()));
             queue_sp->SetLibdispatchQueueAddress(
                 thread_sp->GetQueueLibdispatchQueueAddress());
             queue_list.AddQueue(queue_sp);
           }
         }
       }
     }
+    if (is_scripted_process) {
+      if (const char *queue_name = thread_sp->GetQueueName()) {
+        QueueSP queue_sp = std::make_shared<Queue>(
+            m_process->shared_from_this(), LLDB_INVALID_QUEUE_ID, queue_name);
+        queue_list.AddQueue(queue_sp);
+      }
+    }
   }
 }
 
 // Returns either an array of introspection_dispatch_item_info_ref's for the
 // pending items on a queue or an array introspection_dispatch_item_info_ref's
 // and code addresses for the pending items on a queue.  The information about
 // each of these pending items then needs to be fetched individually by passing
 // the ref to libBacktraceRecording.