[lldb] [POSIX-DYLD] Update the cached exe path after attach

[mgorny]

Summary

Fix the POSIX-DYLD plugin to update the cached executable path after
attaching. Previously, the path was cached in DYLDRendezvous
constructor and not updated afterwards. This meant that if LLDB was
attaching to a process (e.g. via connecting to lldb-server), the code
stored the empty path before DidAttach() resolved it. The fix updates
the cached path in DidAttach().

This fixes a new instance of https://llvm.org/pr17880

[labath]

Seems reasonable(ish), but needs a test case. Do you need to launch a separate server instance, or would just a plain attach work?

[mgorny]

In D92264#2422199, @labath wrote:

Seems reasonable(ish), but needs a test case. Do you need to launch a separate server instance, or would just a plain attach work?

It failed only when connecting to remote lldb-server.

[mgorny]

With D92667, I've finally found a way to reproduce this without running lldb-server explicitly — we need to spawn the program via relative path though. I've added a test that does that, and compares the address from image list with the one gotten after explicitly creating target via file path. This clearly reproduces the problem if you don't modify the sources (i.e. mismatch between 0x400000 and 0x200000).

[emaste]

Seems reasonable to me

[labath]

I am having second thoughts about the image list approach to testing this. The regex matching is somewhat messy.

A bad load address should manifest itself in plenty of other ways.. Could we print the value of some global variable to check that we got the correct one?

lldb/test/API/commands/process/attach/TestProcessAttach.py

101

Do you need to attach at any particular moment during the process startup? The attach command could run pretty quickly, before the loader finished its work...

[mgorny]

In D92264#2436440, @labath wrote:

I am having second thoughts about the image list approach to testing this. The regex matching is somewhat messy.

A bad load address should manifest itself in plenty of other ways.. Could we print the value of some global variable to check that we got the correct one?

I suppose we could do that.

lldb/test/API/commands/process/attach/TestProcessAttach.py

101

I don't know ;-). Do you have any particular suggestion? That said, I suppose this means other tests suffer from the same problem, in particular they could attach before lldb_enable_attach().

[mgorny]

Rewritten the test wrt comments, that is:

1. Added a breakpoint-continue to ensure that the program is past all initial work before we test it.
2. Print a global variable to test it.

I have also confirmed that reading the variable fails without the code change (because of reading wrong memory address), but also the breakpoint doesn't get set correctly.

[labath]

In D92264#2436839, @mgorny wrote:

Rewritten the test wrt comments, that is:

1. Added a breakpoint-continue to ensure that the program is past all initial work before we test it.

I am not sure that will help -- I'm guessing the dynamic loader plugin will go through a substantially different codepath if the attach happens early on vs. if it happens after main is reached. It may be better to update this to do the wait_for_file_on_target dance, which would also prevent attach denied errors.

lldb/test/API/commands/process/attach/TestProcessAttach.py

101

Good point. Some of our attach tests have the additional lldbutil.wait_for_file_on_target call as an additional synchronization. I guess this test is not one of them...

lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.h

struct ThreadInfo {
     uint32_t dtv_offset;    // offset of DTV pointer within pthread
     uint32_t dtv_slot_size; // size of one DTV slot
     uint32_t modid_offset;  // offset of module ID within link_map
     uint32_t tls_offset;    // offset of TLS pointer within DTV slot
   };
 
   DYLDRendezvous(lldb_private::Process *process);
 
+  /// Update the cached executable path.
+  void UpdateExecutablePath();
+
   /// Update the internal snapshot of runtime linker rendezvous and recompute
   /// the currently loaded modules.
   ///
   /// This method should be called once one start up, then once each time the
   /// runtime linker enters the function given by GetBreakAddress().
   ///
   /// \returns true on success and false on failure.
   ///

lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp

static addr_t ResolveRendezvousAddress(Process *process) {
 
   return info_addr;
 }
 
 DYLDRendezvous::DYLDRendezvous(Process *process)
     : m_process(process), m_rendezvous_addr(LLDB_INVALID_ADDRESS), m_current(),
       m_previous(), m_loaded_modules(), m_soentries(), m_added_soentries(),
       m_removed_soentries() {
-  Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_DYNAMIC_LOADER));
-
   m_thread_info.valid = false;
