This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] gdbserver: Reset current_thread when its process is removed.
- From: Aleksandar Ristovski <aristovski at qnx dot com>
- To: Pedro Alves <palves at redhat dot com>, "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Fri, 16 Oct 2015 09:40:47 -0400
- Subject: Re: [PATCH] gdbserver: Reset current_thread when its process is removed.
- Authentication-results: sourceware.org; auth=none
- References: <1444919808-22088-1-git-send-email-aristovski at qnx dot com> <561FD2B2 dot 6070509 at redhat dot com>
On 15-10-15 12:22 PM, Pedro Alves wrote:
> On 10/15/2015 03:36 PM, Aleksandar Ristovski wrote:
>> In case of running gdbserver with --multi, if a nat file
>> removes a process, current_thread may remain set to now
>> freed 'process' entry.
>>
>> This may lead to wrong operation or a crash.
>>
>> gdb/gdbserver/ChangeLog:
>> * inferiors.c (remove_process): Reset current_thread if its
>> associated process gets removed.
>> ---
>> gdb/gdbserver/inferiors.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/gdb/gdbserver/inferiors.c b/gdb/gdbserver/inferiors.c
>> index 21f45fa..4688a44 100644
>> --- a/gdb/gdbserver/inferiors.c
>> +++ b/gdb/gdbserver/inferiors.c
>> @@ -291,6 +291,11 @@ remove_process (struct process_info *process)
>> {
>> clear_symbol_cache (&process->symbol_cache);
>> free_all_breakpoints (process);
>> + if (current_thread && get_thread_process (current_thread) == process)
>> + {
>> + remove_thread (current_thread);
>> + current_thread = NULL;
>> + }
>
> This seems very papering-over-something-else.
>
> - I could argue that current_thread = NULL would be better done
> inside remove_thread than here, since what you say would happen
> as well if the current thread is removed, even without removing
> the process.
Agree.
>
> - And then, if we remove the current thread, why not remove others?
>
True again.
> AFAICS, other targets remove threads from within target_mourn, as that
> way they have a chance of clearing auxiliary info associated with
> the threads (the inferior_target_data()), and then some call
> clear_inferiors, which also clears current_thread.
clear_inferiors wouldn't work if we want to keep other inferiors.
>
>> remove_inferior (&all_processes, &process->entry);
>> free (process);
>> }
>>
Ok, if this is intended use, to first remove threads then it will work.
I still think resetting current_thread in remove_thread is in order.
Assert in remove_process would help people like me.
Attached is another version as a proposal, though now not strictly
necessary. The patch did not show any regressions testing on a
native-gdbserver target board. (GNU/Linux x86_64)
Thanks,
Aleksandar
From 51f95e4541852b0d6a10cf931bdeeada1a9dd99c Mon Sep 17 00:00:00 2001
From: Aleksandar Ristovski <aristovski@qnx.com>
Date: Thu, 15 Oct 2015 09:23:48 -0400
Subject: [PATCH] gdbserver: Reset current_thread when the thread is removed.
Reset current_thread and make sure 'remove_process' is used
after all associated threads have been removed first.
gdb/gdbserver/ChangeLog:
* inferiors.c (thread_pid_matches_callback): New function.
(find_thread_process): New function.
(remove_thread): Reset current_thread.
(remove_process): Assert threads have been removed first.
---
gdb/gdbserver/inferiors.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/gdb/gdbserver/inferiors.c b/gdb/gdbserver/inferiors.c
index 21f45fa..fe47a72 100644
--- a/gdb/gdbserver/inferiors.c
+++ b/gdb/gdbserver/inferiors.c
@@ -141,6 +141,21 @@ find_thread_ptid (ptid_t ptid)
return (struct thread_info *) find_inferior_id (&all_threads, ptid);
}
+static int
+thread_pid_matches_callback (struct inferior_list_entry *entry, void *args)
+{
+ return (ptid_get_pid (entry->id) == *(pid_t *)args);
+}
+
+static struct thread_info *
+find_thread_process (const struct process_info *const process)
+{
+ pid_t pid = ptid_get_pid (ptid_of (process));
+
+ return (struct thread_info *)
+ find_inferior (&all_threads, thread_pid_matches_callback, &pid);
+}
+
ptid_t
gdb_id_to_thread_id (ptid_t gdb_id)
{
@@ -166,6 +181,8 @@ remove_thread (struct thread_info *thread)
discard_queued_stop_replies (ptid_of (thread));
remove_inferior (&all_threads, (struct inferior_list_entry *) thread);
free_one_thread (&thread->entry);
+ if (current_thread == thread)
+ current_thread = NULL;
}
/* Return a pointer to the first inferior in LIST, or NULL if there isn't one.
@@ -291,6 +308,7 @@ remove_process (struct process_info *process)
{
clear_symbol_cache (&process->symbol_cache);
free_all_breakpoints (process);
+ gdb_assert (find_thread_process (process) == NULL);
remove_inferior (&all_processes, &process->entry);
free (process);
}
--
1.9.1