[3/3] gdbserver: win32 simplify process removal
Checks
Commit Message
From: Simon Marchi <simon.marchi@polymtl.ca>
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 | 9 ---------
gdbserver/dll.h | 1 -
gdbserver/inferiors.cc | 15 ---------------
gdbserver/inferiors.h | 6 ------
gdbserver/win32-low.cc | 11 +++++------
5 files changed, 5 insertions(+), 37 deletions(-)
Comments
Am Mittwoch, 4. Dezember 2024 um 17:39:44 MEZ hat <simark@simark.ca> Folgendes geschrieben:
> From: Simon Marchi <simon.marchi@polymtl.ca>
>
> 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.
Very nice that you noticed that all this code is redundant, this LGTM.
And I tested it a bit as well, and didn't see any problems.
Reviewed-By: Hannes Domani <ssbssa@yahoo.de>
Hannes
On 2024-12-07 10:19, Hannes Domani wrote:
> Am Mittwoch, 4. Dezember 2024 um 17:39:44 MEZ hat <simark@simark.ca> Folgendes geschrieben:
>
>> From: Simon Marchi <simon.marchi@polymtl.ca>
>>
>> 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.
>
> Very nice that you noticed that all this code is redundant, this LGTM.
> And I tested it a bit as well, and didn't see any problems.
>
> Reviewed-By: Hannes Domani <ssbssa@yahoo.de>
Thank for reviewing. I pushed the patch, but unfortunately I forgot to
add your Reviewed-By, and I also wanted to add Tested-By for Kevin,
sorry about that.
Simon
@@ -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 ();
- });
-}
@@ -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);
@@ -130,21 +130,6 @@ set_thread_regcache_data (struct thread_info *thread, struct regcache *data)
thread->regcache_data = data;
}
-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)
{
@@ -93,10 +93,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 (); }
@@ -160,8 +156,6 @@ int have_attached_inferiors_p (void);
/* Switch to a thread of PROC. */
void switch_to_process (process_info *proc);
-void clear_inferiors (void);
-
void *thread_target_data (struct thread_info *);
struct regcache *thread_regcache_data (struct thread_info *);
void set_thread_regcache_data (struct thread_info *, struct regcache *);
@@ -676,7 +676,7 @@ gdbserver_windows_process::handle_output_debug_string
}
static void
-win32_clear_inferiors (void)
+win32_clear_process ()
{
if (windows_process.open_process_used)
{
@@ -686,7 +686,6 @@ win32_clear_inferiors (void)
for_each_thread (delete_thread_info);
windows_process.siginfo_er.ExceptionCode = 0;
- clear_inferiors ();
}
/* Implementation of target_ops::kill. */
@@ -709,9 +708,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;
}
@@ -730,7 +729,7 @@ win32_process_target::detach (process_info *process)
return -1;
DebugSetProcessKillOnExit (FALSE);
- win32_clear_inferiors ();
+ win32_clear_process ();
remove_process (process);
return 0;
@@ -1219,7 +1218,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: