| Message ID | 1428569112-18004-1-git-send-email-qiyaoltc@gmail.com |
|---|---|
| State | New, archived |
| Headers |
Received: (qmail 92997 invoked by alias); 9 Apr 2015 08:45:21 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 92986 invoked by uid 89); 9 Apr 2015 08:45:20 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.6 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-f41.google.com Received: from mail-pa0-f41.google.com (HELO mail-pa0-f41.google.com) (209.85.220.41) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Thu, 09 Apr 2015 08:45:19 +0000 Received: by pabsx10 with SMTP id sx10so143038523pab.3 for <gdb-patches@sourceware.org>; Thu, 09 Apr 2015 01:45:17 -0700 (PDT) X-Received: by 10.66.237.35 with SMTP id uz3mr54549396pac.46.1428569117668; Thu, 09 Apr 2015 01:45:17 -0700 (PDT) Received: from E107787-LIN.cambridge.arm.com (gcc1-power7.osuosl.org. [140.211.15.137]) by mx.google.com with ESMTPSA id c16sm13733326pdl.61.2015.04.09.01.45.16 for <gdb-patches@sourceware.org> (version=TLSv1.2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Thu, 09 Apr 2015 01:45:16 -0700 (PDT) From: Yao Qi <qiyaoltc@gmail.com> To: gdb-patches@sourceware.org Subject: [PATCH] gdbserver gnu/linux: stepping over breakpoint Date: Thu, 9 Apr 2015 09:45:12 +0100 Message-Id: <1428569112-18004-1-git-send-email-qiyaoltc@gmail.com> X-IsSubscribed: yes |
Commit Message
Yao Qi
April 9, 2015, 8:45 a.m. UTC
From: Yao Qi <yao.qi@linaro.org> Hi, I see the following error on arm linux gdbserver, continue^M Continuing.^M ../../../binutils-gdb/gdb/gdbserver/linux-arm-low.c:458: A problem internal to GDBserver has been detected.^M raw_bkpt_type_to_arm_hwbp_type: unhandled raw type^M Remote connection closed^M (gdb) FAIL: gdb.base/cond-eval-mode.exp: hbreak: continue After we make GDBserver handling Zx/zx packet idempotent, [PATCH 3/3] [GDBserver] Make Zx/zx packet handling idempotent. https://sourceware.org/ml/gdb-patches/2014-04/msg00480.html > Now removal/insertion of all kinds of breakpoints/watchpoints, either > internal, or from GDB, always go through the target methods. GDBserver handles all kinds of breakpoints/watchpoints through target methods. However, some target backends, such as arm, don't support Z0 packet but need software breakpoint to do breakpoint stepping over in linux-low.c:start_step_over, if (can_hardware_single_step ()) { step = 1; } else { CORE_ADDR raddr = (*the_low_target.breakpoint_reinsert_addr) (); set_reinsert_breakpoint (raddr); step = 0; } a software breakpoint is requested to the backend, and the error is triggered. This problem should affect targets having breakpoint_reinsert_addr hooked. Instead of handling memory breakpoint in these affected linux backend, this patch handles memory breakpoint in linux_{insert,remove}_point, that, if memory breakpoint is requested, call {insert,remove}_memory_breakpoint respectively. Then, it becomes unnecessary to handle memory breakpoint for linux x86 backend, so this patch removes the code there. This patch is tested with GDBserver on x86_64-linux and arm-linux (-marm, -mthumb). Testing this patch on mips, nios2, bfin, and sparc is welcome. Note that there are still some fails in gdb.base/cond-eval-mode.exp with -mthumb, because GDBserver doesn't know how to select the correct breakpoint instruction according to the arm-or-thumb-mode of requested address. This is a separate issue, anyway. gdb/gdbserver: 2015-04-09 Yao Qi <yao.qi@linaro.org> * linux-low.c (linux_insert_point): Call insert_memory_breakpoint if TYPE is raw_bkpt_type_sw. (linux_remove_point): Call remove_memory_breakpoint if type is raw_bkpt_type_sw. * linux-x86-low.c (x86_insert_point): Don't call insert_memory_breakpoint. (x86_remove_point): Don't call remove_memory_breakpoint. --- gdb/gdbserver/linux-low.c | 8 ++++++-- gdb/gdbserver/linux-x86-low.c | 6 ------ 2 files changed, 6 insertions(+), 8 deletions(-)
Comments
On 04/09/2015 09:45 AM, Yao Qi wrote: > gdb/gdbserver: > > 2015-04-09 Yao Qi <yao.qi@linaro.org> > > * linux-low.c (linux_insert_point): Call > insert_memory_breakpoint if TYPE is raw_bkpt_type_sw. > (linux_remove_point): Call remove_memory_breakpoint if type is > raw_bkpt_type_sw. > * linux-x86-low.c (x86_insert_point): Don't call > insert_memory_breakpoint. > (x86_remove_point): Don't call remove_memory_breakpoint. LGTM. Thanks, Pedro Alves
On 09/04/15 10:04, Pedro Alves wrote: > On 04/09/2015 09:45 AM, Yao Qi wrote: > >> gdb/gdbserver: >> >> 2015-04-09 Yao Qi <yao.qi@linaro.org> >> >> * linux-low.c (linux_insert_point): Call >> insert_memory_breakpoint if TYPE is raw_bkpt_type_sw. >> (linux_remove_point): Call remove_memory_breakpoint if type is >> raw_bkpt_type_sw. >> * linux-x86-low.c (x86_insert_point): Don't call >> insert_memory_breakpoint. >> (x86_remove_point): Don't call remove_memory_breakpoint. > > LGTM. > Patch is pushed in.
On 04/09/2015 09:45 AM, Yao Qi wrote: > From: Yao Qi <yao.qi@linaro.org> > > Hi, > I see the following error on arm linux gdbserver, > > continue^M > Continuing.^M > ../../../binutils-gdb/gdb/gdbserver/linux-arm-low.c:458: A problem internal to GDBserver has been detected.^M > raw_bkpt_type_to_arm_hwbp_type: unhandled raw type^M > Remote connection closed^M > (gdb) FAIL: gdb.base/cond-eval-mode.exp: hbreak: continue > > After we make GDBserver handling Zx/zx packet idempotent, > > [PATCH 3/3] [GDBserver] Make Zx/zx packet handling idempotent. > https://sourceware.org/ml/gdb-patches/2014-04/msg00480.html > >> Now removal/insertion of all kinds of breakpoints/watchpoints, either >> internal, or from GDB, always go through the target methods. > > GDBserver handles all kinds of breakpoints/watchpoints through target > methods. However, some target backends, such as arm, don't support Z0 > packet but need software breakpoint to do breakpoint stepping over in > linux-low.c:start_step_over, > > if (can_hardware_single_step ()) > { > step = 1; > } > else > { > CORE_ADDR raddr = (*the_low_target.breakpoint_reinsert_addr) (); > set_reinsert_breakpoint (raddr); > step = 0; > } > > a software breakpoint is requested to the backend, and the error is > triggered. This problem should affect targets having > breakpoint_reinsert_addr hooked. BTW, I think the patch is OK on its own right, as it would be needed to step over thread event breakpoints on older kernels (that's all breakpoint_reinsert_addr handles, see comment above start_step_over), but gdbserver shouldn't be using those event breakpoints nowadays (thread_db_use_events ends up false), so it's surprising that that's even reached. The test isn't even threaded. It sounds like gdbserver is trying to step over the breakpoint at "foo"? Didn't gdb itself step over it? How come that was reached in gdbserver? Did we mishandle the breakpoint's reference count in gdbserver? Thanks, Pedro Alves
Pedro Alves <palves@redhat.com> writes: > even reached. The test isn't even threaded. It sounds like > gdbserver is trying to step over the breakpoint at "foo"? Didn't > gdb itself step over it? How come that was reached in gdbserver? > Did we mishandle the breakpoint's reference count in gdbserver? Shouldn't GDBserver step over breakpoint when the target side condition is false?
On 04/09/2015 04:06 PM, Yao Qi wrote: > Pedro Alves <palves@redhat.com> writes: > >> even reached. The test isn't even threaded. It sounds like >> gdbserver is trying to step over the breakpoint at "foo"? Didn't >> gdb itself step over it? How come that was reached in gdbserver? >> Did we mishandle the breakpoint's reference count in gdbserver? > > Shouldn't GDBserver step over breakpoint when the target side condition > is false? Oh, this is stepping past an hardware breakpoint, not software breakpoint. Yes, GDBserver should be stepping past such breakpoints. But, given GDBserver's software single-step support is really really really really too simple: /* We only place breakpoints in empty marker functions, and thread locking is outside of the function. So rather than importing software single-step, we can just run until exit. */ static CORE_ADDR arm_reinsert_addr (void) { struct regcache *regcache = get_thread_regcache (current_thread, 1); unsigned long pc; collect_register_by_name (regcache, "lr", &pc); return pc; } ... we should probably disable target side conditions on software single-step gdbserver ports. E.g., try "si" through this function: void function () { i = 0; i = 0; // set cond breakpoint that evals false here i = 0; } I'd guess the "si" over the breakpoint ends in the caller of "function"... Thanks, Pedro Alves
On 04/09/2015 04:22 PM, Pedro Alves wrote: > On 04/09/2015 04:06 PM, Yao Qi wrote: >> Pedro Alves <palves@redhat.com> writes: >> >>> even reached. The test isn't even threaded. It sounds like >>> gdbserver is trying to step over the breakpoint at "foo"? Didn't >>> gdb itself step over it? How come that was reached in gdbserver? >>> Did we mishandle the breakpoint's reference count in gdbserver? >> >> Shouldn't GDBserver step over breakpoint when the target side condition >> is false? > > Oh, this is stepping past an hardware breakpoint, not software > breakpoint. Yes, GDBserver should be stepping past such > breakpoints. But, given GDBserver's software single-step > support is really really really really too simple: > > /* We only place breakpoints in empty marker functions, and thread locking > is outside of the function. So rather than importing software single-step, > we can just run until exit. */ > static CORE_ADDR > arm_reinsert_addr (void) > { > struct regcache *regcache = get_thread_regcache (current_thread, 1); > unsigned long pc; > collect_register_by_name (regcache, "lr", &pc); > return pc; > } > > ... we should probably disable target side conditions on software > single-step gdbserver ports. E.g., try "si" through this function: Sorry, "si" probably works as gdb steps over the breakpoint itself. Try "step" or "next" instead, which kick in the range stepping support, which then causes gdbserver to handle the step-over itself. > > void > function () > { > i = 0; > i = 0; // set cond breakpoint that evals false here > i = 0; > } > > I'd guess the "si" over the breakpoint ends in the caller > of "function"... Thanks, Pedro Alves
On 04/09/2015 04:24 PM, Pedro Alves wrote: > On 04/09/2015 04:22 PM, Pedro Alves wrote: >> On 04/09/2015 04:06 PM, Yao Qi wrote: >>> Pedro Alves <palves@redhat.com> writes: >>> >>>> even reached. The test isn't even threaded. It sounds like >>>> gdbserver is trying to step over the breakpoint at "foo"? Didn't >>>> gdb itself step over it? How come that was reached in gdbserver? >>>> Did we mishandle the breakpoint's reference count in gdbserver? >>> >>> Shouldn't GDBserver step over breakpoint when the target side condition >>> is false? >> >> Oh, this is stepping past an hardware breakpoint, not software >> breakpoint. Yes, GDBserver should be stepping past such >> breakpoints. But, given GDBserver's software single-step >> support is really really really really too simple: >> >> /* We only place breakpoints in empty marker functions, and thread locking >> is outside of the function. So rather than importing software single-step, >> we can just run until exit. */ >> static CORE_ADDR >> arm_reinsert_addr (void) >> { >> struct regcache *regcache = get_thread_regcache (current_thread, 1); >> unsigned long pc; >> collect_register_by_name (regcache, "lr", &pc); >> return pc; >> } >> >> ... we should probably disable target side conditions on software >> single-step gdbserver ports. E.g., try "si" through this function: > > Sorry, "si" probably works as gdb steps over the breakpoint > itself. Try "step" or "next" instead, which kick in the range > stepping support, which then causes gdbserver to handle the > step-over itself. Hmm^2, no, range stepping couldn't possibly work correctly with that limited software single-step support. Duh. We only enable range stepping on x86, even. So maybe that's "good enough" for stepping past conditional breakpoints in practice. Guess we should update the comments to mention this. Thanks, Pedro Alves
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c index e4c5420..6dd9224 100644 --- a/gdb/gdbserver/linux-low.c +++ b/gdb/gdbserver/linux-low.c @@ -5115,7 +5115,9 @@ static int linux_insert_point (enum raw_bkpt_type type, CORE_ADDR addr, int size, struct raw_breakpoint *bp) { - if (the_low_target.insert_point != NULL) + if (type == raw_bkpt_type_sw) + return insert_memory_breakpoint (bp); + else if (the_low_target.insert_point != NULL) return the_low_target.insert_point (type, addr, size, bp); else /* Unsupported (see target.h). */ @@ -5126,7 +5128,9 @@ static int linux_remove_point (enum raw_bkpt_type type, CORE_ADDR addr, int size, struct raw_breakpoint *bp) { - if (the_low_target.remove_point != NULL) + if (type == raw_bkpt_type_sw) + return remove_memory_breakpoint (bp); + else if (the_low_target.remove_point != NULL) return the_low_target.remove_point (type, addr, size, bp); else /* Unsupported (see target.h). */ diff --git a/gdb/gdbserver/linux-x86-low.c b/gdb/gdbserver/linux-x86-low.c index e293ba4..763df08 100644 --- a/gdb/gdbserver/linux-x86-low.c +++ b/gdb/gdbserver/linux-x86-low.c @@ -561,9 +561,6 @@ x86_insert_point (enum raw_bkpt_type type, CORE_ADDR addr, switch (type) { - case raw_bkpt_type_sw: - return insert_memory_breakpoint (bp); - case raw_bkpt_type_hw: case raw_bkpt_type_write_wp: case raw_bkpt_type_access_wp: @@ -590,9 +587,6 @@ x86_remove_point (enum raw_bkpt_type type, CORE_ADDR addr, switch (type) { - case raw_bkpt_type_sw: - return remove_memory_breakpoint (bp); - case raw_bkpt_type_hw: case raw_bkpt_type_write_wp: case raw_bkpt_type_access_wp: