Re: [PATCH 3/4] Create private_thread_info hierarchy

[Pedro Alves <palves@redhat.com>]

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.  :-)

>  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?

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.

>  
> -  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? :-)


> +
> +      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.

> -/* 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.  :-)

Thanks,
Pedro Alves