diff mbox series

[1/3] gdbserver: add and use `process_info::find_thread(ptid)`

Message ID 20241204163857.2297347-2-simark@simark.ca
State New
Headers
Series Fix GDBserver Windows build and a cleanup |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 fail Patch failed to apply
linaro-tcwg-bot/tcwg_gdb_build--master-arm fail Patch failed to apply

Commit Message

Simon Marchi Dec. 4, 2024, 4:35 p.m. UTC 
  From: Simon Marchi <simon.marchi@efficios.com>

Add an overload of `process_info::find_thread` that finds a thread by
ptid.  Use it in two spots.

Change-Id: I2b7fb819bf4f83f7bd37f8641c38e878119b3814
---
 gdbserver/inferiors.cc | 28 ++++++++++++++++------------
 gdbserver/inferiors.h  |  4 ++++
 2 files changed, 20 insertions(+), 12 deletions(-)

Comments

Rohr, Stephan Dec. 4, 2024, 7:22 p.m. UTC | #1 
Hi Simon,

This patch looks good, just a minor nit below.


> -----Original Message-----
> From: simark@simark.ca <simark@simark.ca>
> Sent: Wednesday, 4 December 2024 17:35
> To: gdb-patches@sourceware.org
> Cc: Simon Marchi <simon.marchi@efficios.com>
> Subject: [PATCH 1/3] gdbserver: add and use `process_info::find_thread(ptid)`
> 
> From: Simon Marchi <simon.marchi@efficios.com>
> 
> Add an overload of `process_info::find_thread` that finds a thread by
> ptid.  Use it in two spots.
> 
> Change-Id: I2b7fb819bf4f83f7bd37f8641c38e878119b3814
> ---
>  gdbserver/inferiors.cc | 28 ++++++++++++++++------------
>  gdbserver/inferiors.h  |  4 ++++
>  2 files changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/gdbserver/inferiors.cc b/gdbserver/inferiors.cc
> index e72b1c9bfc25..18d22cdae61d 100644
> --- a/gdbserver/inferiors.cc
> +++ b/gdbserver/inferiors.cc
> @@ -71,13 +71,7 @@ find_thread_ptid (ptid_t ptid)
>    if (process == nullptr)
>      return nullptr;
> 
> -  auto &thread_map = process->thread_map ();
> -
> -  if (auto it = thread_map.find (ptid);
> -      it != thread_map.end ())
> -    return it->second;
> -
> -  return nullptr;
> +  return process->find_thread (ptid);
>  }
> 
>  /* Find a thread associated with the given PROCESS, or NULL if no
> @@ -250,6 +244,18 @@ find_process (gdb::function_view<bool
> (process_info *)> func)
> 
>  /* See inferiors.h.  */
> 
> +thread_info *
> +process_info::find_thread (ptid_t ptid)
> +{
> +  if (auto it = m_ptid_thread_map.find (ptid);
> +      it != m_ptid_thread_map.end ())
> +    return it->second;
> +
> +  return nullptr;
> +}
> +
> +/* See inferiors.h.  */
> +
>  thread_info *
>  process_info::find_thread (gdb::function_view<bool (thread_info *)> func)
>  {
> @@ -300,11 +306,9 @@ find_thread (ptid_t filter, gdb::function_view<bool
> (thread_info *)> func)
>    if (filter.is_pid ())
>      return process->find_thread (func);
> 
> -  auto &thread_map = process->thread_map ();
> -
> -  if (auto it = thread_map.find (filter);
> -      it != thread_map.end () && func (it->second))
> -    return it->second;
> +  if (thread_info *thread = process->find_thread (filter);
> +      thread != nullptr && func (thread))
> +    return thread;
> 
>    return nullptr;
>  }
> diff --git a/gdbserver/inferiors.h b/gdbserver/inferiors.h
> index 5372a3cd5b78..728b043f3944 100644
> --- a/gdbserver/inferiors.h
> +++ b/gdbserver/inferiors.h
> @@ -97,6 +97,10 @@ struct process_info : public
> intrusive_list_node<process_info>
>    std::unordered_map<ptid_t, thread_info *> &thread_map ()
>    { return m_ptid_thread_map; }
> 
> +  /* Return the thread with ptid PTID, or nullptr if no such thread is
> +     found.  */
> +  thread_info *find_thread (ptid_t ptid);
> +

