Delay loading of qProcessInfo-informed binaries until later in the Process setup

[jasonmolenda]

Summary

I handle two types of binary specification in qProcessInfo -- the main-binary-{uuid,address,slide} and binary-addresses -- so the remote stub can tell lldb to discover/load binaries when we connect.

I did this loading in ProcessGDBRemote::DoConnectRemote() which is very early in process setup, e.g. we don't have threads yet. This caused problems for one group that has a python script in the dSYM and need a more fully set up process.

This patch moves this binary loading from DoConnectRemote() to DidLaunchOrAttach() where we're nearly done setting up the Process. There's a method, MaybeLoadExecutableModule which reads the load address of the main binary from the remote stub (maybge on linux or something) and sets the load address in the Target; I added my new LoadStubBinaries() method next to that one in DidLaunchOrAttach(). I originally thought to simply add the code to MaybeLoadExecutableModule but then the method was doing two completely unrelated things and it would only serve to confuse people in the future I think.

I don't know of a particularly good reviewer on this one, but I'll add Jim without thinking of a better choice offhand.

[jingham]

This seems okay to me, I can't see why you would be required to fill in the "main binary" or "extra binaries" when we're just stopping for the attach. We shouldn't need them till we've decided we're launched or attached and need to print any info about that.

But given that it was at one point important to call LoadStubBinaries AFTER you had a process, you should leave a comment saying why it is important to wait till you have a process before reading in more binaries.

[jasonmolenda]

Add a comment about the intentional placement of binary loading/load address setting in DidLaunchOrAttach to address Jim's feedback.

[JDevlieghere]

LGMT

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

998

I know you just moved this code, but I don't think I've ever seen anyone write llvm::StringRef() instead of "".

1007–1008

I think the word "use" is missing at the end of line 1007.

1019–1020

I suspect you meant to replace "in this Process/Target" with "in the process of loading it"?

[jasonmolenda]

Thanks for the feedback Jonas, updated patch.

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h

SetThreadStopInfo(lldb::tid_t tid,
                     lldb::addr_t dispatch_queue_t, std::string &queue_name,
                     lldb::QueueKind queue_kind, uint64_t queue_serial);
 
   void ClearThreadIDList();
 
   bool UpdateThreadIDList();
 
   void DidLaunchOrAttach(ArchSpec &process_arch);
+  void LoadStubBinaries();
   void MaybeLoadExecutableModule();
 
   Status ConnectToDebugserver(llvm::StringRef host_port);
 
   const char *GetDispatchQueueNameForThread(lldb::addr_t thread_dispatch_qaddr,
                                             std::string &dispatch_queue_name);
 
   DynamicLoader *GetDynamicLoader() override;

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

if (m_gdb_comm.GetStopReply(response)) {
           target.SetArchitecture(m_gdb_comm.GetProcessArchitecture());
         } else {
           if (m_gdb_comm.GetHostArchitecture().IsValid()) {
             target.SetArchitecture(m_gdb_comm.GetHostArchitecture());
           }
         }
       }
 
-      // The remote stub may know about the "main binary" in
-      // the context of a firmware debug session, and can
-      // give us a UUID and an address/slide of where the
-      // binary is loaded in memory.
-      UUID standalone_uuid;
-      addr_t standalone_value;
-      bool standalone_value_is_offset;
-      if (m_gdb_comm.GetProcessStandaloneBinary(
-              standalone_uuid, standalone_value, standalone_value_is_offset)) {
-        ModuleSP module_sp;
-
-        if (standalone_uuid.IsValid()) {
-          const bool force_symbol_search = true;
-          const bool notify = true;
-          DynamicLoader::LoadBinaryWithUUIDAndAddress(
-              this, llvm::StringRef(), standalone_uuid, standalone_value,
-              standalone_value_is_offset, force_symbol_search, notify);
-        }
-      }
-
-      // The remote stub may know about a list of binaries to
-      // force load into the process -- a firmware type situation
-      // where multiple binaries are present in virtual memory,
-      // and we are only given the addresses of the binaries.
-      // Not intended for use with userland debugging when we
-      // a DynamicLoader plugin that knows how to find the loaded
-      // binaries and will track updates as binaries are added.
-
-      std::vector<addr_t> bin_addrs = m_gdb_comm.GetProcessStandaloneBinaries();
-      if (bin_addrs.size()) {
-        UUID uuid;
-        const bool value_is_slide = false;
-        for (addr_t addr : bin_addrs) {
-          const bool notify = true;
-          // First see if this is a special platform
-          // binary that may determine the DynamicLoader and
-          // Platform to be used in this Process/Target in the
-          // process of loading it.
-          if (GetTarget()
-                  .GetDebugger()
-                  .GetPlatformList()
-                  .LoadPlatformBinaryAndSetup(this, addr, notify))
-            continue;
-
-          const bool force_symbol_search = true;
-          // Second manually load this binary into the Target.
-          DynamicLoader::LoadBinaryWithUUIDAndAddress(
-              this, llvm::StringRef(), uuid, addr, value_is_slide,
-              force_symbol_search, notify);
-        }
-      }
-
       const StateType state = SetThreadStopInfo(response);
       if (state != eStateInvalid) {
         SetPrivateState(state);
       } else
         error.SetErrorStringWithFormat(
             "Process %" PRIu64 " was reported after connecting to "
             "'%s', but state was not stopped: %s",
             pid, remote_url.str().c_str(), StateAsCString(state));
