]> git.ipfire.org Git - thirdparty/binutils-gdb.git/commitdiff
gdbserver: simplify win32 process removal
authorSimon Marchi <simon.marchi@polymtl.ca>
Wed, 4 Dec 2024 20:07:32 +0000 (15:07 -0500)
committerSimon Marchi <simon.marchi@polymtl.ca>
Sat, 7 Dec 2024 20:48:50 +0000 (15:48 -0500)
In the spirit of encapsulation, I'm looking to remove the need for
external code to access the "ptid -> thread" map of process_info, making
it an internal implementation detail.  The only remaining use is in
function clear_inferiors, and it led me down this rabbit hole:

 - clear_inferiors is really only used by the Windows port and doesn't
   really make sense in the grand scheme of things, I think (when would
   you want to remove all threads of all processes, without removing
   those processes?)

 - ok, so let's remove clear_inferiors and inline the code where it's
   called, in function win32_clear_inferiors

 - the Windows port does not support multi-process, so it's not really
   necessary to loop over all processes like this:

       for_each_process ([] (process_info *process)
         {
           process->thread_list ().clear ();
           process->thread_map ().clear ();
         });

   We could just do:

     current_process ()->thread_list ().clear ();
     current_process ()->thread_map ().clear ();

   (or pass down the process from the caller, but it's not important
   right now)

 - so, the code that we've inlined in win32_clear_inferiors does 3
   things:

    - clear the process' thread list and map (which deletes the
      thread_info objects)

    - clear the dll list, which just basically frees some objects

    - switch to no current process / no current thread

 - let's now look at where this win32_clear_inferiors function is used:

     - in win32_process_target::kill, where the process is removed just
       after

     - in win32_process_target::detach, where the process is removed
       just after

     - in win32_process_target::wait, when handling a process exit.
       After this returns, we could be in handle_target_event (if async)
       or resume (if sync), both in `server.cc`.  In both of these
       cases, target_mourn_inferior gets called, we end up in
       win32_process_target::mourn, which removes the process

 - in all 3 cases above, we end up removing the process, which takes
   care of the 3 actions listed above:

     - the thread list and map get cleared when the process gets
       destroyed

     - same with the dll list

     - remove_process switches to no current process / current thread
       if the process being removed is the current one

 - I conclude that it's probably unnecessary to do the cleanup in
   win32_clear_inferiors, because it's going to get done right after
   anyway.

Therefore, this patch does:

 - remove clear_inferiors, remove the call in win32_clear_inferiors

 - remove clear_dlls, which is now unused

 - remove process_info::thread_map, which is now unused

 - rename win32_clear_inferiors to win32_clear_process, which seems more
   accurate

win32_clear_inferiors also does:

    for_each_thread (delete_thread_info);

which also makes sure to delete all threads, but it also deletes the
Windows private data object (windows_thread_info), so I'll leave this
one there for now.  But if we could make the thread private data
destruction automatic, on thread destruction, it could be removed, I
think.

There should be no user-visible change with this patch.  Of course,
operations don't happen in the same order as before, so there might be
some important detail I'm missing.  I'm only able to build-test this, if
someone could give it a test run on Windows, it would be appreciated.

Change-Id: I4a560affe763a2c965a97754cc02f3083dbe6fbf

gdbserver/dll.cc
gdbserver/dll.h
gdbserver/inferiors.cc
gdbserver/inferiors.h
gdbserver/win32-low.cc

index ff637a03fed14360e8236486e8a807c2473ae554..f49aa560aec57e246948d944ec377b0beb8d1dc5 100644 (file)
@@ -89,12 +89,3 @@ unloaded_dll (process_info *proc, const char *name, CORE_ADDR base_addr)
       proc->dlls_changed = true;
     }
 }
