From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 35639 invoked by alias); 21 Apr 2015 08:28:19 -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 35623 invoked by uid 89); 21 Apr 2015 08:28:18 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.3 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-pa0-f51.google.com Received: from mail-pa0-f51.google.com (HELO mail-pa0-f51.google.com) (209.85.220.51) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Tue, 21 Apr 2015 08:28:17 +0000 Received: by pacyx8 with SMTP id yx8so234205271pac.1 for ; Tue, 21 Apr 2015 01:28:15 -0700 (PDT) X-Received: by 10.68.106.101 with SMTP id gt5mr24381928pbb.60.1429604895199; Tue, 21 Apr 2015 01:28:15 -0700 (PDT) Received: from E107787-LIN (gcc1-power7.osuosl.org. [140.211.15.137]) by mx.google.com with ESMTPSA id w1sm1260998pdp.25.2015.04.21.01.28.12 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Tue, 21 Apr 2015 01:28:14 -0700 (PDT) From: Yao Qi To: Pedro Alves 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> Date: Tue, 21 Apr 2015 08:28:00 -0000 In-Reply-To: <1429267521-21047-6-git-send-email-palves@redhat.com> (Pedro Alves's message of "Fri, 17 Apr 2015 11:45:09 +0100") Message-ID: <86mw21zy0e.fsf@gmail.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2015-04/txt/msg00759.txt.bz2 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. > 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? > @@ -2972,35 +2983,17 @@ infrun_thread_stop_requested_callback (struct thr= ead_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 =3D displaced_step_inferior_states; > - displaced; > - displaced =3D displaced->next) > - { > - struct displaced_step_request *it, **prev_next_p; > - > - it =3D displaced->step_request_queue; > - prev_next_p =3D &displaced->step_request_queue; > - while (it) > - { > - if (ptid_match (it->ptid, ptid)) > - { > - *prev_next_p =3D it->next; > - it->next =3D NULL; > - xfree (it); > - } > - else > - { > - prev_next_p =3D &it->next; > - } > + struct thread_info *tp; >=20=20 > - it =3D *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. > + 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); > + } >=20=20 > iterate_over_threads (infrun_thread_stop_requested_callback, &ptid); > } > @@ -4051,6 +4044,9 @@ Cannot fill $_exitsignal with the correct signal nu= mber.\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; >=20=20 > + /* Dead threads don't need to step-over. Remove from queue. */ > + if (tp->step_over_next !=3D NULL) > + thread_step_over_chain_remove (tp); > + I am wondering how this can happen? A thread needs step-over becomes dead? > /* If this is the current thread, or there's code out there that > relies on it existing (refcount > 0) we can't delete yet. Mark > it as exited, and notify it. */ --=20 Yao (=E9=BD=90=E5=B0=A7)