This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v5 2/5] Share parts of gdb/gdbthread.h with gdbserver


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]