Re: [PATCH 1/2] Add constructor and destructor to thread_info

[Pedro Alves <palves@redhat.com>]

On 03/28/2017 09:24 AM, Yao Qi wrote:
> This patch adds constructor and destructor to thread_info.

Great, thanks.

LGTM.  Some nits below.

> diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
> index 06ed78f..8ada8f7 100644
> --- a/gdb/gdbthread.h
> +++ b/gdb/gdbthread.h
> @@ -179,7 +179,11 @@ typedef VEC (value_ptr) value_vec;
>  
>  struct thread_info
>  {
> -  struct thread_info *next;
> +public:
> +  explicit thread_info (struct inferior *inf, ptid_t ptid);

I'd remove the unnecessary "struct" while at it.

> +  ~thread_info ();
> +
> +  struct thread_info *next = NULL;
>    ptid_t ptid;			/* "Actual process id";
>  				    In fact, this may be overloaded with 
>  				    kernel thread id, etc.  */
> @@ -226,13 +230,13 @@ struct thread_info
>  
>    /* The name of the thread, as specified by the user.  This is NULL
>       if the thread does not have a user-given name.  */
> -  char *name;
> +  char *name = NULL;
>  
>    /* Non-zero means the thread is executing.  Note: this is different
>       from saying that there is an active target and we are stopped at
>       a breakpoint, for instance.  This is a real indicator whether the
>       thread is off and running.  */
> -  int executing;
> +  int executing = 0;

and do:

  bool executing = false;

etc.  (and tweak "Non-zero" references to say "True" instead.)

(the patch's subject just becomes "C++-fy thread_info a bit" instead.)

>  
>    /* Non-zero if this thread is resumed from infrun's perspective.
>       Note that a thread can be marked both as not-executing and
> @@ -241,19 +245,19 @@ struct thread_info
>       thread really run until that wait status has been processed, but
>       we should not process that wait status if we didn't try to let
>       the thread run.  */
> -  int resumed;
> +  int resumed = 0;
>  
>    /* Frontend view of the thread state.  Note that the THREAD_RUNNING/
>       THREAD_STOPPED states are different from EXECUTING.  When the
>       thread is stopped internally while handling an internal event,
>       like a software single-step breakpoint, EXECUTING will be false,
>       but STATE will still be THREAD_RUNNING.  */
> -  enum thread_state state;
> +  enum thread_state state = THREAD_STOPPED;
>  
>    /* If this is > 0, then it means there's code out there that relies
>       on this thread being listed.  Don't delete it from the lists even
>       if we detect it exiting.  */
> -  int refcount;
> +  int refcount = 0;
>  
>    /* State of GDB control of inferior thread execution.
>       See `struct thread_control_state'.  */
> @@ -263,8 +267,8 @@ struct thread_info
>       call.  See `struct thread_suspend_state'.  */
>    struct thread_suspend_state suspend;
>  
> -  int current_line;
> -  struct symtab *current_symtab;
> +  int current_line = 0;
> +  struct symtab *current_symtab = 0;

NULL ?

>  
>    /* Internal stepping state.  */
>  
> @@ -274,20 +278,20 @@ struct thread_info
>       by proceed and keep_going, and among other things, it's used in
>       adjust_pc_after_break to distinguish a hardware single-step
>       SIGTRAP from a breakpoint SIGTRAP.  */
> -  CORE_ADDR prev_pc;
> +  CORE_ADDR prev_pc = 0;
>  
>    /* Did we set the thread stepping a breakpoint instruction?  This is
>       used in conjunction with PREV_PC to decide whether to adjust the
>       PC.  */
> -  int stepped_breakpoint;
> +  int stepped_breakpoint = 0;
>  
>    /* Should we step over breakpoint next time keep_going is called?  */
> -  int stepping_over_breakpoint;
> +  int stepping_over_breakpoint = 0;
>  
>    /* Should we step over a watchpoint next time keep_going is called?
>       This is needed on targets with non-continuable, non-steppable
>       watchpoints.  */
> -  int stepping_over_watchpoint;
> +  int stepping_over_watchpoint = 0;
>  
>    /* Set to TRUE if we should finish single-stepping over a breakpoint
>       after hitting the current step-resume breakpoint.  The context here
> @@ -298,12 +302,12 @@ struct thread_info
>       step_after_step_resume_breakpoint is set to TRUE at this moment in
>       order to keep GDB in mind that there is still a breakpoint to step over
>       when GDB gets back SIGTRAP from step_resume_breakpoint.  */
> -  int step_after_step_resume_breakpoint;
> +  int step_after_step_resume_breakpoint = 0;
>  
>    /* Pointer to the state machine manager object that handles what is
>       left to do for the thread's execution command after the target
>       stops.  Several execution commands use it.  */
> -  struct thread_fsm *thread_fsm;
> +  struct thread_fsm *thread_fsm = NULL;
>  
>    /* This is used to remember when a fork or vfork event was caught by
>       a catchpoint, and thus the event is to be followed at the next
> @@ -311,37 +315,37 @@ struct thread_info
>    struct target_waitstatus pending_follow;
>  
>    /* True if this thread has been explicitly requested to stop.  */
> -  int stop_requested;
> +  int stop_requested = 0;
>  
>    /* The initiating frame of a nexting operation, used for deciding
>       which exceptions to intercept.  If it is null_frame_id no
>       bp_longjmp or bp_exception but longjmp has been caught just for
>       bp_longjmp_call_dummy.  */
> -  struct frame_id initiating_frame;
> +  struct frame_id initiating_frame = null_frame_id;
>  
>    /* Private data used by the target vector implementation.  */
> -  struct private_thread_info *priv;
> +  struct private_thread_info *priv = NULL;
>  
>    /* Function that is called to free PRIVATE.  If this is NULL, then
>       xfree will be called on PRIVATE.  */
> -  void (*private_dtor) (struct private_thread_info *);
> +  void (*private_dtor) (struct private_thread_info *) = NULL;
>  
>    /* Branch trace information for this thread.  */
>    struct btrace_thread_info btrace;
>  
>    /* Flag which indicates that the stack temporaries should be stored while
>       evaluating expressions.  */
> -  int stack_temporaries_enabled;
> +  int stack_temporaries_enabled = 0;
>  
>    /* Values that are stored as temporaries on stack while evaluating
>       expressions.  */
> -  value_vec *stack_temporaries;
> +  value_vec *stack_temporaries = NULL;
>  
>    /* Step-over chain.  A thread is in the step-over queue if these are
>       non-NULL.  If only a single thread is in the chain, then these
>       fields point to self.  */
> -  struct thread_info *step_over_prev;
> -  struct thread_info *step_over_next;
> +  struct thread_info *step_over_prev = NULL;
> +  struct thread_info *step_over_next = NULL;
>  };
>  
>  /* Create an empty thread list, or empty the existing one.  */
> diff --git a/gdb/thread.c b/gdb/thread.c
> index 99fe424..28907c5 100644
> --- a/gdb/thread.c
> +++ b/gdb/thread.c
> @@ -192,21 +192,6 @@ clear_thread_inferior_resources (struct thread_info *tp)
>    thread_cancel_execution_command (tp);
>  }
>  
> -static void
> -free_thread (struct thread_info *tp)
> -{
> -  if (tp->priv)
> -    {
> -      if (tp->private_dtor)
> -	tp->private_dtor (tp->priv);
> -      else
> -	xfree (tp->priv);
> -    }
> -
> -  xfree (tp->name);
> -  xfree (tp);
> -}
> -
>  void
>  init_thread_list (void)
>  {
> @@ -220,7 +205,8 @@ init_thread_list (void)
>    for (tp = thread_list; tp; tp = tpnext)
>      {
>        tpnext = tp->next;
> -      free_thread (tp);
> +      delete tp;
> +

Spurious whitespace.

>      }
>  
>    thread_list = NULL;
> @@ -233,16 +219,7 @@ init_thread_list (void)
>  static struct thread_info *
>  new_thread (struct inferior *inf, ptid_t ptid)
>  {
> -  struct thread_info *tp;
> -
> -  gdb_assert (inf != NULL);
> -
> -  tp = XCNEW (struct thread_info);
> -
> -  tp->ptid = ptid;
> -  tp->global_num = ++highest_thread_num;
> -  tp->inf = inf;
> -  tp->per_inf_num = ++inf->highest_thread_num;
> +  struct thread_info *tp = new thread_info (inf, ptid);

Drop "struct" ?

>  
>    if (thread_list == NULL)
>      thread_list = tp;
> @@ -255,11 +232,6 @@ new_thread (struct inferior *inf, ptid_t ptid)
>        last->next = tp;
>      }
>  
> -  /* Nothing to follow yet.  */
> -  tp->pending_follow.kind = TARGET_WAITKIND_SPURIOUS;
> -  tp->state = THREAD_STOPPED;
> -  tp->suspend.waitstatus.kind = TARGET_WAITKIND_IGNORE;
> -
>    return tp;
>  }
>  
> @@ -336,6 +308,38 @@ add_thread (ptid_t ptid)
>    return add_thread_with_info (ptid, NULL);
>  }
>  
> +thread_info::thread_info (struct inferior *inf, ptid_t ptid)
> +{
> +  gdb_assert (inf != NULL);
> +
> +  this->ptid = ptid;
> +  this->global_num = ++highest_thread_num;
> +  this->inf = inf;
> +  this->per_inf_num = ++inf->highest_thread_num;
> +
> +  /* Nothing to follow yet.  */
> +  memset (&this->pending_follow, 0, sizeof (this->pending_follow));
> +  memset (&this->control, 0, sizeof (this->control));
> +  memset (&this->suspend, 0, sizeof (this->suspend));
> +  memset (&this->btrace, 0, sizeof (this->btrace));
> +
> +  this->pending_follow.kind = TARGET_WAITKIND_SPURIOUS;
> +  this->suspend.waitstatus.kind = TARGET_WAITKIND_IGNORE;

These could all be in-class initialized like you did to the other
fields, instead of using the explicit memset:

 - struct btrace_thread_info btrace;
 + struct btrace_thread_info btrace {};

 - struct target_waitstatus pending_follow;
 + struct target_waitstatus pending_follow {TARGET_WAITKIND_SPURIOUS};

(maybe drop the "struct" too while at it).

>  /* Delete thread PTID and notify of thread exit.  If this is
> @@ -1655,8 +1659,6 @@ set_thread_refcount (void *data)
>  struct cleanup *
>  make_cleanup_restore_current_thread (void)
>  {
> -  struct thread_info *tp;
> -  struct frame_info *frame;
>    struct current_thread_cleanup *old = XNEW (struct current_thread_cleanup);
>  
>    old->inferior_ptid = inferior_ptid;
> @@ -1668,6 +1670,9 @@ make_cleanup_restore_current_thread (void)
>  
>    if (!ptid_equal (inferior_ptid, null_ptid))
>      {
> +      struct thread_info *tp;

I'd move this one further down to where it is initialized:

      thread_info *tp = find_thread_ptid (inferior_ptid);

> +      struct frame_info *frame;
> +
>        old->was_stopped = is_stopped (inferior_ptid);
>        if (old->was_stopped
>  	  && target_has_registers
> 

Thanks,
Pedro Alves