diff options
author | Simon Marchi <simon.marchi@polymtl.ca> | 2024-12-04 15:07:32 -0500 |
---|---|---|
committer | Simon Marchi <simon.marchi@polymtl.ca> | 2024-12-07 15:48:50 -0500 |
commit | 7893943879cedcc6520d0849fa3d10375dda0333 (patch) | |
tree | 55636d84d95e4bdc4a388ea2a347aa55cf0c25f5 | |
parent | 0623a1a5365bbb373a802dc12208a3a98943ce9b (diff) | |
download | binutils-7893943879cedcc6520d0849fa3d10375dda0333.zip binutils-7893943879cedcc6520d0849fa3d10375dda0333.tar.gz binutils-7893943879cedcc6520d0849fa3d10375dda0333.tar.bz2 |
gdbserver: simplify win32 process removal
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
-rw-r--r-- | gdbserver/dll.cc | 9 | ||||
-rw-r--r-- | gdbserver/dll.h | 1 | ||||
-rw-r--r-- | gdbserver/inferiors.cc | 15 | ||||
-rw-r--r-- | gdbserver/inferiors.h | 6 | ||||
-rw-r--r-- | gdbserver/win32-low.cc | 11 |
5 files changed, 5 insertions, 37 deletions
diff --git a/gdbserver/dll.cc b/gdbserver/dll.cc index ff637a0..f49aa56 100644 --- a/gdbserver/dll.cc +++ b/gdbserver/dll.cc @@ -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 (); - }); -} diff --git a/gdbserver/dll.h b/gdbserver/dll.h index 30c6a2c..e603df4 100644 --- a/gdbserver/dll.h +++ b/gdbserver/dll.h @@ -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); diff --git a/gdbserver/inferiors.cc b/gdbserver/inferiors.cc index 7a0209b..7bb322b 100644 --- a/gdbserver/inferiors.cc +++ b/gdbserver/inferiors.cc @@ -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) { diff --git a/gdbserver/inferiors.h b/gdbserver/inferiors.h index 2f3d42b..890e9b9 100644 --- a/gdbserver/inferiors.h +++ b/gdbserver/inferiors.h @@ -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); diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc index dab57f7..49f0ed3 100644 --- a/gdbserver/win32-low.cc +++ b/gdbserver/win32-low.cc @@ -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: |