+  UpdateExecutablePath();
+}
 
-  // Cache a copy of the executable path
+void DYLDRendezvous::UpdateExecutablePath() {
   if (m_process) {
+    Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_DYNAMIC_LOADER));
     Module *exe_mod = m_process->GetTarget().GetExecutableModulePointer();
     if (exe_mod) {
       m_exe_file_spec = exe_mod->GetPlatformFileSpec();
       LLDB_LOGF(log, "DYLDRendezvous::%s exe module executable path set: '%s'",
                 __FUNCTION__, m_exe_file_spec.GetCString());
     } else {
       LLDB_LOGF(log,
                 "DYLDRendezvous::%s cannot cache exe module path: null "

lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp

LLDB_LOGF(
       __FUNCTION__, m_process ? m_process->GetID() : LLDB_INVALID_PROCESS_ID);
 
   // ask the process if it can load any of its own modules
   auto error = m_process->LoadModules();
   LLDB_LOG_ERROR(log, std::move(error), "Couldn't load modules: {0}");
 
   ModuleSP executable_sp = GetTargetExecutable();
   ResolveExecutableModule(executable_sp);
+  m_rendezvous.UpdateExecutablePath();
 
   // find the main process load offset
   addr_t load_offset = ComputeLoadOffset();
   LLDB_LOGF(log,
             "DynamicLoaderPOSIXDYLD::%s pid %" PRIu64
             " executable '%s', load_offset 0x%" PRIx64,
             __FUNCTION__,
             m_process ? m_process->GetID() : LLDB_INVALID_PROCESS_ID,

lldb/test/API/commands/process/attach/TestProcessAttach.py

 
 
 class ProcessAttachTestCase(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
     NO_DEBUG_INFO_TESTCASE = True
 
+    def setUp(self):
+        # Call super's setUp().
+        TestBase.setUp(self)
+        # Find the line number to break for main.c.
+        self.line = line_number('main.cpp',
+                                '// Waiting to be attached...')
+
     @skipIfiOSSimulator
     def test_attach_to_process_by_id(self):
         """Test attach by process id"""
         self.build()
         exe = self.getBuildArtifact(exe_name)
 
         # Spawn a new process
         popen = self.spawnSubprocess(exe)
def test_attach_to_process_by_name(self):
 
         self.runCmd("process attach -n " + exe_name)
 
         target = self.dbg.GetSelectedTarget()
 
         process = target.GetProcess()
         self.assertTrue(process, PROCESS_IS_VALID)
 
+    def test_attach_to_process_by_id_correct_executable_offset(self):
+        """
+        Test that after attaching to a process the executable offset
+        is determined correctly on FreeBSD.  This is a regression test
+        for dyld plugin getting the correct executable path,
+        and therefore being able to identify it in the module list.
+        """
+
+        self.build()
+        exe = self.getBuildArtifact(exe_name)
+
+        # In order to reproduce, we must spawn using a relative path
+        popen = self.spawnSubprocess(os.path.relpath(exe))
+
+        self.runCmd("process attach -p " + str(popen.pid))
+
+        # Make suer we did not attach to early
+        lldbutil.run_break_set_by_file_and_line(
+            self, "main.cpp", self.line, num_expected_locations=1, loc_exact=False)
+        self.runCmd("process continue")
+        self.expect("p g_val", substrs=["$0 = 12345"])
+
     def tearDown(self):
         # Destroy process before TestBase.tearDown()
         self.dbg.GetSelectedTarget().GetProcess().Destroy()
 
         # Call super's tearDown().
         TestBase.tearDown(self)

lldb/test/API/commands/process/attach/main.cpp

 #include <stdio.h>
 
 #include <chrono>
 #include <thread>
 
+volatile int g_val = 12345;
+
 int main(int argc, char const *argv[]) {
     int temp;
     lldb_enable_attach();
 
     // Waiting to be attached by the debugger.
     temp = 0;
 
     while (temp < 30) // Waiting to be attached...
     {
         std::this_thread::sleep_for(std::chrono::seconds(2));
         temp++;
     }
 
     printf("Exiting now\n");
 }