This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/2] Add constructor and destructor to thread_info
- From: Pedro Alves <palves at redhat dot com>
- To: Yao Qi <qiyaoltc at gmail dot com>, gdb-patches at sourceware dot org
- Date: Tue, 28 Mar 2017 13:12:04 +0100
- Subject: Re: [PATCH 1/2] Add constructor and destructor to thread_info
- Authentication-results: sourceware.org; auth=none
- Authentication-results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
- Authentication-results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=palves at redhat dot com
- Dkim-filter: OpenDKIM Filter v2.11.0 mx1.redhat.com B76F8C05AA60
- Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com B76F8C05AA60
- References: <1490689483-16084-1-git-send-email-yao.qi@linaro.org> <1490689483-16084-2-git-send-email-yao.qi@linaro.org>
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