Hello Pedro,

Pedro Alves wrote:
> On Friday 12 June 2009 19:57:02, Aleksandar Ristovski wrote:
> 
>> This patch introduces extra thread info for nto target, and 
>> fixes a bug in procfs_find_new_threads.
> 
> 
>> Index: gdb/nto-procfs.c
>> ===================================================================
>> --- gdb/nto-procfs.c    (revision 2)
>> +++ gdb/nto-procfs.c    (working copy)
>> @@ -220,11 +220,94 @@ static int
>>  procfs_thread_alive (struct target_ops *ops, ptid_t ptid)
>>  {
> 
>> +
>> +  status.tid = tid;
>> +  if ((err = devctl (ctl_fd, DCMD_PROC_TIDSTATUS, 
>> +             &status, sizeof (status), 0)) != EOK)
>> +    return 0;
>> +
>> +  /* Thread is alive or dead but not yet joined,
>> +     or dead and there is an alive (or dead unjoined) thread with
>> +     higher tid. We return tidinfo.  
> 
>                    ^ Missing space.
> 
> This "We return tidinfo." looks out of place though.
> I suspect this whole comment was copied from the devctl's docs or
> implementation.

Indeed. I copied it. (Ironically, I also wrote it where I 
copied it from). I changed wording to be more applicable to 
this place.

> 
>> +
>> +     Client should check if the tid is the same as 
>> +     requested; if not, requested tid is dead.  */
>> +  return (status.tid == tid) && (status.state != STATE_DEAD);
>> +}
>> +
>> +static void
>> +update_thread_private_data_name (struct thread_info *new_thread,
>> +                                const char *newname)
>> +{
> 
> 
>> +
>> +  if (!new_thread->private)
>> +    {
>> +      new_thread->private = xmalloc (sizeof (struct private_thread_info)
>> +                                    + newnamelen + 1);
> 
> Note: pedanticaly, '+ 1' here isn't really needed, since the name field was
> declared as name[1].  FYI, a standard practice is to use
> offsetof(struct foo, flexiblearraymember) + arraysize instead of sizeof.

Changed to "offsetof".

> 
>> +static void 
>> +update_thread_private_data (struct thread_info *new_thread, 
>> +                           pthread_t tid, int state, int flags)
>> +{
>> +  struct private_thread_info *pti;
>> +  procfs_info pidinfo;
>> +  struct _thread_name *tn;
>> +  procfs_threadctl tctl;
>> +#if _NTO_VERSION > 630
>> +  gdb_assert (new_thread != NULL);
>> +
>> +  if (devctl (ctl_fd, DCMD_PROC_INFO, &pidinfo,
>> +             sizeof(pidinfo), 0) != EOK)
>> +    return;
> 
> Should this set the private data to a known state, or
> release it, so it doesn't show stale data?  (can this really
> happen?)

I don't think so; if devctl returned != EOK, we really don't 
know anything about the thread and it will be deleted from 
the thread list... even if it happens that gdb still prints 
info about this thread, I think printing name is still o.k. 
since status should now say "DEAD".

> 
>>   2009-06-12  Aleksandar Ristovski  <aristovski@qnx.com>
>>
>>         * nto-tdep.c (nto_thread_state_str): New array.
>>         (nto_extra_thread_info): New function definition.
>>         * nto-tdep.h (gdbthread.h): New include.
>>         (private_thread_info): New struct.
>>         (nto_extra_thread_info): New declaration.
>>         * nto-procfs.c (procfs_thread_alive): Properly check if
>>         thread is still alive.
>>         (update_thread_private_data_name, update_thread_private_data): New
>>         function definition.
>>         (procfs_find_new_threads): Fetch thread private data.
>>         (init_procfs_ops): Register to_extra_thread_info.
> 
> Otherwise, looks okay to me.
> 

New patch attached with fixes above.


Thanks,

-- 
Aleksandar Ristovski
QNX Software Systems


* nto-tdep.c (nto_thread_state_str): New array.
(nto_extra_thread_info): New function definition.
* nto-tdep.h (gdbthread.h): New include.
(private_thread_info): New struct.
(nto_extra_thread_info): New declaration.
* nto-procfs.c (procfs_thread_alive): Properly check if
thread is still alive.
(update_thread_private_data_name, 
update_thread_private_data): New
function definition.
(procfs_find_new_threads): Fetch thread private data.
(init_procfs_ops): Register to_extra_thread_info.