From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 97740 invoked by alias); 9 Nov 2015 11:24: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 97720 invoked by uid 89); 9 Nov 2015 11:24:06 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.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-f52.google.com Received: from mail-pa0-f52.google.com (HELO mail-pa0-f52.google.com) (209.85.220.52) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Mon, 09 Nov 2015 11:24:04 +0000 Received: by padhx2 with SMTP id hx2so188461408pad.1 for ; Mon, 09 Nov 2015 03:24:03 -0800 (PST) X-Received: by 10.68.111.101 with SMTP id ih5mr39959161pbb.84.1447068243007; Mon, 09 Nov 2015 03:24:03 -0800 (PST) Received: from E107787-LIN (gcc2-power8.osuosl.org. [140.211.9.43]) by smtp.gmail.com with ESMTPSA id cx5sm15828198pbc.50.2015.11.09.03.23.58 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Mon, 09 Nov 2015 03:24:01 -0800 (PST) From: Yao Qi To: Pedro Alves Cc: Yao Qi , gdb-patches@sourceware.org Subject: Re: [PATCH 2/2] Fix gdb.threads/multiple-step-overs.exp fails on arm References: <1446130862-12824-1-git-send-email-yao.qi@linaro.org> <1446130862-12824-3-git-send-email-yao.qi@linaro.org> <563A3E3F.9060204@redhat.com> Date: Mon, 09 Nov 2015 11:24:00 -0000 In-Reply-To: <563A3E3F.9060204@redhat.com> (Pedro Alves's message of "Wed, 04 Nov 2015 17:19:59 +0000") Message-ID: <86d1vjxvp5.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-11/txt/msg00224.txt.bz2 Pedro Alves writes: Hi Pedro, >> GDB resumes the whole process (all threads) rather than the specific >> thread it wants to single step (as shown in [1]). That is wrong. > > (I understand this, but I think it'd make it clearer to explicitly > state _why_ that is wrong.) I am not sure how to make the description clearer, how about this? GDB resumes the whole process (all threads) rather than the specific thread for which GDB wants to step over the breakpoint (as shown in [1]). That is wrong. >> diff --git a/gdb/infrun.c b/gdb/infrun.c >> index 0265d35..c619b61 100644 >> --- a/gdb/infrun.c >> +++ b/gdb/infrun.c >> @@ -2631,14 +2631,12 @@ resume (enum gdb_signal sig) >> gdb_assert (!(thread_has_single_step_breakpoints_set (tp) && step)); >>=20=20 >> /* Decide the set of threads to ask the target to resume. */ >> - if ((step || thread_has_single_step_breakpoints_set (tp)) >> + if ((step || thread_has_single_step_breakpoints_set (tp) >> + || displaced_step_in_progress_thread (tp->ptid)) >> && tp->control.trap_expected) > > > I wonder, can't we just remove the "step" check, like: > > if (tp->control.trap_expected) > { > Yes, it works! as the patch below, > > >> { >> /* We're allowing a thread to run past a breakpoint it has >> - hit, by single-stepping the thread with the breakpoint >> - removed. In which case, we need to single-step only this >> - thread, and keep others stopped, as they can miss this >> - breakpoint if allowed to run. */ >> + hit, by single-stepping (in-line or out-of-line) the thread. */ > > The change looks good to me, though I think we should clarify > the comment here. How about: > > /* We're allowing a thread to run past a breakpoint it has > hit, either by single-stepping the thread with the breakpoint > removed, or by displaced stepping, with the breakpoint inserted. > In the former case, we need to single-step only this thread, and= keep > others stopped, as they can miss this breakpoint if allowed to r= un. > That's not really a problem for displaced stepping, but, we stil= l keep > other threads stopped, in case another thread is also stopped fo= r a > breakpoint waiting for its turn in the displaced stepping queue.= */ > They are copied into the patch. > I think we could optimize this by checking for > thread_step_over_chain_next (tp) =3D=3D NULL, because if no other thread > is waiting for a step-over, then we could resume all threads, but > that's maybe not worth it. 'thread_step_over_chain_next (tp) =3D=3D NULL' doesn't work on arm-linux, however, I didn't investigate why it doesn't. Patch below is regression tested on x86_64-linux and arm-linux. --=20 Yao (=E9=BD=90=E5=B0=A7) =46rom 9f1915b1d183b14286b1e785dec84d5c95f8ff5c Mon Sep 17 00:00:00 2001 From: Yao Qi Date: Wed, 28 Oct 2015 11:58:34 +0000 Subject: [PATCH] Fix gdb.threads/multiple-step-overs.exp fails on arm Hi, Some tests in gdb.threads/multiple-step-overs.exp fail on arm target when the displaced stepping on, but they pass when displaced stepping is off. FAIL: gdb.threads/multiple-step-overs.exp: displaced=3Don: step: step FAIL: gdb.threads/multiple-step-overs.exp: displaced=3Don: next: next FAIL: gdb.threads/multiple-step-overs.exp: displaced=3Don: continue: conti= nue FAIL: gdb.threads/multiple-step-overs.exp: displaced=3Don: signal thr1: co= ntinue to sigusr1_handler when displaced stepping is on, Sending packet: $vCont;c#a8...infrun: infrun_async(1)^M <--- [1] infrun: prepare_to_wait^M infrun: target_wait (-1.0.0, status) =3D^M infrun: -1.0.0 [Thread 0],^M infrun: status->kind =3D ignore^M infrun: TARGET_WAITKIND_IGNORE^M infrun: prepare_to_wait^M Packet received: T05swbreak:;0b:f8faffbe;0d:409ee7b6;0f:d0880000;thread:p63= 5.636;core:0;^M infrun: target_wait (-1.0.0, status) =3D^M infrun: 1589.1590.0 [Thread 1590],^M infrun: status->kind =3D stopped, signal =3D GDB_SIGNAL_TRAP^M infrun: TARGET_WAITKIND_STOPPED^M infrun: stop_pc =3D 0x88d0^M infrun: context switch^M infrun: Switching context from Thread 1591 to Thread 1590^ GDB resumes the whole process (all threads) rather than the specific thread for which GDB wants to step over the breakpoint (as shown in [1]). That is wrong. when displaced stepping is off, GDB behaves correctly, only resumes the specific thread (as shown in [2]). Sending packet: $vCont;c:p611.613#b2...infrun: infrun_async(1)^M <-- [2] infrun: prepare_to_wait^M infrun: target_wait (-1.0.0, status) =3D^M infrun: -1.0.0 [Thread 0],^M infrun: status->kind =3D ignore^M infrun: TARGET_WAITKIND_IGNORE^M infrun: prepare_to_wait^M Packet received: T05swbreak:;0b:f8faffbe;0d:409e67b6;0f:48880000;thread:p61= 1.613;core:1;^M infrun: target_wait (-1.0.0, status) =3D^M infrun: 1553.1555.0 [Thread 1555],^M infrun: status->kind =3D stopped, signal =3D GDB_SIGNAL_TRAP^M infrun: TARGET_WAITKIND_STOPPED^M infrun: clear_step_over_info^M infrun: stop_pc =3D 0x8848 The current logic in GDB on deciding the set of threads to resume is: /* Decide the set of threads to ask the target to resume. */ if ((step || thread_has_single_step_breakpoints_set (tp)) && tp->control.trap_expected) { /* We're allowing a thread to run past a breakpoint it has hit, by single-stepping the thread with the breakpoint removed. In which case, we need to single-step only this thread, and keep others stopped, as they can miss this breakpoint if allowed to run. */ resume_ptid =3D inferior_ptid; } else resume_ptid =3D internal_resume_ptid (user_step); it doesn't handle the case correctly that GDB continue (instead of single step) the thread for displaced stepping. I also update the comment below to reflect the code. I remove the "with the breakpoint removed" comment, because GDB doesn't remove breakpoints in displaced stepping, so we don't have to worry that other threads may miss the breakpoint. Patch is regression tested on both x86_64-linux and arm-linux. gdb: 2015-11-09 Yao Qi * infrun.c (resume): Check control.trap_expected only when deciding the set of threads to resume. diff --git a/gdb/infrun.c b/gdb/infrun.c index 185b79b..4a66d17 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -2654,14 +2654,17 @@ resume (enum gdb_signal sig) gdb_assert (!(thread_has_single_step_breakpoints_set (tp) && step)); =20 /* Decide the set of threads to ask the target to resume. */ - if ((step || thread_has_single_step_breakpoints_set (tp)) - && tp->control.trap_expected) + if (tp->control.trap_expected) { /* We're allowing a thread to run past a breakpoint it has - hit, by single-stepping the thread with the breakpoint - removed. In which case, we need to single-step only this - thread, and keep others stopped, as they can miss this - breakpoint if allowed to run. */ + hit, either by single-stepping the thread with the breakpoint + removed, or by displaced stepping, with the breakpoint inserted. + In the former case, we need to single-step only this thread, + and keep others stopped, as they can miss this breakpoint if + allowed to run. That's not really a problem for displaced + stepping, but, we still keep other threads stopped, in case + another thread is also stopped for a breakpoint waiting for + its turn in the displaced stepping queue. */ resume_ptid =3D inferior_ptid; } else