if (target_arch.IsValid()) {
                target_arch.GetTriple().getTriple());
     } else {
       // The target doesn't have a valid architecture yet, set it from the
       // architecture we got from the remote GDB server
       GetTarget().SetArchitecture(process_arch);
     }
   }
 
+  // Target and Process are reasonably initailized;
+  // load any binaries we have metadata for / set load address.
+  LoadStubBinaries();
   MaybeLoadExecutableModule();
 
   // Find out which StructuredDataPlugins are supported by the debug monitor.
   // These plugins transmit data over async $J packets.
   if (StructuredData::Array *supported_packets =
           m_gdb_comm.GetSupportedStructuredDataPlugins())
     MapSupportedStructuredDataPlugins(*supported_packets);
 
   // If connected to LLDB ("native-signals+"), use signal defs for
   // the remote platform.  If connected to GDB, just use the standard set.
   if (!m_gdb_comm.UsesNativeSignals()) {
     SetUnixSignals(std::make_shared<GDBRemoteSignals>());
   } else {
     PlatformSP platform_sp = GetTarget().GetPlatform();
     if (platform_sp && platform_sp->IsConnected())
       SetUnixSignals(platform_sp->GetUnixSignals());
     else
       SetUnixSignals(UnixSignals::Create(GetTarget().GetArchitecture()));
   }
 }
 
+void ProcessGDBRemote::LoadStubBinaries() {
+  // The remote stub may know about the "main binary" in
+  // the context of a firmware debug session, and can
+  // give us a UUID and an address/slide of where the
+  // binary is loaded in memory.
+  UUID standalone_uuid;
+  addr_t standalone_value;
+  bool standalone_value_is_offset;
+  if (m_gdb_comm.GetProcessStandaloneBinary(standalone_uuid, standalone_value,
+                                            standalone_value_is_offset)) {
+    ModuleSP module_sp;
+
+    if (standalone_uuid.IsValid()) {
+      const bool force_symbol_search = true;
+      const bool notify = true;
+      DynamicLoader::LoadBinaryWithUUIDAndAddress(
+          this, "", standalone_uuid, standalone_value,
+          standalone_value_is_offset, force_symbol_search, notify);
+    }
+  }
+
+  // The remote stub may know about a list of binaries to
+  // force load into the process -- a firmware type situation
+  // where multiple binaries are present in virtual memory,
+  // and we are only given the addresses of the binaries.
+  // Not intended for use with userland debugging, when we use
+  // a DynamicLoader plugin that knows how to find the loaded
+  // binaries, and will track updates as binaries are added.
+
+  std::vector<addr_t> bin_addrs = m_gdb_comm.GetProcessStandaloneBinaries();
+  if (bin_addrs.size()) {
+    UUID uuid;
+    const bool value_is_slide = false;
+    for (addr_t addr : bin_addrs) {
+      const bool notify = true;
+      // First see if this is a special platform
+      // binary that may determine the DynamicLoader and
+      // Platform to be used in this Process and Target.
+      if (GetTarget()
+              .GetDebugger()
+              .GetPlatformList()
+              .LoadPlatformBinaryAndSetup(this, addr, notify))
+        continue;
+
+      const bool force_symbol_search = true;
+      // Second manually load this binary into the Target.
+      DynamicLoader::LoadBinaryWithUUIDAndAddress(this, llvm::StringRef(), uuid,
+                                                  addr, value_is_slide,
+                                                  force_symbol_search, notify);
+    }
+  }
+}
+
 void ProcessGDBRemote::MaybeLoadExecutableModule() {
   ModuleSP module_sp = GetTarget().GetExecutableModule();
   if (!module_sp)
     return;
 
   std::optional<QOffsets> offsets = m_gdb_comm.GetQOffsets();
   if (!offsets)
     return;