Nit: should we mention the full type ptid_t or rather omit the type of PTID in
the comment?

>    /* Find the first thread for which FUNC returns true.  Return nullptr if no
>       such thread is found.  */
>    thread_info *find_thread (gdb::function_view<bool (thread_info *)> func);
> --
> 2.47.1

Reviewed-By: Stephan Rohr <stephan.rohr@intel.com>

Stephan

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
Simon Marchi Dec. 4, 2024, 8:13 p.m. UTC | #2 
>> diff --git a/gdbserver/inferiors.h b/gdbserver/inferiors.h
>> index 5372a3cd5b78..728b043f3944 100644
>> --- a/gdbserver/inferiors.h
>> +++ b/gdbserver/inferiors.h
>> @@ -97,6 +97,10 @@ struct process_info : public
>> intrusive_list_node<process_info>
>>    std::unordered_map<ptid_t, thread_info *> &thread_map ()
>>    { return m_ptid_thread_map; }
>>
>> +  /* Return the thread with ptid PTID, or nullptr if no such thread is
>> +     found.  */
>> +  thread_info *find_thread (ptid_t ptid);
>> +
> 
> Nit: should we mention the full type ptid_t or rather omit the type of PTID in
> the comment?

I use `ptid` as a generic name, not directly the type or the variable
name.  You can think of it as a longer version would be "with the
process/thread identifier PTID".  It sounds good in my head at least.

>>    /* Find the first thread for which FUNC returns true.  Return nullptr if no
>>       such thread is found.  */
>>    thread_info *find_thread (gdb::function_view<bool (thread_info *)> func);
>> --
>> 2.47.1
> 
> Reviewed-By: Stephan Rohr <stephan.rohr@intel.com>

Ack, thanks.

Simon
diff mbox series

Patch

diff --git a/gdbserver/inferiors.cc b/gdbserver/inferiors.cc
index e72b1c9bfc25..18d22cdae61d 100644
--- a/gdbserver/inferiors.cc
+++ b/gdbserver/inferiors.cc
@@ -71,13 +71,7 @@  find_thread_ptid (ptid_t ptid)
   if (process == nullptr)
     return nullptr;
 
-  auto &thread_map = process->thread_map ();
-
-  if (auto it = thread_map.find (ptid);
-      it != thread_map.end ())
-    return it->second;
-
-  return nullptr;
+  return process->find_thread (ptid);
 }
 
 /* Find a thread associated with the given PROCESS, or NULL if no
@@ -250,6 +244,18 @@  find_process (gdb::function_view<bool (process_info *)> func)
 
 /* See inferiors.h.  */
 
+thread_info *
+process_info::find_thread (ptid_t ptid)
+{
+  if (auto it = m_ptid_thread_map.find (ptid);
+      it != m_ptid_thread_map.end ())
+    return it->second;
+
+  return nullptr;
+}
+
+/* See inferiors.h.  */
+
 thread_info *
 process_info::find_thread (gdb::function_view<bool (thread_info *)> func)
 {
@@ -300,11 +306,9 @@  find_thread (ptid_t filter, gdb::function_view<bool (thread_info *)> func)
   if (filter.is_pid ())
     return process->find_thread (func);
 
-  auto &thread_map = process->thread_map ();
-
-  if (auto it = thread_map.find (filter);
-      it != thread_map.end () && func (it->second))
-    return it->second;
+  if (thread_info *thread = process->find_thread (filter);
+      thread != nullptr && func (thread))
+    return thread;
 
   return nullptr;
 }
diff --git a/gdbserver/inferiors.h b/gdbserver/inferiors.h
index 5372a3cd5b78..728b043f3944 100644
--- a/gdbserver/inferiors.h
+++ b/gdbserver/inferiors.h
@@ -97,6 +97,10 @@  struct process_info : public intrusive_list_node<process_info>
   std::unordered_map<ptid_t, thread_info *> &thread_map ()
   { return m_ptid_thread_map; }
 
+  /* Return the thread with ptid PTID, or nullptr if no such thread is
+     found.  */
+  thread_info *find_thread (ptid_t ptid);
+
   /* Find the first thread for which FUNC returns true.  Return nullptr if no
      such thread is found.  */
   thread_info *find_thread (gdb::function_view<bool (thread_info *)> func);