[lldb/Target] Delay image loading after corefile process creation

[medismailben]

This patch is a follow-up to db223b7. Similarly to it, it changes the timing of binary image loading for the ProcessMachCore plugin.

This issue came up after getting reports of scripting resources that would fail to execute because they relied on data provided by the corefile process (i.e. for reading memory). However, rior to this change, the scripting resource loading would happen as part of the binary image loading, which in turns happened before the process finished being created.

This patch address that issue by delaying the binary image loading phase until we receive the corefile process stop event event, ensuring that the process is fully formed.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Med Ismail Bennani (medismailben)

Changes

This patch is a follow-up to db223b7. Similarly to it, it changes the timing of binary image loading for the ProcessMachCore plugin.

This issue came up after getting reports of scripting resources that would fail to execute because they relied on data provided by the corefile process (i.e. for reading memory). However, rior to this change, the scripting resource loading would happen as part of the binary image loading, which in turns happened before the process finished being created.

This patch address that issue by delaying the binary image loading phase until we receive the corefile process stop event event, ensuring that the process is fully formed.

---

Full diff: https://github.com/llvm/llvm-project/pull/70351.diff

4 Files Affected:

• (modified) lldb/include/lldb/Target/Process.h (+2)
• (modified) lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp (+2-2)
• (modified) lldb/source/Plugins/Process/mach-core/ProcessMachCore.h (+2)
• (modified) lldb/source/Target/Process.cpp (+16-13)

diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h
index a6d3e6c2d16926e..e25e82302a56dd9 100644
--- a/lldb/include/lldb/Target/Process.h
+++ b/lldb/include/lldb/Target/Process.h
@@ -614,6 +614,8 @@ class Process : public std::enable_shared_from_this<Process>,
     return error;
   }
 
+  virtual void DidLoadCore() {}
+
   /// The "ShadowListener" for a process is just an ordinary Listener that 
   /// listens for all the Process event bits.  It's convenient because you can
   /// specify it in the LaunchInfo or AttachInfo, so it will get events from
diff --git a/lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp b/lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
index b11062a0224abc2..9b10a0b832915d3 100644
--- a/lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
+++ b/lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
@@ -570,8 +570,6 @@ Status ProcessMachCore::DoLoadCore() {
 
   CreateMemoryRegions();
 
-  LoadBinariesAndSetDYLD();
-
   CleanupMemoryRegionPermissions();
 
   AddressableBits addressable_bits = core_objfile->GetAddressableBits();
@@ -580,6 +578,8 @@ Status ProcessMachCore::DoLoadCore() {
   return error;
 }
 
+void ProcessMachCore::DidLoadCore() { LoadBinariesAndSetDYLD(); }
+
 lldb_private::DynamicLoader *ProcessMachCore::GetDynamicLoader() {
   if (m_dyld_up.get() == nullptr)
     m_dyld_up.reset(DynamicLoader::FindPlugin(this, m_dyld_plugin_name));
diff --git a/lldb/source/Plugins/Process/mach-core/ProcessMachCore.h b/lldb/source/Plugins/Process/mach-core/ProcessMachCore.h
index c8820209e3f3830..0e61daa625b53cc 100644
--- a/lldb/source/Plugins/Process/mach-core/ProcessMachCore.h
+++ b/lldb/source/Plugins/Process/mach-core/ProcessMachCore.h
@@ -46,6 +46,8 @@ class ProcessMachCore : public lldb_private::PostMortemProcess {
   // Creating a new process, or attaching to an existing one
   lldb_private::Status DoLoadCore() override;
 
+  void DidLoadCore() override;
+
   lldb_private::DynamicLoader *GetDynamicLoader() override;
 
   // PluginInterface protocol
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index f82ab05362fbee9..f4bacf314dd746a 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -2639,19 +2639,6 @@ Status Process::LoadCore() {
     else
       StartPrivateStateThread();
 
-    DynamicLoader *dyld = GetDynamicLoader();
-    if (dyld)
-      dyld->DidAttach();
-
-    GetJITLoaders().DidAttach();
-
-    SystemRuntime *system_runtime = GetSystemRuntime();
-    if (system_runtime)
-      system_runtime->DidAttach();
-
-    if (!m_os_up)
-      LoadOperatingSystemPlugin(false);
-
     // We successfully loaded a core file, now pretend we stopped so we can
     // show all of the threads in the core file and explore the crashed state.
     SetPrivateState(eStateStopped);
@@ -2668,7 +2655,23 @@ Status Process::LoadCore() {
                 StateAsCString(state));
       error.SetErrorString(
           "Did not get stopped event after loading the core file.");
+    } else {
+      DidLoadCore();
+
+      DynamicLoader *dyld = GetDynamicLoader();
+      if (dyld)
+        dyld->DidAttach();
+
+      GetJITLoaders().DidAttach();
+
+      SystemRuntime *system_runtime = GetSystemRuntime();
+      if (system_runtime)
+        system_runtime->DidAttach();
+
+      if (!m_os_up)
+        LoadOperatingSystemPlugin(false);
     }
+
     RestoreProcessEvents();
   }
   return error;

[jasonmolenda]

Yes this looks good to me. I originally had a little concern about changing the ordering of how we call the methods in the corefile process plugins, but I don't think that's going to be an issue for them.

[jimingham]

Is there a reason this DidLoadCore only has a non-trivial implementation for Mach?

The generic and Mach parts of this are fine, so if there's a reason why this isn't needed for the other platform's core files, then this LGTM as well.

[medismailben]

Add test like @jimingham suggested.

[clayborg]

Looks good, just a question if we want to use the existing Process::DidAttach() instead of adding a new Process::DidLoadCore()

[clayborg]

Call DidAttach(...) instead?

[JDevlieghere]

LGTM per my last comment.

[medismailben]

Reverting because this broke some tests:

********************
Failed Tests (5):
  lldb-api :: functionalities/postmortem/elf-core/TestLinuxCore.py
  lldb-api :: functionalities/postmortem/mach-core/TestMachCore.py
  lldb-api :: functionalities/postmortem/netbsd-core/TestNetBSDCore.py
  lldb-api :: functionalities/unwind/noreturn/module-end/TestNoReturnModuleEnd.py
  lldb-api :: tools/lldb-dap/coreFile/TestDAP_coreFile.py