From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 80876 invoked by alias); 28 Mar 2017 12:12:08 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 80857 invoked by uid 89); 28 Mar 2017 12:12:07 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 28 Mar 2017 12:12:05 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B76F8C05AA60; Tue, 28 Mar 2017 12:12:05 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com B76F8C05AA60 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@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com B76F8C05AA60 Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.phx2.redhat.com [10.5.9.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 18F7417DE5; Tue, 28 Mar 2017 12:12:04 +0000 (UTC) Subject: Re: [PATCH 1/2] Add constructor and destructor to thread_info To: Yao Qi , gdb-patches@sourceware.org References: <1490689483-16084-1-git-send-email-yao.qi@linaro.org> <1490689483-16084-2-git-send-email-yao.qi@linaro.org> From: Pedro Alves Message-ID: <06385391-127e-87b7-d727-7d03a23f293c@redhat.com> Date: Tue, 28 Mar 2017 12:12:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1490689483-16084-2-git-send-email-yao.qi@linaro.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-03/txt/msg00478.txt.bz2 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