[1/2] Add constructor and destructor to thread_info

  This patch adds constructor and destructor to thread_info.

gdb:

2017-03-28  Yao Qi  <yao.qi@linaro.org>

	* gdbthread.h (struct thread_info): Declare constructor and
	destructor.  Add some member initializers.
	* thread.c (free_thread): Remove.
	(init_thread_list): Call delete instead of free_thread.
	(new_thread): Call thread_info constructor.
	(thread_info::thread_info): New function.
	(thread_info::~thread_info): New function.
	(delete_thread_1): Call delete instead of free_thread.
	(make_cleanup_restore_current_thread): Move tp and frame to
	inner block.
---
 gdb/gdbthread.h | 48 ++++++++++++++++++++-----------------
 gdb/thread.c    | 73 ++++++++++++++++++++++++++++++---------------------------
 2 files changed, 65 insertions(+), 56 deletions(-)
  

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
  

Pedro Alves <palves@redhat.com> writes:

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

Other thread_info fields can be changed to bool too.  Once I change
"executing" to bool, the 2nd argument of set_executing should be bool
too.  I'll change them in separate patches.


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

You mean spurious newline?

> 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};
>

This will only initialize the first member of union
target_waitstatus.value to zero, so some bits of
target_waitstatus.value.related_pid is not initialized.

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

On 03/28/2017 03:08 PM, Yao Qi wrote:

>>
>> etc.  (and tweak "Non-zero" references to say "True" instead.)
>>
>> (the patch's subject just becomes "C++-fy thread_info a bit" instead.)
>>
> 
> Other thread_info fields can be changed to bool too.  

Yes, hence the "etc.".

> Once I change
> "executing" to bool, the 2nd argument of set_executing should be bool
> too.  I'll change them in separate patches.

Thanks, TBC, I'm fine with incremental progress toward bool
and with doing it opportunistically, rather than requiring targeted
patches.  Otherwise if we require changing everything at once, the
result is that people will avoid doing it, for fear of the growing
cascading work that propagating a type change everywhere tends to require.

> 
> 
>>> @@ -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.
>>
> 
> You mean spurious newline?
> 

Yes, sorry.

>> 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};
>>
> 
> This will only initialize the first member of union
> target_waitstatus.value to zero, so some bits of
> target_waitstatus.value.related_pid is not initialized.

Ah, blah, unions.  Well, the result is that the remaining fields of
the struct end up list->value->zero-initialized.  Zero-initialization
for unions means the first member is zero-initialized, and padding
is initialized to zero.  So I think the end result is the
same anyway.

Thanks,
Pedro Alves
  

Pedro Alves <palves@redhat.com> writes:

>>>  - struct target_waitstatus pending_follow;
>>>  + struct target_waitstatus pending_follow {TARGET_WAITKIND_SPURIOUS};
>>>
>> 
>> This will only initialize the first member of union
>> target_waitstatus.value to zero, so some bits of
>> target_waitstatus.value.related_pid is not initialized.
>
> Ah, blah, unions.  Well, the result is that the remaining fields of
> the struct end up list->value->zero-initialized.  Zero-initialization
> for unions means the first member is zero-initialized, and padding
> is initialized to zero.  So I think the end result is the
> same anyway.

It is different from
"memset (&this->pending_follow, 0, sizeof (this->pending_follow))".  The
first member "integer" is zero-initialized, but it is not the largest
member, so part of "related_pid" is not zero-initialized.

  |<------------------ union----------------->|
  |<------- related_pid ------->|<- padding ->|
  |<- integer ->|
  |      0      |<uninitialized>|     0       |
  

On 03/28/2017 04:40 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>>>>  - struct target_waitstatus pending_follow;
>>>>  + struct target_waitstatus pending_follow {TARGET_WAITKIND_SPURIOUS};
>>>>
>>>
>>> This will only initialize the first member of union
>>> target_waitstatus.value to zero, so some bits of
>>> target_waitstatus.value.related_pid is not initialized.
>>
>> Ah, blah, unions.  Well, the result is that the remaining fields of
>> the struct end up list->value->zero-initialized.  Zero-initialization
>> for unions means the first member is zero-initialized, and padding
>> is initialized to zero.  So I think the end result is the
>> same anyway.
> 
> It is different from
> "memset (&this->pending_follow, 0, sizeof (this->pending_follow))".  The
> first member "integer" is zero-initialized, but it is not the largest
> member, so part of "related_pid" is not zero-initialized.
> 
>   |<------------------ union----------------->|
>   |<------- related_pid ------->|<- padding ->|
>   |<- integer ->|
>   |      0      |<uninitialized>|     0       |
> 

I'm not so sure about that.  That is not my interpretation,
though the standard isn't crystal clear here.  The definition
of "padding" is left vague.  It doesn't look like that's the
interpretation of either G++ nor Clang though.  G++ always zeroes
the whole object, while Clang initializes only the first member,
leaving everything else uninitialized, which I found surprising,
since no padding _at all_ of any kind is initialized then.

I've sent an inquiry to std-discussion at isocpp.org list earlier
today, though I haven't gotten an answer yet.  See:

  https://groups.google.com/a/isocpp.org/forum/#!topic/std-discussion/ljaMNnkahHA

Though given Clang's behavior, it's probably not a good idea to assume
everything's zeroed out.

target_waitstatus would be a natural fit for something like
C++17's std::variant.  Particularly, the lifetime of value.execd_pathname is
not well defined...  Let's go with what you had, and revisit if/when we
change/fix target_waitstatus.

Thanks,
Pedro Alves