From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 40746 invoked by alias); 22 Apr 2015 20:14:18 -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 40041 invoked by uid 89); 22 Apr 2015 20:14:17 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=no version=3.3.2 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 (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 22 Apr 2015 20:14:16 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t3MKEEeH020339 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Wed, 22 Apr 2015 16:14:14 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t3MKEC3T030512; Wed, 22 Apr 2015 16:14:13 -0400 Message-ID: <55380114.6040607@redhat.com> Date: Wed, 22 Apr 2015 20:14:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Yao Qi CC: gdb-patches@sourceware.org Subject: Re: [PATCH v3 05/17] Embed the pending step-over chain in thread_info objects References: <1429267521-21047-1-git-send-email-palves@redhat.com> <1429267521-21047-6-git-send-email-palves@redhat.com> <86mw21zy0e.fsf@gmail.com> In-Reply-To: <86mw21zy0e.fsf@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2015-04/txt/msg00845.txt.bz2 On 04/21/2015 09:28 AM, Yao Qi wrote: > Pedro Alves writes: > > Hi Pedro, > This patch looks good to me, some questions below. > >> (displaced_step_prepare): Assert that trap_expected is set. Use >> thread_step_over_chain_enqueue. Split starting a new displaced >> step to ... >> (start_step_over): ... this new function. > > If I read this patch correctly, start_step_over is moved from > displaced_step_fixup. That is why we call start_step_over after each > displaced_step_fixup. Correct. > >> v3: >> >> More comments. The step-over chain is now a global instead of >> being per-inferior. Previous versions had actually broken >> multiple-processes displaced stepping at the same time. Added new > > How does per-inferior step-over chain (or displaced stepping queue) > break multi-process displaced stepping? v1 and v2 put the head of the step-over chain in the inferior structure. start_step_over would look up the inferior structure of the thread that just finished the step over, and try to start a step-over of another thread of that inferior. And if we had just finished an in-line step over, and the step-over that we could start now is a displaced-step of _another_ inferior, in v2, we wouldn't start it (because start_step_over_inferior wouldn't see that thread). And if we could start a displaced-step for a thread of the event inferior, start_step_over would return immediately, instead of trying to start a displaced step in another inferior too. Even in-line step-overs were broken. E.g., say you have two inferiors, each with one thread. Everything is stopped at a breakpoint that must be stepped over. sched-multi is on, and the user does "continue" to continue both inferiors. We'd start an in-line step-over for inferior 1. Once that is done, we'd try starting a new step over in the same inferior, and we'd miss that the other inferior has a thread to step over too. I went a bit in circles a trying to address that. The fact that for in-line step overs we must stop all threads currently (we should be able to stop only threads sharing the stepped thread's address space instead, but we're not there yet), but not for displaced stepping started making it too complicated. Related I also considered that we could have more that one displaced step scratch pad slot per inferior (e.g., "reserve" a few more bytes around the entry point). Another thing I realized is that per-inferior queue breaks the forward-progress-guarantee ordering, as we'd be giving priority to start step overs on threads of the same inferior that had just finished a step over, potentially starving threads of other inferiors. The ordering issue is that there was no ordering between the step-over chains of the multiple inferiors, so if we left the chain per-inferior, we wouldn't know which inferior's chain had the thread that was waiting for a step-over for the longest time. All in all, I realized that a single list is simpler and more flexible. > >> @@ -2972,35 +2983,17 @@ infrun_thread_stop_requested_callback (struct thread_info *info, void *arg) >> static void >> infrun_thread_stop_requested (ptid_t ptid) >> { >> - struct displaced_step_inferior_state *displaced; >> - >> - /* PTID was requested to stop. Remove it from the displaced >> - stepping queue, so we don't try to resume it automatically. */ >> - >> - for (displaced = displaced_step_inferior_states; >> - displaced; >> - displaced = displaced->next) >> - { >> - struct displaced_step_request *it, **prev_next_p; >> - >> - it = displaced->step_request_queue; >> - prev_next_p = &displaced->step_request_queue; >> - while (it) >> - { >> - if (ptid_match (it->ptid, ptid)) >> - { >> - *prev_next_p = it->next; >> - it->next = NULL; >> - xfree (it); >> - } >> - else >> - { >> - prev_next_p = &it->next; >> - } >> + struct thread_info *tp; >> >> - it = *prev_next_p; >> - } >> - } >> + /* PTID was requested to stop. Remove matching threads from the >> + step-over queue, so we don't try to resume them >> + automatically. */ > > I can understand the code below, except the comment "we don't try to > resume them automatically". It looks not necessary here. By "resumed automatically" I meant that if thread A is left in the step-over chain (or currently in mainline in the displaced step queue) it ends up stepped when all others threads in the step over queue are done with their steps, even if thread A is supposed to be stopped. But I agree it's not really necessary. I'll remove it. > >> + ALL_NON_EXITED_THREADS (tp) >> + if (ptid_match (tp->ptid, ptid)) >> + { >> + if (thread_is_in_step_over_chain (tp)) >> + thread_step_over_chain_remove (tp); >> + } >> >> iterate_over_threads (infrun_thread_stop_requested_callback, &ptid); >> } >> @@ -4051,6 +4044,9 @@ Cannot fill $_exitsignal with the correct signal number.\n")); >> that this operation also cleans up the child process for vfork, >> because their pages are shared. */ >> displaced_step_fixup (ecs->ptid, GDB_SIGNAL_TRAP); >> + /* Start a new step-over in another thread if there's one >> + that needs it. */ >> + start_step_over (); > > The comment is confusing to me, especially the "one" and the "it". Do > you mean "in another thread if there is one thread that needs step-over"? > >> @@ -323,6 +403,10 @@ delete_thread_1 (ptid_t ptid, int silent) >> if (!tp) >> return; >> >> + /* Dead threads don't need to step-over. Remove from queue. */ >> + if (tp->step_over_next != NULL) >> + thread_step_over_chain_remove (tp); >> + > > I am wondering how this can happen? A thread needs step-over becomes dead? It can happen if the process exits or disappears (target is closed, etc.) while the thread was waiting for its turn. Thanks, Pedro Alves