[v5,2/5] Share parts of gdb/gdbthread.h with gdbserver

  Again, it was necessary to share a few functions declared on
gdb/gdbthread.h with gdbserver, because they are needed by
fork_inferior.  I decided to implement them on
gdb/gdbserver/inferiors.c because that's where the thread functions
are also implemented on gdbserver.  Some of these functions do not
need to be implemented on gdbserver, or don't make sense there, so
they are left blank and commented properly.

gdb/ChangeLog:
yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>

	* Makefile.in (HFILES_NO_SRCDIR): Add "common/common-gdbthread.h".
	* common/common-gdbthread.h: New file, with parts from
	"gdb/gdbthread.h".
	* gdbthread.h: Include "common-gdbthread.h".
	(init_thread_list): Moved to "common/common-gdbthread.h".
	(add_thread_silent): Likewise.
	(switch_to_thread): Likewise.

gdb/gdbserver/ChangeLog:
yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>

	* inferiors.c (init_thread_list): New function.
	(switch_to_thread): Likewise.
	(add_thread_silent): Likewise.
---
 gdb/Makefile.in               |  1 +
 gdb/common/common-gdbthread.h | 32 ++++++++++++++++++++++++++++++++
 gdb/gdbserver/inferiors.c     | 23 +++++++++++++++++++++++
 gdb/gdbthread.h               |  9 +--------
 4 files changed, 57 insertions(+), 8 deletions(-)
 create mode 100644 gdb/common/common-gdbthread.h
  

On 03/30/2017 02:49 AM, Sergio Durigan Junior wrote:
> +struct thread_info *
> +add_thread_silent (ptid_t ptid)
> +{
> +  pid_t pid = ptid_get_pid (ptid);
> +
> +  /* Check if there is a process already.  */
> +  if (find_process_pid (pid) == NULL)
> +    add_process (pid, 0);
> +
> +  return add_thread (ptid_build (pid, pid, 0), NULL);

This ptid_build here is always using the "pid" as
lwpid.  This suggests to me that "add_thread_silent" was not the
right function entry point to share, since we're adding a hack
to one of the implementations.  Either we need a new function,
like "add_initial_thread (pid_t pid)", or maybe we could move the
add_thread_silent call in fork_inferior to the gdb-side, only?

Thanks,
Pedro Alves
  

On Friday, March 31 2017, Pedro Alves wrote:

> On 03/30/2017 02:49 AM, Sergio Durigan Junior wrote:
>> +struct thread_info *
>> +add_thread_silent (ptid_t ptid)
>> +{
>> +  pid_t pid = ptid_get_pid (ptid);
>> +
>> +  /* Check if there is a process already.  */
>> +  if (find_process_pid (pid) == NULL)
>> +    add_process (pid, 0);
>> +
>> +  return add_thread (ptid_build (pid, pid, 0), NULL);
>
> This ptid_build here is always using the "pid" as
> lwpid.  This suggests to me that "add_thread_silent" was not the
> right function entry point to share, since we're adding a hack
> to one of the implementations.  Either we need a new function,
> like "add_initial_thread (pid_t pid)", or maybe we could move the
> add_thread_silent call in fork_inferior to the gdb-side, only?

OOC, could you expand on why using pid as the lwpid means that this is a
hack?

On a side note, it seems from your comment that the best alternative
would be to move the add_thread_silent call to the GDB-specific
postfork_hook, and undo the changes on gdbserver/{linux,lynx}-low.c that
add the "*_update_process" functions.  This would mean that, on GDB, the
prefork_hook would always call add_thread_silent, but on gdbserver each
target would be responsible for adding the process/thread after calling
fork_inferior.
  

On 04/07/2017 03:53 AM, Sergio Durigan Junior wrote:
> On Friday, March 31 2017, Pedro Alves wrote:
> 
>> On 03/30/2017 02:49 AM, Sergio Durigan Junior wrote:
>>> +struct thread_info *
>>> +add_thread_silent (ptid_t ptid)
>>> +{
>>> +  pid_t pid = ptid_get_pid (ptid);
>>> +
>>> +  /* Check if there is a process already.  */
>>> +  if (find_process_pid (pid) == NULL)
>>> +    add_process (pid, 0);
>>> +
>>> +  return add_thread (ptid_build (pid, pid, 0), NULL);
>>
>> This ptid_build here is always using the "pid" as
>> lwpid.  This suggests to me that "add_thread_silent" was not the
>> right function entry point to share, since we're adding a hack
>> to one of the implementations.  Either we need a new function,
>> like "add_initial_thread (pid_t pid)", or maybe we could move the
>> add_thread_silent call in fork_inferior to the gdb-side, only?
> 
> OOC, could you expand on why using pid as the lwpid means that this is a
> hack?

Sure.  The function's purpose is creating a thread with a given
PTID, but while that is what currently happens on the gdb side, and
you're not changing it, the new implementation on the gdbserver side
ignores the ptid's fields except the PID.  It'd be reasonable for
some other future shared bits of code to call add_thread_silent on
other contexts with the lwp/tid fields of ptid filled in,
but it wouldn't work because of this hack.  Trying to fix that
issue when we stumble into this by making gdbserver's implementation
NOT ignore the passed-in ptid wouldn't work because it'd break the
fork-child path.  That's force us to fix it properly then and revisit
this exact issue we're discussing.  So I'd rather address it now,
upfront.

Note, while the right ptid for the initial thread for
Linux is "(pid,pid,0)", that's not the case for all/other ports.
So that hack above is also baking in a Linux-ism that happens to
work because gdbserver has fewer Unix-like ports than GDB.

> 
> On a side note, it seems from your comment that the best alternative
> would be to move the add_thread_silent call to the GDB-specific
> postfork_hook, and undo the changes on gdbserver/{linux,lynx}-low.c that
> add the "*_update_process" functions.  This would mean that, on GDB, the
> prefork_hook would always call add_thread_silent, but on gdbserver each
> target would be responsible for adding the process/thread after calling
> fork_inferior.

It'd be nice to be able to normalize the APIs between gdb and gdbserver.

It sounds like if we make each target responsible for adding the right main
thread after fork_inferior returns on the gdb side too, then we'll be able
to get rid of this hack in linux-nat.c:linux_nat_wait_1 while at it:

  /* The first time we get here after starting a new inferior, we may
     not have added it to the LWP list yet - this is the earliest
     moment at which we know its PID.  */
  if (ptid_is_pid (inferior_ptid))
    {
      /* Upgrade the main thread's ptid.  */
      thread_change_ptid (inferior_ptid,
			  ptid_build (ptid_get_pid (inferior_ptid),
				      ptid_get_pid (inferior_ptid), 0));

      lp = add_initial_lwp (inferior_ptid);
      lp->resumed = 1;
    }

Thanks,
Pedro Alves