-
-void
-clear_dlls (void)
-{
-  for_each_process ([] (process_info *proc)
-    {
-      proc->all_dlls.clear ();
-    });
-}
index 30c6a2c65d06b946489e42d7ee0612d54971f872..e603df4be2b26017fa5b795f861d8d4111779d85 100644 (file)
@@ -32,7 +32,6 @@ struct dll_info
   CORE_ADDR base_addr;
 };
 
-extern void clear_dlls (void);
 extern void loaded_dll (const char *name, CORE_ADDR base_addr);
 extern void loaded_dll (process_info *proc, const char *name,
                        CORE_ADDR base_addr);
index 7a0209b167365747dd0503828d1207efdd1cc6c5..7bb322b0787fbe85ea7a4c2928bea39726964667 100644 (file)
@@ -112,21 +112,6 @@ process_info::remove_thread (thread_info *thread)
   m_thread_list.erase (m_thread_list.iterator_to (*thread));
 }
 
-void
-clear_inferiors (void)
-{
-  for_each_process ([] (process_info *process)
-    {
-      process->thread_list ().clear ();
-      process->thread_map ().clear ();
-    });
-
-  clear_dlls ();
-
-  switch_to_thread (nullptr);
-  current_process_ = nullptr;
-}
-
 struct process_info *
 add_process (int pid, int attached)
 {
index 2f3d42b8308171b2864de671d67f3c76d88ee332..890e9b92a9c46158041d9bde535d15412d599f21 100644 (file)
@@ -92,10 +92,6 @@ struct process_info : public intrusive_list_node<process_info>
   owning_intrusive_list<thread_info> &thread_list ()
   { return m_thread_list; }
 
-  /* Return a reference to the private thread map.  */
-  std::unordered_map<ptid_t, thread_info *> &thread_map ()
-  { return m_ptid_thread_map; }
-
   /* Return the number of threads in this process.  */
   unsigned int thread_count () const
   { return m_ptid_thread_map.size (); }
@@ -159,8 +155,6 @@ int have_attached_inferiors_p (void);
 /* Switch to a thread of PROC.  */
 void switch_to_process (process_info *proc);
 
-void clear_inferiors (void);
-
 /* Set the inferior current working directory.  If CWD is empty, unset
    the directory.  */
 void set_inferior_cwd (std::string cwd);
index dab57f7256b2ab9e89494c0efbaa4deac033034f..49f0ed3b1f4d2b2ac1d1c194d3910e4918568241 100644 (file)
@@ -660,7 +660,7 @@ gdbserver_windows_process::handle_output_debug_string
 }
 
 static void
-win32_clear_inferiors (void)
+win32_clear_process ()
 {
   if (windows_process.open_process_used)
     {
@@ -670,7 +670,6 @@ win32_clear_inferiors (void)
 
   for_each_thread (delete_thread_info);
   windows_process.siginfo_er.ExceptionCode = 0;
-  clear_inferiors ();
 }
 
 /* Implementation of target_ops::kill.  */
@@ -693,9 +692,9 @@ win32_process_target::kill (process_info *process)
        windows_process.handle_output_debug_string (nullptr);
     }
 
-  win32_clear_inferiors ();
-
+  win32_clear_process ();
   remove_process (process);
+
   return 0;
 }
 
@@ -714,7 +713,7 @@ win32_process_target::detach (process_info *process)
     return -1;
 
   DebugSetProcessKillOnExit (FALSE);
-  win32_clear_inferiors ();
+  win32_clear_process ();
   remove_process (process);
 
   return 0;
@@ -1197,7 +1196,7 @@ win32_process_target::wait (ptid_t ptid, target_waitstatus *ourstatus,
        case TARGET_WAITKIND_EXITED:
          OUTMSG2 (("Child exited with retcode = %x\n",
                    ourstatus->exit_status ()));
-         win32_clear_inferiors ();
+         win32_clear_process ();
          return ptid_t (windows_process.current_event.dwProcessId);
        case TARGET_WAITKIND_STOPPED:
        case TARGET_WAITKIND_SIGNALLED: