From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24526 invoked by alias); 13 Apr 2012 19:46:09 -0000 Received: (qmail 24515 invoked by uid 22791); 13 Apr 2012 19:46:08 -0000 X-SWARE-Spam-Status: No, hits=-4.1 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,TW_EG X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 13 Apr 2012 19:45:45 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1SImRE-0004wk-H0 from Luis_Gustavo@mentor.com ; Fri, 13 Apr 2012 12:45:44 -0700 Received: from NA1-MAIL.mgc.mentorg.com ([147.34.98.181]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Fri, 13 Apr 2012 12:45:19 -0700 Received: from [0.0.0.0] ([172.16.63.104]) by NA1-MAIL.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Fri, 13 Apr 2012 12:45:40 -0700 Message-ID: <4F888270.20209@mentor.com> Date: Fri, 13 Apr 2012 19:54:00 -0000 From: Luis Gustavo Reply-To: "Gustavo, Luis" User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.27) Gecko/20120216 Lightning/1.0b2 Thunderbird/3.1.19 MIME-Version: 1.0 To: Pedro Alves CC: gdb-patches@sourceware.org Subject: Re: [PATCH] Fix displaced stepping for remote targets References: <4F67E54C.1010904@mentor.com> <4F8861C0.3050805@redhat.com> In-Reply-To: <4F8861C0.3050805@redhat.com> Content-Type: multipart/mixed; boundary="------------050002040101080103030302" X-IsSubscribed: yes 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 X-SW-Source: 2012-04/txt/msg00382.txt.bz2 This is a multi-part message in MIME format. --------------050002040101080103030302 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-length: 4707 On 04/13/2012 02:26 PM, Pedro Alves wrote: > On 03/20/2012 02:02 AM, Luis Gustavo wrote: > >> Hi, >> >> While debugging a remote target that supports hardware single-stepping with a GDB that supports displaced stepping, i've ran into the following problem... >> >> When reaching a breakpoint, GDB should take care of relocating the instruction underneath that breakpoint to the scratch space where it will be executed out-of-line. >> >> If a target supports hw single-stepping for displaced stepping, GDB should just send a vCont;s packet to tell the target to step a single instruction. In my case, GDB was always sending a vCont;c instead. >> >> I've tracked it down to infrun.c:resume, where we have this check: >> >> if (gdbarch_cannot_step_breakpoint (gdbarch)) >> { >> /* Most targets can step a breakpoint instruction, thus >> executing it normally. But if this one cannot, just >> continue and we will hit it anyway. */ >> if (step&& breakpoint_inserted_here_p (aspace, pc)) >> step = 0; >> } >> >> My target can't step breakpoints and, if we're doing displaced stepping, it's because we're trying to step off a breakpoint, thus breakpoint_inserted_here_p returns true, and we disable single-stepping by setting step to 0. > > > That is key. Most targets can step breakpoints. The issue is reproducible on x86_64 GNU/Linux > with this: > > diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c > index da22c1c..c98773e 100644 > --- a/gdb/amd64-linux-tdep.c > +++ b/gdb/amd64-linux-tdep.c > @@ -1300,6 +1300,8 @@ amd64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch) > /* Reserve a number for orig_rax. */ > set_gdbarch_num_regs (gdbarch, AMD64_LINUX_NUM_REGS); > > + set_gdbarch_cannot_step_breakpoint (gdbarch, 1); > + > if (! tdesc_has_registers (tdesc)) > tdesc = tdesc_amd64_linux; > tdep->tdesc = tdesc; > > >> >> It seems to me we need to update the PC prior to calling breakpoint_inserted_here_p since the displaced stepping machinery adjusted the old PC to point to the space in the scratch area. That way we can properly command the target to step the displaced instruction and we can check for breakpoints at the real execution place. >> > > > Yeah. > >> The following patch fixes this by pushing the if block further down in the code and taking care of updating PC if displaced stepping is being used. >> >> I've regtested this on x86 and everything looks OK. This also makes GDB send vCont;s now. >> >> Ok? > > > No... > >> Index: HEAD-git/gdb/infrun.c >> =================================================================== >> --- HEAD-git.orig/gdb/infrun.c 2012-03-19 20:40:57.589621232 -0300 >> +++ HEAD-git/gdb/infrun.c 2012-03-19 22:12:08.729898472 -0300 >> @@ -1884,15 +1884,6 @@ a command like `return' or `jump' to con >> resume_ptid = inferior_ptid; >> } >> >> - if (gdbarch_cannot_step_breakpoint (gdbarch)) >> - { >> - /* Most targets can step a breakpoint instruction, thus >> - executing it normally. But if this one cannot, just >> - continue and we will hit it anyway. */ >> - if (step&& breakpoint_inserted_here_p (aspace, pc)) >> - step = 0; >> - } >> - >> if (debug_displaced >> && use_displaced_stepping (gdbarch) >> && tp->control.trap_expected) >> @@ -1902,12 +1893,25 @@ a command like `return' or `jump' to con >> CORE_ADDR actual_pc = regcache_read_pc (resume_regcache); >> gdb_byte buf[4]; >> >> + /* Update pc to reflect the new address from which we will execute >> + instructions due to displaced stepping. */ >> + pc = actual_pc; > > > Notice the condition just above: > > if (debug_displaced > ^^^^^^^^^^^^^^^ > && use_displaced_stepping (gdbarch) > && tp->control.trap_expected) > { > struct regcache *resume_regcache = get_thread_regcache (resume_ptid); > struct gdbarch *resume_gdbarch = get_regcache_arch (resume_regcache); > CORE_ADDR actual_pc = regcache_read_pc (resume_regcache); > gdb_byte buf[4]; > > /* Update pc to reflect the new address from which we will execute > instructions due to displaced stepping. */ > pc = actual_pc; > > This this part of the fix is only working if you do "set debug displaced on". > Not good. :-) Best do this right after the displaced_step_prepare call? > Doh! Of course... This block looked more functional than a simple debug output block. No surprise it works while debugging a displaced stepping issue. :-) Thanks for spotting that. How about the attached one? Luis --------------050002040101080103030302 Content-Type: text/x-patch; name="0001-fix_remote_displaced_stepping.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="0001-fix_remote_displaced_stepping.diff" Content-length: 831 2012-04-13 Luis Machado * infrun.c (resume): Update PC address to the real PC after preparing to do displaced stepping. Index: HEAD-git/gdb/infrun.c =================================================================== --- HEAD-git.orig/gdb/infrun.c 2012-04-13 16:01:25.592842215 -0300 +++ HEAD-git/gdb/infrun.c 2012-04-13 16:22:53.652909294 -0300 @@ -1789,6 +1789,10 @@ a command like `return' or `jump' to con return; } + /* Update pc to reflect the new address from which we will execute + instructions due to displaced stepping. */ + pc = regcache_read_pc (get_thread_regcache (inferior_ptid)); + displaced = get_displaced_stepping_state (ptid_get_pid (inferior_ptid)); step = gdbarch_displaced_step_hw_singlestep (gdbarch, displaced->step_closure); --------------050002040101080103030302--