Re: [PATCH 3/4] Create private_thread_info hierarchy

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2017-11-23 09:41, Pedro Alves wrote:
> On 11/22/2017 04:41 PM, Simon Marchi wrote:
>> From: Simon Marchi <simon.marchi@polymtl.ca>
> 
>> Including gdbthread.h from darwin-nat.h gave these errors:
>> 
>> /Users/simark/src/binutils-gdb/gdb/gdbthread.h:609:3: error: must use 
>> 'class' tag to refer to type 'thread_info' in this scope
>>   thread_info *m_thread;
>>   ^
>>   class
>> /usr/include/mach/thread_act.h:240:15: note: class 'thread_info' is 
>> hidden by a non-type declaration of 'thread_info' here
>> kern_return_t thread_info
>>               ^
>> 
>> It turns out that there is a thread_info function in the 
>> Darwin/XNU/mach API:
>> 
>>   http://web.mit.edu/darwin/src/modules/xnu/osfmk/man/thread_info.html
>> 
>> Therefore, I had to add the class keyword at a couple of places in 
>> gdbthread.h,
>> I don't really see a way around it.
> 
> The way around it is to move all of gdb under "namespace gdb", and then
> references to thread_info hit gdb::thread_info instead.  :-)

But that would require us to improve gdb's C++ debugging support, so 
that we don't have to type gdb:: every time!  Is such thing even 
possible?!

>>  private:
>> -  thread_info *m_thread;
>> +  /* Use the "class" keyword here, because of a class with a 
>> "thread_info"
>> +     function in the Darwin API.  */
>> +  class thread_info *m_thread;
> 
> This comment sounds incorrect.  A a "thread_info" function/method
> inside some class couldn't possibly interfere, right?  The issue is
> that there's a _free-function_ called thread_info, right?

Err yeah, the "class with a" part should not be there:

+  /* Use the "class" keyword here, because of a "thread_info"
+     function in the Darwin API.  */

> 
> Some nits below.
> 
>> @@ -776,8 +779,10 @@ sync_threadlists (void)
>> 
>>  	  if (cmp_result == 0)
>>  	    {
>> -	      gbuf[gi]->priv->pdtid = pdtid;
>> -	      gbuf[gi]->priv->tid = tid;
>> +	      aix_thread_info *priv = (aix_thread_info *) gbuf[gi]->priv.get 
>> ();
> 
> ...
> 
>> @@ -808,8 +815,9 @@ static int
>>  iter_tid (struct thread_info *thread, void *tidp)
>>  {
>>    const pthdb_tid_t tid = *(pthdb_tid_t *)tidp;
>> +  aix_thread_info *priv = (aix_thread_info *) thread->priv.get ();
> 
> This seems to scream for a "get_aix_thread_info (thread_info *);"
> function.

Will do.

>> 
>> -  return (thread->priv->tid == tid);
>> +  return priv->tid == tid;
>>  }
>> 
> 
>> @@ -1381,11 +1379,11 @@ thread_db_pid_to_str (struct target_ops *ops, 
>> ptid_t ptid)
>>    if (thread_info != NULL && thread_info->priv != NULL)
>>      {
>>        static char buf[64];
>> -      thread_t tid;
>> +      thread_db_thread_info *priv
>> +	= (thread_db_thread_info *) thread_info->priv.get ();
> 
> ...
> 
>> -  if (info->priv->dying)
>> +  thread_db_thread_info *priv = (thread_db_thread_info *) 
>> info->priv.get ();
>> +
>> +  if (priv->dying)
>>      return "Exiting";
>> 
> 
> ...
> 
>>    return NULL;
>> @@ -1434,7 +1434,9 @@ thread_db_thread_handle_to_thread_info (struct 
>> target_ops *ops,
>> 
>>    ALL_NON_EXITED_THREADS (tp)
>>      {
>> -      if (tp->inf == inf && tp->priv != NULL && handle_tid == 
>> tp->priv->tid)
>> +      thread_db_thread_info *priv = (thread_db_thread_info *) 
>> tp->priv.get ();
> 
> You know what I'll say, right? :-)

I think I'm starting to get the idea.

>> +
>> +      if (tp->inf == inf && priv != NULL && handle_tid == priv->tid)
>>          return tp;
>>      }
>> 
> 
>> diff --git a/gdb/nto-procfs.c b/gdb/nto-procfs.c
>> index 1da1a98..5906eb6 100644
>> --- a/gdb/nto-procfs.c
>> +++ b/gdb/nto-procfs.c
>> @@ -248,38 +248,24 @@ static void
>>  update_thread_private_data_name (struct thread_info *new_thread,
>>  				 const char *newname)
>>  {
>> -  int newnamelen;
>> -  struct private_thread_info *pti;
>> +  nto_thread_info *pti = (nto_thread_info *) new_thread->priv.get ();
> 
> The comment really applies to all ports.

Hehehe.

>> -/* Private data that we'll store in (struct thread_info)->private.  
>> */
>> -struct private_thread_info
>> +/* Private data that we'll store in (struct thread_info)->priv.  */
>> +struct remote_thread_info : public private_thread_info
>>  {
>> -  char *extra;
>> -  char *name;
>> -  int core;
>> +  std::string extra;
>> +  std::string name;
>> +  int core = -1;
>> 
>>    /* Thread handle, perhaps a pthread_t or thread_t value, stored as 
>> a
>>       sequence of bytes.  */
>> -  gdb::byte_vector *thread_handle;
>> +  gdb::byte_vector thread_handle;
> 
> Ah, OK, you did the pointer->value thing here.  Great.  :-)

Yes, but as I mentioned in my other reply, I think it's better to do it 
in the previous patch.

Thanks,

Simon