[PATCH] gdbserver: Reset current_thread when its process is removed.

[PATCH] gdbserver: Reset current_thread when its process is removed.

[Aleksandar Ristovski]

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;
+    }
   remove_inferior (&all_processes, &process->entry);
   free (process);
 }
-- 
1.9.1

Re: [PATCH] gdbserver: Reset current_thread when its process is removed.

[Pedro Alves]

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.

- And then, if we remove the current thread, why not remove others?

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.

>    remove_inferior (&all_processes, &process->entry);
>    free (process);
>  }
> 

Thanks,
Pedro Alves

Re: [PATCH] gdbserver: Reset current_thread when its process is removed.

[Aleksandar Ristovski]

[-- Attachment #1: Type: text/plain, Size: 2197 bytes --]

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-gdbserver-Reset-current_thread-when-the-thread-is-re.patch --]
[-- Type: text/x-patch; name="0001-gdbserver-Reset-current_thread-when-the-thread-is-re.patch", Size: 2136 bytes --]

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

Re: [PATCH] gdbserver: Reset current_thread when its process is removed.

[Pedro Alves]

On 10/16/2015 02:40 PM, Aleksandar Ristovski wrote:

> +static int
> +thread_pid_matches_callback (struct inferior_list_entry *entry, void *args)
> +{

...

> +static struct thread_info *
> +find_thread_process (const struct process_info *const process)
> +{

OK with intro comments added.

Thanks,
Pedro Alves

Re: [PATCH] gdbserver: Reset current_thread when its process is removed.

[Aleksandar Ristovski]

On 15-10-16 10:04 AM, Pedro Alves wrote:
> On 10/16/2015 02:40 PM, Aleksandar Ristovski wrote:
> 
>> +static int
>> +thread_pid_matches_callback (struct inferior_list_entry *entry, void *args)
>> +{
> 
> ...
> 
>> +static struct thread_info *
>> +find_thread_process (const struct process_info *const process)
>> +{
> 
> OK with intro comments added.
> 

Like this?

diff --git a/gdb/gdbserver/inferiors.c b/gdb/gdbserver/inferiors.c
index fe47a72..72a3ef1 100644
--- a/gdb/gdbserver/inferiors.c
+++ b/gdb/gdbserver/inferiors.c
@@ -141,12 +141,18 @@ find_thread_ptid (ptid_t ptid)
   return (struct thread_info *) find_inferior_id (&all_threads, ptid);
 }

+/* Predicate function for matching thread entry's pid to the given
+   pid value passed by address in ARGS.  */
+
 static int
 thread_pid_matches_callback (struct inferior_list_entry *entry, void *args)
 {
   return (ptid_get_pid (entry->id) == *(pid_t *)args);
 }

+/* Find a thread associated with the given PROCESS, or NULL if no
+   such thread exists.  */
+
 static struct thread_info *
 find_thread_process (const struct process_info *const process)
 {

Re: [PATCH] gdbserver: Reset current_thread when its process is removed.

[Pedro Alves]

On 10/16/2015 03:48 PM, Aleksandar Ristovski wrote:
> On 15-10-16 10:04 AM, Pedro Alves wrote:

>> OK with intro comments added.
>>
> 
> Like this?

That's perfect.  Please push.

Thanks,
Pedro Alves