* [PATCH 3/3] gdbserver: Remove thread_to_gdb_id
2017-09-10 20:11 [PATCH 0/3] Small cleanups in gdbserver Simon Marchi
@ 2017-09-10 20:11 ` Simon Marchi
2017-09-15 11:02 ` Pedro Alves
2017-09-10 20:11 ` [PATCH 2/3] gdbserver: Remove gdb_id_to_thread_id Simon Marchi
2017-09-10 20:11 ` [PATCH 1/3] gdbserver: Remove duplicate functions to find any thread of process Simon Marchi
2 siblings, 1 reply; 13+ messages in thread
From: Simon Marchi @ 2017-09-10 20:11 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
As explained in the previous patch, the gdb_id concept is no longer
relevant. The function thread_to_gdb_id is trivial, it returns the
thread's ptid. Remove it and replace its usage with ptid_of.
The changes in nto-low.c and lynx-low.c are fairly straightforward, but
I was not able to build test them.
gdb/gdbserver/ChangeLog:
* inferiors.h (thread_to_gdb_id): Remove.
* inferiors.c (thread_to_gdb_id): Remove.
* server.c (handle_qxfer_threads_worker, handle_query): Adjust.
* lynx-low.c (lynx_resume, lynx_wait_1, lynx_fetch_registers,
lynx_store_registers, lynx_read_memory, lynx_write_memory):
Likewise.
* nto-low.c (nto_fetch_registers, nto_store_registers,
nto_stopped_by_watchpoint, nto_stopped_data_address): Likewise.
---
gdb/gdbserver/inferiors.c | 6 ------
gdb/gdbserver/inferiors.h | 2 --
gdb/gdbserver/lynx-low.c | 14 +++++++-------
gdb/gdbserver/nto-low.c | 8 ++++----
gdb/gdbserver/server.c | 22 +++++++++-------------
5 files changed, 20 insertions(+), 32 deletions(-)
diff --git a/gdb/gdbserver/inferiors.c b/gdb/gdbserver/inferiors.c
index 933dd76..72f0412 100644
--- a/gdb/gdbserver/inferiors.c
+++ b/gdb/gdbserver/inferiors.c
@@ -121,12 +121,6 @@ add_thread (ptid_t thread_id, void *target_data)
return new_thread;
}
-ptid_t
-thread_to_gdb_id (struct thread_info *thread)
-{
- return thread->entry.id;
-}
-
/* Wrapper around get_first_inferior to return a struct thread_info *. */
struct thread_info *
diff --git a/gdb/gdbserver/inferiors.h b/gdb/gdbserver/inferiors.h
index 6316fe5..aef8433 100644
--- a/gdb/gdbserver/inferiors.h
+++ b/gdb/gdbserver/inferiors.h
@@ -143,8 +143,6 @@ struct process_info *find_process_pid (int pid);
int have_started_inferiors_p (void);
int have_attached_inferiors_p (void);
-ptid_t thread_to_gdb_id (struct thread_info *);
-
void clear_inferiors (void);
struct inferior_list_entry *find_inferior
(struct inferior_list *,
diff --git a/gdb/gdbserver/lynx-low.c b/gdb/gdbserver/lynx-low.c
index 77f570e..f074dd5 100644
--- a/gdb/gdbserver/lynx-low.c
+++ b/gdb/gdbserver/lynx-low.c
@@ -350,7 +350,7 @@ lynx_resume (struct thread_resume *resume_info, size_t n)
the moment we resume its execution for the first time. It is
fine to use the current_thread's ptid in those cases. */
if (ptid_equal (ptid, minus_one_ptid))
- ptid = thread_to_gdb_id (current_thread);
+ ptid = ptid_of (current_thread);
regcache_invalidate_pid (ptid_get_pid (ptid));
@@ -423,7 +423,7 @@ lynx_wait_1 (ptid_t ptid, struct target_waitstatus *status, int options)
ptid_t new_ptid;
if (ptid_equal (ptid, minus_one_ptid))
- pid = lynx_ptid_get_pid (thread_to_gdb_id (current_thread));
+ pid = lynx_ptid_get_pid (ptid_of (current_thread));
else
pid = BUILDPID (lynx_ptid_get_pid (ptid), lynx_ptid_get_tid (ptid));
@@ -612,7 +612,7 @@ static void
lynx_fetch_registers (struct regcache *regcache, int regno)
{
struct lynx_regset_info *regset = lynx_target_regsets;
- ptid_t inferior_ptid = thread_to_gdb_id (current_thread);
+ ptid_t inferior_ptid = ptid_of (current_thread);
lynx_debug ("lynx_fetch_registers (regno = %d)", regno);
@@ -637,7 +637,7 @@ static void
lynx_store_registers (struct regcache *regcache, int regno)
{
struct lynx_regset_info *regset = lynx_target_regsets;
- ptid_t inferior_ptid = thread_to_gdb_id (current_thread);
+ ptid_t inferior_ptid = ptid_of (current_thread);
lynx_debug ("lynx_store_registers (regno = %d)", regno);
@@ -673,7 +673,7 @@ lynx_read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
int buf;
const int xfer_size = sizeof (buf);
CORE_ADDR addr = memaddr & -(CORE_ADDR) xfer_size;
- ptid_t inferior_ptid = thread_to_gdb_id (current_thread);
+ ptid_t inferior_ptid = ptid_of (current_thread);
while (addr < memaddr + len)
{
@@ -706,7 +706,7 @@ lynx_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, int len)
int buf;
const int xfer_size = sizeof (buf);
CORE_ADDR addr = memaddr & -(CORE_ADDR) xfer_size;
- ptid_t inferior_ptid = thread_to_gdb_id (current_thread);
+ ptid_t inferior_ptid = ptid_of (current_thread);
while (addr < memaddr + len)
{
@@ -742,7 +742,7 @@ lynx_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, int len)
static void
lynx_request_interrupt (void)
{
- ptid_t inferior_ptid = thread_to_gdb_id (get_first_thread ());
+ ptid_t inferior_ptid = ptid_of (get_first_thread ());
kill (lynx_ptid_get_pid (inferior_ptid), SIGINT);
}
diff --git a/gdb/gdbserver/nto-low.c b/gdb/gdbserver/nto-low.c
index a5f1543..01e951c 100644
--- a/gdb/gdbserver/nto-low.c
+++ b/gdb/gdbserver/nto-low.c
@@ -631,7 +631,7 @@ nto_fetch_registers (struct regcache *regcache, int regno)
TRACE ("current_thread is NULL\n");
return;
}
- ptid = thread_to_gdb_id (current_thread);
+ ptid = ptid_of (current_thread);
if (!nto_set_thread (ptid))
return;
@@ -678,7 +678,7 @@ nto_store_registers (struct regcache *regcache, int regno)
TRACE ("current_thread is NULL\n");
return;
}
- ptid = thread_to_gdb_id (current_thread);
+ ptid = ptid_of (current_thread);
if (!nto_set_thread (ptid))
return;
@@ -869,7 +869,7 @@ nto_stopped_by_watchpoint (void)
{
ptid_t ptid;
- ptid = thread_to_gdb_id (current_thread);
+ ptid = ptid_of (current_thread);
if (nto_set_thread (ptid))
{
const int watchmask = _DEBUG_FLAG_TRACE_RD | _DEBUG_FLAG_TRACE_WR
@@ -901,7 +901,7 @@ nto_stopped_data_address (void)
{
ptid_t ptid;
- ptid = thread_to_gdb_id (current_thread);
+ ptid = ptid_of (current_thread);
if (nto_set_thread (ptid))
{
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 3ea24ff..bca5340 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -1586,7 +1586,7 @@ handle_qxfer_threads_worker (struct inferior_list_entry *inf, void *arg)
{
struct thread_info *thread = (struct thread_info *) inf;
struct buffer *buffer = (struct buffer *) arg;
- ptid_t ptid = thread_to_gdb_id (thread);
+ ptid_t ptid = ptid_of (thread);
char ptid_s[100];
int core = target_core_of_thread (ptid);
char core_s[21];
@@ -2070,21 +2070,21 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
/* Reply the current thread id. */
if (strcmp ("qC", own_buf) == 0 && !disable_packet_qC)
{
- ptid_t gdb_id;
+ ptid_t ptid;
require_running (own_buf);
if (!ptid_equal (general_thread, null_ptid)
&& !ptid_equal (general_thread, minus_one_ptid))
- gdb_id = general_thread;
+ ptid = general_thread;
else
{
thread_ptr = get_first_inferior (&all_threads);
- gdb_id = thread_to_gdb_id ((struct thread_info *)thread_ptr);
+ ptid = thread_ptr->id;
}
sprintf (own_buf, "QC");
own_buf += 2;
- write_ptid (own_buf, gdb_id);
+ write_ptid (own_buf, ptid);
return;
}
@@ -2140,28 +2140,24 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
{
if (strcmp ("qfThreadInfo", own_buf) == 0)
{
- ptid_t gdb_id;
-
require_running (own_buf);
thread_ptr = get_first_inferior (&all_threads);
*own_buf++ = 'm';
- gdb_id = thread_to_gdb_id ((struct thread_info *)thread_ptr);
- write_ptid (own_buf, gdb_id);
+ ptid_t ptid = thread_ptr->id;
+ write_ptid (own_buf, ptid);
thread_ptr = thread_ptr->next;
return;
}
if (strcmp ("qsThreadInfo", own_buf) == 0)
{
- ptid_t gdb_id;
-
require_running (own_buf);
if (thread_ptr != NULL)
{
*own_buf++ = 'm';
- gdb_id = thread_to_gdb_id ((struct thread_info *)thread_ptr);
- write_ptid (own_buf, gdb_id);
+ ptid_t ptid = thread_ptr->id;
+ write_ptid (own_buf, ptid);
thread_ptr = thread_ptr->next;
return;
}
--
2.7.4
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 3/3] gdbserver: Remove thread_to_gdb_id
2017-09-10 20:11 ` [PATCH 3/3] gdbserver: Remove thread_to_gdb_id Simon Marchi
@ 2017-09-15 11:02 ` Pedro Alves
2017-09-15 13:54 ` Simon Marchi
0 siblings, 1 reply; 13+ messages in thread
From: Pedro Alves @ 2017-09-15 11:02 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
On 09/10/2017 09:11 PM, Simon Marchi wrote:
> As explained in the previous patch, the gdb_id concept is no longer
> relevant. The function thread_to_gdb_id is trivial, it returns the
> thread's ptid. Remove it and replace its usage with ptid_of.
>
> The changes in nto-low.c and lynx-low.c are fairly straightforward, but
> I was not able to build test them.
Seems fine to me. Nits below.
> @@ -869,7 +869,7 @@ nto_stopped_by_watchpoint (void)
> {
> ptid_t ptid;
>
> - ptid = thread_to_gdb_id (current_thread);
> + ptid = ptid_of (current_thread);
> if (nto_set_thread (ptid))
> {
> const int watchmask = _DEBUG_FLAG_TRACE_RD | _DEBUG_FLAG_TRACE_WR
> @@ -901,7 +901,7 @@ nto_stopped_data_address (void)
> {
> ptid_t ptid;
>
> - ptid = thread_to_gdb_id (current_thread);
> + ptid = ptid_of (current_thread);
>
Above two hunks could merge decl + init.
> @@ -2070,21 +2070,21 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
> /* Reply the current thread id. */
> if (strcmp ("qC", own_buf) == 0 && !disable_packet_qC)
> {
> - ptid_t gdb_id;
> + ptid_t ptid;
> require_running (own_buf);
>
> if (!ptid_equal (general_thread, null_ptid)
> && !ptid_equal (general_thread, minus_one_ptid))
In the other patch you converted equivalent checks to use op!= ,
I don't mind doing it here as well while at it:
if (general_thread != null_ptid
&& general_thread != minus_one_ptid)
> - gdb_id = general_thread;
> + ptid = general_thread;
> else
> {
> thread_ptr = get_first_inferior (&all_threads);
> - gdb_id = thread_to_gdb_id ((struct thread_info *)thread_ptr);
> + ptid = thread_ptr->id;
> }
>
> sprintf (own_buf, "QC");
> own_buf += 2;
> - write_ptid (own_buf, gdb_id);
> + write_ptid (own_buf, ptid);
> return;
> }
>
> @@ -2140,28 +2140,24 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
> {
> if (strcmp ("qfThreadInfo", own_buf) == 0)
> {
> - ptid_t gdb_id;
> -
> require_running (own_buf);
> thread_ptr = get_first_inferior (&all_threads);
>
> *own_buf++ = 'm';
> - gdb_id = thread_to_gdb_id ((struct thread_info *)thread_ptr);
> - write_ptid (own_buf, gdb_id);
> + ptid_t ptid = thread_ptr->id;
> + write_ptid (own_buf, ptid);
Could even get rid of the ptid_t variable?
write_ptid (own_buf, thread_ptr->id);
> thread_ptr = thread_ptr->next;
> return;
> }
>
> if (strcmp ("qsThreadInfo", own_buf) == 0)
> {
> - ptid_t gdb_id;
> -
> require_running (own_buf);
> if (thread_ptr != NULL)
> {
> *own_buf++ = 'm';
> - gdb_id = thread_to_gdb_id ((struct thread_info *)thread_ptr);
> - write_ptid (own_buf, gdb_id);
> + ptid_t ptid = thread_ptr->id;
> + write_ptid (own_buf, ptid);
Ditto.
> thread_ptr = thread_ptr->next;
> return;
> }
>
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 3/3] gdbserver: Remove thread_to_gdb_id
2017-09-15 11:02 ` Pedro Alves
@ 2017-09-15 13:54 ` Simon Marchi
2017-09-15 14:00 ` Pedro Alves
0 siblings, 1 reply; 13+ messages in thread
From: Simon Marchi @ 2017-09-15 13:54 UTC (permalink / raw)
To: Pedro Alves; +Cc: Simon Marchi, gdb-patches
On 2017-09-15 13:02, Pedro Alves wrote:
> On 09/10/2017 09:11 PM, Simon Marchi wrote:
>> As explained in the previous patch, the gdb_id concept is no longer
>> relevant. The function thread_to_gdb_id is trivial, it returns the
>> thread's ptid. Remove it and replace its usage with ptid_of.
>>
>> The changes in nto-low.c and lynx-low.c are fairly straightforward,
>> but
>> I was not able to build test them.
>
> Seems fine to me. Nits below.
>
>> @@ -869,7 +869,7 @@ nto_stopped_by_watchpoint (void)
>> {
>> ptid_t ptid;
>>
>> - ptid = thread_to_gdb_id (current_thread);
>> + ptid = ptid_of (current_thread);
>> if (nto_set_thread (ptid))
>> {
>> const int watchmask = _DEBUG_FLAG_TRACE_RD | _DEBUG_FLAG_TRACE_WR
>> @@ -901,7 +901,7 @@ nto_stopped_data_address (void)
>> {
>> ptid_t ptid;
>>
>> - ptid = thread_to_gdb_id (current_thread);
>> + ptid = ptid_of (current_thread);
>>
>
> Above two hunks could merge decl + init.
In the files I can't build, I usually try to be as unintrusive as
possible, but I'm fine with doing this, it's obvious (tm).
>> @@ -2070,21 +2070,21 @@ handle_query (char *own_buf, int packet_len,
>> int *new_packet_len_p)
>> /* Reply the current thread id. */
>> if (strcmp ("qC", own_buf) == 0 && !disable_packet_qC)
>> {
>> - ptid_t gdb_id;
>> + ptid_t ptid;
>> require_running (own_buf);
>>
>> if (!ptid_equal (general_thread, null_ptid)
>> && !ptid_equal (general_thread, minus_one_ptid))
>
> In the other patch you converted equivalent checks to use op!= ,
> I don't mind doing it here as well while at it:
>
> if (general_thread != null_ptid
> && general_thread != minus_one_ptid)
>
>
>> - gdb_id = general_thread;
>> + ptid = general_thread;
>> else
>> {
>> thread_ptr = get_first_inferior (&all_threads);
>> - gdb_id = thread_to_gdb_id ((struct thread_info *)thread_ptr);
>> + ptid = thread_ptr->id;
>> }
>>
>> sprintf (own_buf, "QC");
>> own_buf += 2;
>> - write_ptid (own_buf, gdb_id);
>> + write_ptid (own_buf, ptid);
>> return;
>> }
>>
>> @@ -2140,28 +2140,24 @@ handle_query (char *own_buf, int packet_len,
>> int *new_packet_len_p)
>> {
>> if (strcmp ("qfThreadInfo", own_buf) == 0)
>> {
>> - ptid_t gdb_id;
>> -
>> require_running (own_buf);
>> thread_ptr = get_first_inferior (&all_threads);
>>
>> *own_buf++ = 'm';
>> - gdb_id = thread_to_gdb_id ((struct thread_info *)thread_ptr);
>> - write_ptid (own_buf, gdb_id);
>> + ptid_t ptid = thread_ptr->id;
>> + write_ptid (own_buf, ptid);
>
> Could even get rid of the ptid_t variable?
>
> write_ptid (own_buf, thread_ptr->id);
>
>> thread_ptr = thread_ptr->next;
>> return;
>> }
>>
>> if (strcmp ("qsThreadInfo", own_buf) == 0)
>> {
>> - ptid_t gdb_id;
>> -
>> require_running (own_buf);
>> if (thread_ptr != NULL)
>> {
>> *own_buf++ = 'm';
>> - gdb_id = thread_to_gdb_id ((struct thread_info *)thread_ptr);
>> - write_ptid (own_buf, gdb_id);
>> + ptid_t ptid = thread_ptr->id;
>> + write_ptid (own_buf, ptid);
>
> Ditto.
>
>> thread_ptr = thread_ptr->next;
>> return;
>> }
>>
Ack everything. The changes are small enough that I don't think it
warrants a v2. I'll push directly once the handle_detach refactor is in
(and post the final versions here).
Thanks!
Simon
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 3/3] gdbserver: Remove thread_to_gdb_id
2017-09-15 13:54 ` Simon Marchi
@ 2017-09-15 14:00 ` Pedro Alves
0 siblings, 0 replies; 13+ messages in thread
From: Pedro Alves @ 2017-09-15 14:00 UTC (permalink / raw)
To: Simon Marchi; +Cc: Simon Marchi, gdb-patches
On 09/15/2017 02:54 PM, Simon Marchi wrote:
>
> Ack everything. The changes are small enough that I don't think it
> warrants a v2. I'll push directly once the handle_detach refactor is in
> (and post the final versions here).
>
> Thanks!
Agreed, that's fine with me.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] gdbserver: Remove gdb_id_to_thread_id
2017-09-10 20:11 [PATCH 0/3] Small cleanups in gdbserver Simon Marchi
2017-09-10 20:11 ` [PATCH 3/3] gdbserver: Remove thread_to_gdb_id Simon Marchi
@ 2017-09-10 20:11 ` Simon Marchi
2017-09-15 10:55 ` Pedro Alves
2017-09-10 20:11 ` [PATCH 1/3] gdbserver: Remove duplicate functions to find any thread of process Simon Marchi
2 siblings, 1 reply; 13+ messages in thread
From: Simon Marchi @ 2017-09-10 20:11 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
From what I understand, this function is not doing anything useful as of
today.
Here's the result of my archeological research:
- The field thread_info::gdb_id was added in
a06660f7 Use LWP IDs for thread IDs in gdbserver
There was problem when using a 32-bits gdb with a 64-bits gdbserver.
For some reason that I don't fully understand, the thread ids
exchanged between gdb and gdbserver could overflow a 32 bits data
type. My guess is that they were the thread address (e.g. the
0x7ffff7f20b40 in "Thread 0x7ffff7f20b40 (LWP 1058)" today). This
patch changed that so gdb/gdbserver would talk in terms of the OS
assigned numerical id (as shown in ps). It therefore added a way to
convert between this gdb_id (the numerical id) and the thread id (the
address).
- 95954743cb Implement the multiprocess extensions, and add linux
multiprocess supportNon-stop mode support.
This patch made gdbserver deal with threads using their numerical ids
and not the address-like id. Starting from there, the gdb_id <->
thread id conversion was not needed anymore, since the remote protocol
and gdbserver were using the same kind of ids again. The gdb_id field
in the thread_info structure was also unused starting there.
- d50171e4 Teach linux gdbserver to step-over-breakpoints.
This patch moved the thread_info structure around, and got rid of the
gdb_id field (which was unused).
Looking at the implementation of gdb_id_to_thread_id, it is not doing
anything useful. It is looking up a thread by ptid using
find_thread_ptid, which basically loops over all threads looking at
their entry.id field. If a thread with that ptid is found, it returns
its entry.id field. So it will always return the same thing as it input
(with the exception of if no thread exist with that ptid, then it will
return null_ptid).
gdb/gdbserver/ChangeLog:
* inferiors.h (gdb_id_to_thread_id): Remove.
* inferiors.c (gdb_id_to_thread_id): Remove.
* server.c (process_serial_event): Adjust to gdb_id_to_thread_id
removal. Move pid declaration closer to where it's used.
---
gdb/gdbserver/inferiors.c | 8 --
gdb/gdbserver/inferiors.h | 1 -
gdb/gdbserver/server.c | 183 +++++++++++++++++++++++-----------------------
3 files changed, 90 insertions(+), 102 deletions(-)
diff --git a/gdb/gdbserver/inferiors.c b/gdb/gdbserver/inferiors.c
index 2212850..933dd76 100644
--- a/gdb/gdbserver/inferiors.c
+++ b/gdb/gdbserver/inferiors.c
@@ -173,14 +173,6 @@ find_any_thread_of_pid (int pid)
return (struct thread_info *) entry;
}
-ptid_t
-gdb_id_to_thread_id (ptid_t gdb_id)
-{
- struct thread_info *thread = find_thread_ptid (gdb_id);
-
- return thread ? thread->entry.id : null_ptid;
-}
-
static void
free_one_thread (struct inferior_list_entry *inf)
{
diff --git a/gdb/gdbserver/inferiors.h b/gdb/gdbserver/inferiors.h
index f229e67..6316fe5 100644
--- a/gdb/gdbserver/inferiors.h
+++ b/gdb/gdbserver/inferiors.h
@@ -144,7 +144,6 @@ int have_started_inferiors_p (void);
int have_attached_inferiors_p (void);
ptid_t thread_to_gdb_id (struct thread_info *);
-ptid_t gdb_id_to_thread_id (ptid_t);
void clear_inferiors (void);
struct inferior_list_entry *find_inferior
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index bedb87b..3ea24ff 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -4011,7 +4011,6 @@ process_serial_event (void)
unsigned int len;
int res;
CORE_ADDR mem_addr;
- int pid;
unsigned char sig;
int packet_len;
int new_packet_len = -1;
@@ -4039,92 +4038,96 @@ process_serial_event (void)
handle_general_set (own_buf);
break;
case 'D':
- require_running (own_buf);
+ {
+ require_running (own_buf);
- if (multi_process)
- {
- i++; /* skip ';' */
- pid = strtol (&own_buf[i], NULL, 16);
- }
- else
- pid = ptid_get_pid (current_ptid);
+ int pid;
- if ((tracing && disconnected_tracing) || any_persistent_commands ())
- {
- struct process_info *process = find_process_pid (pid);
+ if (multi_process)
+ {
+ i++; /* skip ';' */
+ pid = strtol (&own_buf[i], NULL, 16);
+ }
+ else
+ pid = ptid_get_pid (current_ptid);
- if (process == NULL)
- {
- write_enn (own_buf);
- break;
- }
+ if ((tracing && disconnected_tracing) || any_persistent_commands ())
+ {
+ struct process_info *process = find_process_pid (pid);
- if (tracing && disconnected_tracing)
- fprintf (stderr,
- "Disconnected tracing in effect, "
- "leaving gdbserver attached to the process\n");
-
- if (any_persistent_commands ())
- fprintf (stderr,
- "Persistent commands are present, "
- "leaving gdbserver attached to the process\n");
-
- /* Make sure we're in non-stop/async mode, so we we can both
- wait for an async socket accept, and handle async target
- events simultaneously. There's also no point either in
- having the target stop all threads, when we're going to
- pass signals down without informing GDB. */
- if (!non_stop)
- {
- if (debug_threads)
- debug_printf ("Forcing non-stop mode\n");
+ if (process == NULL)
+ {
+ write_enn (own_buf);
+ break;
+ }
- non_stop = 1;
- start_non_stop (1);
- }
+ if (tracing && disconnected_tracing)
+ fprintf (stderr,
+ "Disconnected tracing in effect, "
+ "leaving gdbserver attached to the process\n");
+
+ if (any_persistent_commands ())
+ fprintf (stderr,
+ "Persistent commands are present, "
+ "leaving gdbserver attached to the process\n");
+
+ /* Make sure we're in non-stop/async mode, so we we can both
+ wait for an async socket accept, and handle async target
+ events simultaneously. There's also no point either in
+ having the target stop all threads, when we're going to
+ pass signals down without informing GDB. */
+ if (!non_stop)
+ {
+ if (debug_threads)
+ debug_printf ("Forcing non-stop mode\n");
- process->gdb_detached = 1;
+ non_stop = 1;
+ start_non_stop (1);
+ }
- /* Detaching implicitly resumes all threads. */
- target_continue_no_signal (minus_one_ptid);
+ process->gdb_detached = 1;
- write_ok (own_buf);
- break; /* from switch/case */
- }
+ /* Detaching implicitly resumes all threads. */
+ target_continue_no_signal (minus_one_ptid);
- fprintf (stderr, "Detaching from process %d\n", pid);
- stop_tracing ();
- if (detach_inferior (pid) != 0)
- write_enn (own_buf);
- else
- {
- discard_queued_stop_replies (pid_to_ptid (pid));
- write_ok (own_buf);
-
- if (extended_protocol || target_running ())
- {
- /* There is still at least one inferior remaining or
- we are in extended mode, so don't terminate gdbserver,
- and instead treat this like a normal program exit. */
- last_status.kind = TARGET_WAITKIND_EXITED;
- last_status.value.integer = 0;
- last_ptid = pid_to_ptid (pid);
+ write_ok (own_buf);
+ break; /* from switch/case */
+ }
- current_thread = NULL;
- }
- else
- {
- putpkt (own_buf);
- remote_close ();
+ fprintf (stderr, "Detaching from process %d\n", pid);
+ stop_tracing ();
+ if (detach_inferior (pid) != 0)
+ write_enn (own_buf);
+ else
+ {
+ discard_queued_stop_replies (pid_to_ptid (pid));
+ write_ok (own_buf);
- /* If we are attached, then we can exit. Otherwise, we
- need to hang around doing nothing, until the child is
- gone. */
- join_inferior (pid);
- exit (0);
- }
- }
- break;
+ if (extended_protocol || target_running ())
+ {
+ /* There is still at least one inferior remaining or
+ we are in extended mode, so don't terminate gdbserver,
+ and instead treat this like a normal program exit. */
+ last_status.kind = TARGET_WAITKIND_EXITED;
+ last_status.value.integer = 0;
+ last_ptid = pid_to_ptid (pid);
+
+ current_thread = NULL;
+ }
+ else
+ {
+ putpkt (own_buf);
+ remote_close ();
+
+ /* If we are attached, then we can exit. Otherwise, we
+ need to hang around doing nothing, until the child is
+ gone. */
+ join_inferior (pid);
+ exit (0);
+ }
+ }
+ break;
+ }
case '!':
extended_protocol = 1;
write_ok (own_buf);
@@ -4135,23 +4138,18 @@ process_serial_event (void)
case 'H':
if (own_buf[1] == 'c' || own_buf[1] == 'g' || own_buf[1] == 's')
{
- ptid_t gdb_id, thread_id;
- int pid;
+ ptid_t thread_id;
require_running (own_buf);
- gdb_id = read_ptid (&own_buf[2], NULL);
-
- pid = ptid_get_pid (gdb_id);
+ thread_id = read_ptid (&own_buf[2], NULL);
- if (ptid_equal (gdb_id, null_ptid)
- || ptid_equal (gdb_id, minus_one_ptid))
+ if (thread_id == null_ptid || thread_id == minus_one_ptid)
thread_id = null_ptid;
- else if (pid != 0
- && ptid_equal (pid_to_ptid (pid),
- gdb_id))
+ else if (thread_id.is_pid ())
{
- thread_info *thread = find_any_thread_of_pid (pid);
+ /* The ptid represents a pid. */
+ thread_info *thread = find_any_thread_of_pid (thread_id.pid ());
if (thread == NULL)
{
@@ -4163,8 +4161,8 @@ process_serial_event (void)
}
else
{
- thread_id = gdb_id_to_thread_id (gdb_id);
- if (ptid_equal (thread_id, null_ptid))
+ /* The ptid represents a lwp/tid. */
+ if (find_thread_ptid (thread_id) == NULL)
{
write_enn (own_buf);
break;
@@ -4369,13 +4367,12 @@ process_serial_event (void)
case 'T':
{
- ptid_t gdb_id, thread_id;
+ ptid_t thread_id;
require_running (own_buf);
- gdb_id = read_ptid (&own_buf[1], NULL);
- thread_id = gdb_id_to_thread_id (gdb_id);
- if (ptid_equal (thread_id, null_ptid))
+ thread_id = read_ptid (&own_buf[1], NULL);
+ if (find_thread_ptid (thread_id) == NULL)
{
write_enn (own_buf);
break;
--
2.7.4
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 2/3] gdbserver: Remove gdb_id_to_thread_id
2017-09-10 20:11 ` [PATCH 2/3] gdbserver: Remove gdb_id_to_thread_id Simon Marchi
@ 2017-09-15 10:55 ` Pedro Alves
2017-09-15 13:07 ` Simon Marchi
2017-09-15 13:44 ` Simon Marchi
0 siblings, 2 replies; 13+ messages in thread
From: Pedro Alves @ 2017-09-15 10:55 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
On 09/10/2017 09:11 PM, Simon Marchi wrote:
> From what I understand, this function is not doing anything useful as of
> today.
>
> Here's the result of my archeological research:
>
*nod*
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -4011,7 +4011,6 @@ process_serial_event (void)
> unsigned int len;
> int res;
> CORE_ADDR mem_addr;
> - int pid;
> unsigned char sig;
> int packet_len;
> int new_packet_len = -1;
> @@ -4039,92 +4038,96 @@ process_serial_event (void)
> handle_general_set (own_buf);
> break;
> case 'D':
> - require_running (own_buf);
> + {
> + require_running (own_buf);
>
The reindentation makes it hard to see the actual
change. Is it just moving the int pid variable, or something else?
IMO, it'd be nicer to move the whole case 'D' body to a
handle_detach function.
> case '!':
> extended_protocol = 1;
> write_ok (own_buf);
> @@ -4135,23 +4138,18 @@ process_serial_event (void)
> case 'H':
> if (own_buf[1] == 'c' || own_buf[1] == 'g' || own_buf[1] == 's')
> {
> - ptid_t gdb_id, thread_id;
> - int pid;
> + ptid_t thread_id;
>
> require_running (own_buf);
>
> - gdb_id = read_ptid (&own_buf[2], NULL);
> -
> - pid = ptid_get_pid (gdb_id);
> + thread_id = read_ptid (&own_buf[2], NULL);
Move ptid_t declaration here? :
ptid_t thread_id = read_ptid (&own_buf[2], NULL);
>
> - if (ptid_equal (gdb_id, null_ptid)
> - || ptid_equal (gdb_id, minus_one_ptid))
> + if (thread_id == null_ptid || thread_id == minus_one_ptid)
> thread_id = null_ptid;
This can be just:
if (thread_id == minus_one_ptid)
thread_id = null_ptid;
> - else if (pid != 0
> - && ptid_equal (pid_to_ptid (pid),
> - gdb_id))
> + else if (thread_id.is_pid ())
> {
> - thread_info *thread = find_any_thread_of_pid (pid);
> + /* The ptid represents a pid. */
> + thread_info *thread = find_any_thread_of_pid (thread_id.pid ());
>
> if (thread == NULL)
> {
> @@ -4163,8 +4161,8 @@ process_serial_event (void)
> }
> else
> {
> - thread_id = gdb_id_to_thread_id (gdb_id);
> - if (ptid_equal (thread_id, null_ptid))
> + /* The ptid represents a lwp/tid. */
> + if (find_thread_ptid (thread_id) == NULL)
> {
> write_enn (own_buf);
> break;
> @@ -4369,13 +4367,12 @@ process_serial_event (void)
>
> case 'T':
> {
> - ptid_t gdb_id, thread_id;
> + ptid_t thread_id;
>
> require_running (own_buf);
>
> - gdb_id = read_ptid (&own_buf[1], NULL);
> - thread_id = gdb_id_to_thread_id (gdb_id);
> - if (ptid_equal (thread_id, null_ptid))
> + thread_id = read_ptid (&own_buf[1], NULL);
ptid_t thread_id = ... ?
> + if (find_thread_ptid (thread_id) == NULL)
> {
> write_enn (own_buf);
> break;
>
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 2/3] gdbserver: Remove gdb_id_to_thread_id
2017-09-15 10:55 ` Pedro Alves
@ 2017-09-15 13:07 ` Simon Marchi
2017-09-15 13:34 ` Simon Marchi
2017-09-15 13:44 ` Simon Marchi
1 sibling, 1 reply; 13+ messages in thread
From: Simon Marchi @ 2017-09-15 13:07 UTC (permalink / raw)
To: Pedro Alves; +Cc: Simon Marchi, gdb-patches
On 2017-09-15 12:55, Pedro Alves wrote:
> On 09/10/2017 09:11 PM, Simon Marchi wrote:
>> From what I understand, this function is not doing anything useful as
>> of
>> today.
>>
>> Here's the result of my archeological research:
>>
>
> *nod*
>
>> --- a/gdb/gdbserver/server.c
>> +++ b/gdb/gdbserver/server.c
>> @@ -4011,7 +4011,6 @@ process_serial_event (void)
>> unsigned int len;
>> int res;
>> CORE_ADDR mem_addr;
>> - int pid;
>> unsigned char sig;
>> int packet_len;
>> int new_packet_len = -1;
>> @@ -4039,92 +4038,96 @@ process_serial_event (void)
>> handle_general_set (own_buf);
>> break;
>> case 'D':
>> - require_running (own_buf);
>> + {
>> + require_running (own_buf);
>>
>
> The reindentation makes it hard to see the actual
> change. Is it just moving the int pid variable, or something else?
> IMO, it'd be nicer to move the whole case 'D' body to a
> handle_detach function.
Agreed, I'll do a preparatory patch that does that, and then a second
version of this one (also taking into account all your other comments).
Thanks,
Simon
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 2/3] gdbserver: Remove gdb_id_to_thread_id
2017-09-15 13:07 ` Simon Marchi
@ 2017-09-15 13:34 ` Simon Marchi
0 siblings, 0 replies; 13+ messages in thread
From: Simon Marchi @ 2017-09-15 13:34 UTC (permalink / raw)
To: Pedro Alves; +Cc: Simon Marchi, gdb-patches
On 2017-09-15 15:07, Simon Marchi wrote:
> On 2017-09-15 12:55, Pedro Alves wrote:
>> On 09/10/2017 09:11 PM, Simon Marchi wrote:
>>> From what I understand, this function is not doing anything useful as
>>> of
>>> today.
>>>
>>> Here's the result of my archeological research:
>>>
>>
>> *nod*
>>
>>> --- a/gdb/gdbserver/server.c
>>> +++ b/gdb/gdbserver/server.c
>>> @@ -4011,7 +4011,6 @@ process_serial_event (void)
>>> unsigned int len;
>>> int res;
>>> CORE_ADDR mem_addr;
>>> - int pid;
>>> unsigned char sig;
>>> int packet_len;
>>> int new_packet_len = -1;
>>> @@ -4039,92 +4038,96 @@ process_serial_event (void)
>>> handle_general_set (own_buf);
>>> break;
>>> case 'D':
>>> - require_running (own_buf);
>>> + {
>>> + require_running (own_buf);
>>>
>>
>> The reindentation makes it hard to see the actual
>> change. Is it just moving the int pid variable, or something else?
>> IMO, it'd be nicer to move the whole case 'D' body to a
>> handle_detach function.
>
> Agreed, I'll do a preparatory patch that does that, and then a second
> version of this one (also taking into account all your other
> comments).
I realized I did not answer you question. Yes it was just moving the
pid variable.
Simon
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] gdbserver: Remove gdb_id_to_thread_id
2017-09-15 10:55 ` Pedro Alves
2017-09-15 13:07 ` Simon Marchi
@ 2017-09-15 13:44 ` Simon Marchi
1 sibling, 0 replies; 13+ messages in thread
From: Simon Marchi @ 2017-09-15 13:44 UTC (permalink / raw)
To: Pedro Alves; +Cc: Simon Marchi, gdb-patches
On 2017-09-15 12:55, Pedro Alves wrote:
>>
>> - if (ptid_equal (gdb_id, null_ptid)
>> - || ptid_equal (gdb_id, minus_one_ptid))
>> + if (thread_id == null_ptid || thread_id == minus_one_ptid)
>> thread_id = null_ptid;
>
> This can be just:
>
> if (thread_id == minus_one_ptid)
> thread_id = null_ptid;
I thought that too at first, but actually if thread_id == null_ptid,
execution will take the "else" branch and things will break after.
Simon
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] gdbserver: Remove duplicate functions to find any thread of process
2017-09-10 20:11 [PATCH 0/3] Small cleanups in gdbserver Simon Marchi
2017-09-10 20:11 ` [PATCH 3/3] gdbserver: Remove thread_to_gdb_id Simon Marchi
2017-09-10 20:11 ` [PATCH 2/3] gdbserver: Remove gdb_id_to_thread_id Simon Marchi
@ 2017-09-10 20:11 ` Simon Marchi
2017-09-15 4:09 ` Sergio Durigan Junior
2 siblings, 1 reply; 13+ messages in thread
From: Simon Marchi @ 2017-09-10 20:11 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
We have about 6 functions/callbacks to find_inferior meant to find a
thread that belongs to a given pid. Remove all but
find_any_thread_of_pid and replace their uses with
find_any_thread_of_pid.
gdbserver/ChangeLog:
* server.c (first_thread_of): Remove.
(process_serial_event): Replace usage of first_thread_of with
find_any_thread_of_pid.
* tracepoint.c (same_process_p): Remove.
(gdb_agent_about_to_close): Replace usage of same_process_p with
find_any_thread_of_pid.
* linux-x86-low.c (same_process_callback): Remove.
(x86_arch_setup_process_callback): Replace usage of
same_process_callback with find_any_thread_of_pid.
* thread-db.c (any_thread_of): Remove.
(switch_to_process): Replace usage of any_thread_of with
find_any_thread_of_pid.
* inferiors.c (thread_pid_matches_callback): Remove.
(find_thread_process): Adjust to use find_any_thread_of_pid.
---
gdb/gdbserver/inferiors.c | 14 +-------------
gdb/gdbserver/linux-x86-low.c | 15 +--------------
gdb/gdbserver/server.c | 19 +++----------------
gdb/gdbserver/thread-db.c | 15 +--------------
gdb/gdbserver/tracepoint.c | 14 +-------------
5 files changed, 7 insertions(+), 70 deletions(-)
diff --git a/gdb/gdbserver/inferiors.c b/gdb/gdbserver/inferiors.c
index 3c171a1..2212850 100644
--- a/gdb/gdbserver/inferiors.c
+++ b/gdb/gdbserver/inferiors.c
@@ -141,25 +141,13 @@ 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)
{
- pid_t pid = ptid_get_pid (ptid_of (process));
-
- return (struct thread_info *)
- find_inferior (&all_threads, thread_pid_matches_callback, &pid);
+ return find_any_thread_of_pid (process->entry.id.pid ());
}
/* Helper for find_any_thread_of_pid. Returns true if a thread
diff --git a/gdb/gdbserver/linux-x86-low.c b/gdb/gdbserver/linux-x86-low.c
index 844a165..49f6b2d 100644
--- a/gdb/gdbserver/linux-x86-low.c
+++ b/gdb/gdbserver/linux-x86-low.c
@@ -846,17 +846,6 @@ x86_linux_read_description (void)
gdb_assert_not_reached ("failed to return tdesc");
}
-/* Callback for find_inferior. Stops iteration when a thread with a
- given PID is found. */
-
-static int
-same_process_callback (struct inferior_list_entry *entry, void *data)
-{
- int pid = *(int *) data;
-
- return (ptid_get_pid (entry->id) == pid);
-}
-
/* Callback for for_each_inferior. Calls the arch_setup routine for
each process. */
@@ -866,9 +855,7 @@ x86_arch_setup_process_callback (struct inferior_list_entry *entry)
int pid = ptid_get_pid (entry->id);
/* Look up any thread of this processes. */
- current_thread
- = (struct thread_info *) find_inferior (&all_threads,
- same_process_callback, &pid);
+ current_thread = find_any_thread_of_pid (pid);
the_low_target.arch_setup ();
}
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 56c6393..bedb87b 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -3450,17 +3450,6 @@ gdbserver_show_disableable (FILE *stream)
break; \
}
-static int
-first_thread_of (struct inferior_list_entry *entry, void *args)
-{
- int pid = * (int *) args;
-
- if (ptid_get_pid (entry->id) == pid)
- return 1;
-
- return 0;
-}
-
static void
kill_inferior_callback (struct inferior_list_entry *entry)
{
@@ -4162,11 +4151,9 @@ process_serial_event (void)
&& ptid_equal (pid_to_ptid (pid),
gdb_id))
{
- struct thread_info *thread =
- (struct thread_info *) find_inferior (&all_threads,
- first_thread_of,
- &pid);
- if (!thread)
+ thread_info *thread = find_any_thread_of_pid (pid);
+
+ if (thread == NULL)
{
write_enn (own_buf);
break;
diff --git a/gdb/gdbserver/thread-db.c b/gdb/gdbserver/thread-db.c
index 1ffb79d..9b867b8 100644
--- a/gdb/gdbserver/thread-db.c
+++ b/gdb/gdbserver/thread-db.c
@@ -733,25 +733,12 @@ thread_db_init (void)
return 0;
}
-static int
-any_thread_of (struct inferior_list_entry *entry, void *args)
-{
- int *pid_p = (int *) args;
-
- if (ptid_get_pid (entry->id) == *pid_p)
- return 1;
-
- return 0;
-}
-
static void
switch_to_process (struct process_info *proc)
{
int pid = pid_of (proc);
- current_thread =
- (struct thread_info *) find_inferior (&all_threads,
- any_thread_of, &pid);
+ current_thread = find_any_thread_of_pid (pid);
}
/* Disconnect from libthread_db and free resources. */
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 68ce10f..0f41ff4 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -3956,17 +3956,6 @@ cmd_qtstmat (char *packet)
run_inferior_command (packet, strlen (packet) + 1);
}
-/* Helper for gdb_agent_about_to_close.
- Return non-zero if thread ENTRY is in the same process in DATA. */
-
-static int
-same_process_p (struct inferior_list_entry *entry, void *data)
-{
- int *pid = (int *) data;
-
- return ptid_get_pid (entry->id) == *pid;
-}
-
/* Sent the agent a command to close it. */
void
@@ -3981,8 +3970,7 @@ gdb_agent_about_to_close (int pid)
saved_thread = current_thread;
/* Find any thread which belongs to process PID. */
- current_thread = (struct thread_info *)
- find_inferior (&all_threads, same_process_p, &pid);
+ current_thread = find_any_thread_of_pid (pid);
strcpy (buf, "close");
--
2.7.4
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 1/3] gdbserver: Remove duplicate functions to find any thread of process
2017-09-10 20:11 ` [PATCH 1/3] gdbserver: Remove duplicate functions to find any thread of process Simon Marchi
@ 2017-09-15 4:09 ` Sergio Durigan Junior
2017-09-15 12:53 ` Simon Marchi
0 siblings, 1 reply; 13+ messages in thread
From: Sergio Durigan Junior @ 2017-09-15 4:09 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches
On Sunday, September 10 2017, Simon Marchi wrote:
> We have about 6 functions/callbacks to find_inferior meant to find a
> thread that belongs to a given pid. Remove all but
> find_any_thread_of_pid and replace their uses with
> find_any_thread_of_pid.
Thanks for doing. gdbserver is really confusing when dealing with
threads and their data structures, so this patch is a nice step towards
a better interface.
I think this can go in as is, because it is a self contained cleanup,
but I would like us to move yet another step ahead: recently I've
implemente gdbserver's version of "switch_to_thread", and I couldn't
help but think that a "switch_to_any_thread_of_pid" would be good to
have, because that's what's happening most of the time in this patch.
I know I have to bite the bullet and convert places where
"current_thread = find_thread_ptid (ptid)" is still being used so that
"switch_to_thread" is used globally. I intend to submit a patch for
that.
Anyway, this looks good to me.
> gdbserver/ChangeLog:
>
> * server.c (first_thread_of): Remove.
> (process_serial_event): Replace usage of first_thread_of with
> find_any_thread_of_pid.
> * tracepoint.c (same_process_p): Remove.
> (gdb_agent_about_to_close): Replace usage of same_process_p with
> find_any_thread_of_pid.
> * linux-x86-low.c (same_process_callback): Remove.
> (x86_arch_setup_process_callback): Replace usage of
> same_process_callback with find_any_thread_of_pid.
> * thread-db.c (any_thread_of): Remove.
> (switch_to_process): Replace usage of any_thread_of with
> find_any_thread_of_pid.
> * inferiors.c (thread_pid_matches_callback): Remove.
> (find_thread_process): Adjust to use find_any_thread_of_pid.
> ---
> gdb/gdbserver/inferiors.c | 14 +-------------
> gdb/gdbserver/linux-x86-low.c | 15 +--------------
> gdb/gdbserver/server.c | 19 +++----------------
> gdb/gdbserver/thread-db.c | 15 +--------------
> gdb/gdbserver/tracepoint.c | 14 +-------------
> 5 files changed, 7 insertions(+), 70 deletions(-)
>
> diff --git a/gdb/gdbserver/inferiors.c b/gdb/gdbserver/inferiors.c
> index 3c171a1..2212850 100644
> --- a/gdb/gdbserver/inferiors.c
> +++ b/gdb/gdbserver/inferiors.c
> @@ -141,25 +141,13 @@ 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)
> {
> - pid_t pid = ptid_get_pid (ptid_of (process));
> -
> - return (struct thread_info *)
> - find_inferior (&all_threads, thread_pid_matches_callback, &pid);
> + return find_any_thread_of_pid (process->entry.id.pid ());
> }
>
> /* Helper for find_any_thread_of_pid. Returns true if a thread
> diff --git a/gdb/gdbserver/linux-x86-low.c b/gdb/gdbserver/linux-x86-low.c
> index 844a165..49f6b2d 100644
> --- a/gdb/gdbserver/linux-x86-low.c
> +++ b/gdb/gdbserver/linux-x86-low.c
> @@ -846,17 +846,6 @@ x86_linux_read_description (void)
> gdb_assert_not_reached ("failed to return tdesc");
> }
>
> -/* Callback for find_inferior. Stops iteration when a thread with a
> - given PID is found. */
> -
> -static int
> -same_process_callback (struct inferior_list_entry *entry, void *data)
> -{
> - int pid = *(int *) data;
> -
> - return (ptid_get_pid (entry->id) == pid);
> -}
> -
> /* Callback for for_each_inferior. Calls the arch_setup routine for
> each process. */
>
> @@ -866,9 +855,7 @@ x86_arch_setup_process_callback (struct inferior_list_entry *entry)
> int pid = ptid_get_pid (entry->id);
>
> /* Look up any thread of this processes. */
> - current_thread
> - = (struct thread_info *) find_inferior (&all_threads,
> - same_process_callback, &pid);
> + current_thread = find_any_thread_of_pid (pid);
>
> the_low_target.arch_setup ();
> }
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index 56c6393..bedb87b 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -3450,17 +3450,6 @@ gdbserver_show_disableable (FILE *stream)
> break; \
> }
>
> -static int
> -first_thread_of (struct inferior_list_entry *entry, void *args)
> -{
> - int pid = * (int *) args;
> -
> - if (ptid_get_pid (entry->id) == pid)
> - return 1;
> -
> - return 0;
> -}
> -
> static void
> kill_inferior_callback (struct inferior_list_entry *entry)
> {
> @@ -4162,11 +4151,9 @@ process_serial_event (void)
> && ptid_equal (pid_to_ptid (pid),
> gdb_id))
> {
> - struct thread_info *thread =
> - (struct thread_info *) find_inferior (&all_threads,
> - first_thread_of,
> - &pid);
> - if (!thread)
> + thread_info *thread = find_any_thread_of_pid (pid);
> +
> + if (thread == NULL)
> {
> write_enn (own_buf);
> break;
> diff --git a/gdb/gdbserver/thread-db.c b/gdb/gdbserver/thread-db.c
> index 1ffb79d..9b867b8 100644
> --- a/gdb/gdbserver/thread-db.c
> +++ b/gdb/gdbserver/thread-db.c
> @@ -733,25 +733,12 @@ thread_db_init (void)
> return 0;
> }
>
> -static int
> -any_thread_of (struct inferior_list_entry *entry, void *args)
> -{
> - int *pid_p = (int *) args;
> -
> - if (ptid_get_pid (entry->id) == *pid_p)
> - return 1;
> -
> - return 0;
> -}
> -
> static void
> switch_to_process (struct process_info *proc)
> {
> int pid = pid_of (proc);
>
> - current_thread =
> - (struct thread_info *) find_inferior (&all_threads,
> - any_thread_of, &pid);
> + current_thread = find_any_thread_of_pid (pid);
> }
>
> /* Disconnect from libthread_db and free resources. */
> diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
> index 68ce10f..0f41ff4 100644
> --- a/gdb/gdbserver/tracepoint.c
> +++ b/gdb/gdbserver/tracepoint.c
> @@ -3956,17 +3956,6 @@ cmd_qtstmat (char *packet)
> run_inferior_command (packet, strlen (packet) + 1);
> }
>
> -/* Helper for gdb_agent_about_to_close.
> - Return non-zero if thread ENTRY is in the same process in DATA. */
> -
> -static int
> -same_process_p (struct inferior_list_entry *entry, void *data)
> -{
> - int *pid = (int *) data;
> -
> - return ptid_get_pid (entry->id) == *pid;
> -}
> -
> /* Sent the agent a command to close it. */
>
> void
> @@ -3981,8 +3970,7 @@ gdb_agent_about_to_close (int pid)
> saved_thread = current_thread;
>
> /* Find any thread which belongs to process PID. */
> - current_thread = (struct thread_info *)
> - find_inferior (&all_threads, same_process_p, &pid);
> + current_thread = find_any_thread_of_pid (pid);
>
> strcpy (buf, "close");
>
> --
> 2.7.4
--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 1/3] gdbserver: Remove duplicate functions to find any thread of process
2017-09-15 4:09 ` Sergio Durigan Junior
@ 2017-09-15 12:53 ` Simon Marchi
0 siblings, 0 replies; 13+ messages in thread
From: Simon Marchi @ 2017-09-15 12:53 UTC (permalink / raw)
To: Sergio Durigan Junior; +Cc: Simon Marchi, gdb-patches
On 2017-09-15 06:09, Sergio Durigan Junior wrote:
> On Sunday, September 10 2017, Simon Marchi wrote:
>
>> We have about 6 functions/callbacks to find_inferior meant to find a
>> thread that belongs to a given pid. Remove all but
>> find_any_thread_of_pid and replace their uses with
>> find_any_thread_of_pid.
>
> Thanks for doing. gdbserver is really confusing when dealing with
> threads and their data structures, so this patch is a nice step towards
> a better interface.
One thing I'm working one is getting rid of
inferior_list/inferior_list_entry. The patch is pretty much complete,
but I keep hesitating about meaningless details... I should just kick
myself and submit what I have.
> I think this can go in as is, because it is a self contained cleanup,
> but I would like us to move yet another step ahead: recently I've
> implemente gdbserver's version of "switch_to_thread", and I couldn't
> help but think that a "switch_to_any_thread_of_pid" would be good to
> have, because that's what's happening most of the time in this patch.
Ok, sounds like implementing that would just be find_any_thread_of_pid +
switch_to_thread.
> I know I have to bite the bullet and convert places where
> "current_thread = find_thread_ptid (ptid)" is still being used so that
> "switch_to_thread" is used globally. I intend to submit a patch for
> that.
Seems like you already did!
> Anyway, this looks good to me.
Thanks for the review, I'll push it in.
Simon
^ permalink raw reply [flat|nested] 13+ messages in thread