From patchwork Thu Jun 21 15:57:45 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 27982 Received: (qmail 122755 invoked by alias); 21 Jun 2018 15:57:51 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 122745 invoked by uid 89); 21 Jun 2018 15:57:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.4 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=inspect, 1199, 7, 11997, deep X-HELO: mx1.redhat.com Received: from mx3-rdu2.redhat.com (HELO mx1.redhat.com) (66.187.233.73) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 21 Jun 2018 15:57:48 +0000 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 127A0F68AA; Thu, 21 Jun 2018 15:57:47 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5D1922156700; Thu, 21 Jun 2018 15:57:46 +0000 (UTC) Subject: Re: [PATCH] Use thread_info and inferior pointers more throughout To: Tom Tromey References: <20180607180704.3991-1-palves@redhat.com> <87tvqeladc.fsf@tromey.com> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: <32766087-7cca-e149-65aa-f8e2df5ff23e@redhat.com> Date: Thu, 21 Jun 2018 16:57:45 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: <87tvqeladc.fsf@tromey.com> On 06/07/2018 07:28 PM, Tom Tromey wrote: >>>>>> "Pedro" == Pedro Alves writes: > > Pedro> In many cases though, we already have the thread_info or inferior > Pedro> pointer handy, but we "lose" it somewhere along the call stack, only > Pedro> to look it up again by ptid_t/pid. Since thread_info or inferior > Pedro> objects know their parent target, if we pass around thread_info or > Pedro> inferior pointers when possible, we avoid having to add extra > Pedro> target_ops parameters to many functions, and also, we eliminate a > Pedro> number of by ptid_t/int lookups. > > This seems like a good idea. I've sometimes wondered why ptid was used > rather than object pointers; but now it seems that there was no deep > reason :) I think it's just a consequence of how thread support grew in gdb. In the really old days, gdb did not support debugging multiple threads at all, because threads weren't even a thing, and the inferior process was just identified by its integer PID. Later, when support for threads was added, threads were bolted on as kind of a second-class citizen, with gdb context-switching a bunch of globals, filling them with the context of the now-selected thread. See e.g.: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/thread.c;hb=ca67fcb81a300f8e2e58246cc64d971514e95d5a#l295 ptid_t was introduced to replace a single integer PIDs, and I think before that for a period the LWP/TID info was crammed into that single integer PID with a shifting and masking, even. When non-stop support was introduced, gdb's internal threading model was changed to "always a thread", see e.g.: https://sourceware.org/ml/gdb-patches/2008-08/msg00169.html So before _that_, we really had to use ptid instead of object pointers, because when debugging a single threaded process, there was really no thread object to point to. After that, the many run-control-related globals were eliminated, in favor of thread_info fields, and that funny context-switching was mostly eliminated, see e.g.,: https://sourceware.org/ml/gdb-patches/2008-08/msg00441.html Other time more globals have been eliminated since that, but we're still left with a few. In fact, I'm about to send a patch that eliminates another one (stop_pc). :-) That was a fun trip down the memory lane. :-) > > Pedro> - Related, there's a spot where using a RAII object to handle the > Pedro> refcount would be handy, so a new scoped_inc_dec_ref class is added > Pedro> to common/refcounted-object.h. > > It seems to me that this could be done via gdb::ref_ptr plus a simple > policy class that interfaces it to refcounted_object. Yeah, good idea -- I think that is likely to come in handy in other spots too, so I've done it. > > Pedro> +/* A RAII type that increments/decrements the refcount of an object on > Pedro> + enter/exit of a scope. */ > Pedro> + > Pedro> +class scoped_inc_dec_ref > Pedro> +{ > Pedro> +public: > Pedro> + explicit scoped_inc_dec_ref (refcounted_object *obj) > Pedro> + : m_obj (obj) > Pedro> + { > Pedro> + m_obj->incref (); > Pedro> + } > Pedro> + > Pedro> + ~scoped_inc_dec_ref () > Pedro> + { > Pedro> + m_obj->decref (); > Pedro> + } > > ... if you do keep this it probably needs DISABLE_COPY_AND_ASSIGN. > You're right. I removed it instead though. I'm squashing this into the patch that I'm merging. (Also noticed that there was a pair of incref/decref in run_inferior_call that is not necessary.) From ccf4c1b904f0c7b8ee339c7fb14af5f857c305bb Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Fri, 15 Jun 2018 22:51:49 +0100 Subject: [PATCH] thread_info_ref --- gdb/common/refcounted-object.h | 19 +++++++------------ gdb/gdbthread.h | 5 +++++ gdb/infcall.c | 28 ++++++++++++---------------- gdb/thread.c | 8 ++++---- 4 files changed, 28 insertions(+), 32 deletions(-) diff --git a/gdb/common/refcounted-object.h b/gdb/common/refcounted-object.h index 0140342713..a3799d8de4 100644 --- a/gdb/common/refcounted-object.h +++ b/gdb/common/refcounted-object.h @@ -51,25 +51,20 @@ private: int m_refcount = 0; }; -/* A RAII type that increments/decrements the refcount of an object on - enter/exit of a scope. */ +/* A policy class to interface gdb::ref_ptr with a + refcounted_object. */ -class scoped_inc_dec_ref +struct refcounted_object_ref_policy { -public: - explicit scoped_inc_dec_ref (refcounted_object *obj) - : m_obj (obj) + static void incref (refcounted_object *ptr) { - m_obj->incref (); + ptr->incref (); } - ~scoped_inc_dec_ref () + static void decref (refcounted_object *ptr) { - m_obj->decref (); + ptr->decref (); } - -private: - refcounted_object *m_obj; }; #endif /* REFCOUNTED_OBJECT_H */ diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h index 88fd0628f0..bd5ab9193f 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -370,6 +370,11 @@ public: struct thread_info *step_over_next = NULL; }; +/* A gdb::ref_ptr pointer to a thread_info. */ + +using thread_info_ref + = gdb::ref_ptr; + /* Create an empty thread list, or empty the existing one. */ extern void init_thread_list (void); diff --git a/gdb/infcall.c b/gdb/infcall.c index cc2bbeb910..e1861dcd8e 100644 --- a/gdb/infcall.c +++ b/gdb/infcall.c @@ -614,8 +614,6 @@ run_inferior_call (struct call_thread_fsm *sm, /* We want to print return value, please... */ call_thread->control.proceed_to_finish = 1; - call_thread->incref (); - TRY { proceed (real_pc, GDB_SIGNAL_0); @@ -630,8 +628,6 @@ run_inferior_call (struct call_thread_fsm *sm, } END_CATCH - call_thread->decref (); - /* If GDB has the prompt blocked before, then ensure that it remains so. normal_stop calls async_enable_stdin, so reset the prompt state again here. In other cases, stdin will be re-enabled by @@ -750,13 +746,13 @@ call_function_by_hand_dummy (struct value *function, if (execution_direction == EXEC_REVERSE) error (_("Cannot call functions in reverse mode.")); - thread_info *call_thread = inferior_thread (); - /* We're going to run the target, and inspect the thread's state afterwards. Hold a strong reference so that the pointer remains valid even if the thread exits. */ - scoped_inc_dec_ref inc_dec_ref (call_thread); - bool stack_temporaries = thread_stack_temporaries_enabled_p (call_thread); + thread_info_ref call_thread + = thread_info_ref::new_reference (inferior_thread ()); + + bool stack_temporaries = thread_stack_temporaries_enabled_p (call_thread.get ()); frame = get_current_frame (); gdbarch = get_frame_arch (frame); @@ -850,7 +846,7 @@ call_function_by_hand_dummy (struct value *function, { struct value *lastval; - lastval = get_last_thread_stack_temporary (call_thread); + lastval = get_last_thread_stack_temporary (call_thread.get ()); if (lastval != NULL) { CORE_ADDR lastval_addr = value_address (lastval); @@ -1145,9 +1141,9 @@ call_function_by_hand_dummy (struct value *function, /* Everything's ready, push all the info needed to restore the caller (and identify the dummy-frame) onto the dummy-frame stack. */ - dummy_frame_push (caller_state, &dummy_id, call_thread); + dummy_frame_push (caller_state, &dummy_id, call_thread.get ()); if (dummy_dtor != NULL) - register_dummy_frame_dtor (dummy_id, call_thread, + register_dummy_frame_dtor (dummy_id, call_thread.get (), dummy_dtor, dummy_dtor_data); /* Register a clean-up for unwind_on_terminating_exception_breakpoint. */ @@ -1182,7 +1178,7 @@ call_function_by_hand_dummy (struct value *function, struct_return || hidden_first_param_p, struct_addr); - e = run_inferior_call (sm, call_thread, real_pc); + e = run_inferior_call (sm, call_thread.get (), real_pc); gdb::observers::inferior_call_post.notify (call_thread_ptid, funaddr); @@ -1199,7 +1195,7 @@ call_function_by_hand_dummy (struct value *function, which runs its destructors and restores the inferior's suspend state, and restore the inferior control state. */ - dummy_frame_pop (dummy_id, call_thread); + dummy_frame_pop (dummy_id, call_thread.get ()); restore_infcall_control_state (inf_status); /* Get the return value. */ @@ -1207,7 +1203,7 @@ call_function_by_hand_dummy (struct value *function, /* Clean up / destroy the call FSM, and restore the original one. */ - thread_fsm_clean_up (call_thread->thread_fsm, call_thread); + thread_fsm_clean_up (call_thread->thread_fsm, call_thread.get ()); thread_fsm_delete (call_thread->thread_fsm); call_thread->thread_fsm = saved_sm; @@ -1321,7 +1317,7 @@ When the function is done executing, GDB will silently stop."), /* We must get back to the frame we were before the dummy call. */ - dummy_frame_pop (dummy_id, call_thread); + dummy_frame_pop (dummy_id, call_thread.get ()); /* We also need to restore inferior status to that before the dummy call. */ @@ -1362,7 +1358,7 @@ When the function is done executing, GDB will silently stop."), { /* We must get back to the frame we were before the dummy call. */ - dummy_frame_pop (dummy_id, call_thread); + dummy_frame_pop (dummy_id, call_thread.get ()); /* We also need to restore inferior status to that before the dummy call. */ diff --git a/gdb/thread.c b/gdb/thread.c index 535a9d4c05..2b471cd292 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -65,17 +65,17 @@ static int thread_alive (struct thread_info *); /* RAII type used to increase / decrease the refcount of each thread in a given list of threads. */ -class scoped_inc_dec_ref_threads +class scoped_inc_dec_ref { public: - explicit scoped_inc_dec_ref_threads (const std::vector &thrds) + explicit scoped_inc_dec_ref (const std::vector &thrds) : m_thrds (thrds) { for (thread_info *thr : m_thrds) thr->incref (); } - ~scoped_inc_dec_ref_threads () + ~scoped_inc_dec_ref () { for (thread_info *thr : m_thrds) thr->decref (); @@ -1607,7 +1607,7 @@ thread_apply_all_command (const char *cmd, int from_tty) /* Increment the refcounts, and restore them back on scope exit. */ - scoped_inc_dec_ref_threads inc_dec_ref (thr_list_cpy); + scoped_inc_dec_ref inc_dec_ref (thr_list_cpy); std::sort (thr_list_cpy.begin (), thr_list_cpy.end (), tp_array_compar);