[2/3] gdbserver: Remove gdb_id_to_thread_id
Commit Message
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(-)
Comments
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
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
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
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
@@ -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)
{
@@ -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
@@ -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;