From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8121 invoked by alias); 8 Jan 2010 16:24:16 -0000 Received: (qmail 8109 invoked by uid 22791); 8 Jan 2010 16:24:14 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 08 Jan 2010 16:24:08 +0000 Received: (qmail 1375 invoked from network); 8 Jan 2010 16:24:06 -0000 Received: from unknown (HELO orlando) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 8 Jan 2010 16:24:06 -0000 From: Pedro Alves To: Hui Zhu Subject: Re: [RFC] Add support of software single step to process record Date: Fri, 08 Jan 2010 16:24:00 -0000 User-Agent: KMail/1.9.10 Cc: shuchang zhou , gdb-patches@sourceware.org, Joel Brobecker , Michael Snyder , paawan oza , Tom Tromey References: <200912241738.19780.pedro@codesourcery.com> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <201001081624.12634.pedro@codesourcery.com> 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: 2010-01/txt/msg00176.txt.bz2 On Monday 04 January 2010 14:23:21, Hui Zhu wrote: > Sorry guys, the prev patch is so ugly. :-) > Thanks for teach me clear about the gdbarch_software_single_step, Pedro. > I did some extend with your idea. Because record_wait need > record_resume_step point out this resume is signal step or continue. > > if (!step) > { > /* This is not hard single step. */ > if (!gdbarch_software_single_step_p (gdbarch)) > { > /* This is a normal continue. */ > step = 1; > } > else > { > /* This arch support soft sigle step. */ > if (single_step_breakpoints_inserted ()) > { > /* This is a soft single step. */ > record_resume_step = 1; > } > else > { > /* This is a continue. > Try to insert a soft single step breakpoint. */ > if (!gdbarch_software_single_step (gdbarch, > get_current_frame ())) > { > /* This system don't want use soft single step. > Use hard sigle step. */ > step = 1; > } > } > } > } Cool, this looks pretty clear to me now. Thanks. > @@ -1077,6 +1111,7 @@ record_wait (struct target_ops *ops, > /* This is not a single step. */ > ptid_t ret; > CORE_ADDR tmp_pc; > + struct gdbarch *gdbarch = target_thread_architecture (inferior_ptid); > > while (1) > { > @@ -1099,6 +1134,9 @@ record_wait (struct target_ops *ops, > tmp_pc = regcache_read_pc (regcache); > aspace = get_regcache_aspace (regcache); > > + if (gdbarch_software_single_step_p (gdbarch)) > + remove_single_step_breakpoints (); This will gdb_assert inside remove_single_step_breakpoints if SSS bkpts are not inserted, but gdbarch_software_single_step_p returns true. This instead is safer: if (single_step_breakpoints_inserted ()) remove_single_step_breakpoints (); But, what if it was infrun that had inserted the single-step breakpoints, for a "next" or "step", etc.? Shouldn't you check for record_resume_step too? if (!record_resume_step && single_step_breakpoints_inserted ()) remove_single_step_breakpoints (); Otherwise, the check below for else if (breakpoint_inserted_here_p (aspace, tmp_pc)) { /* There is a breakpoint here. Let the core handle it. */ if (software_breakpoint_inserted_here_p (aspace, tmp_pc)) { would fail, and the finished single-step wouldn't be reported to the core, right? Lastly, you may also want to confirm that the SSS bkpt managed by record.d itself explains the SIGTRAP before removing before issueing another single-step. If any unexplainable SIGTRAP happens for any reason while single-stepping, you should report it to infrun instead. In other words: With software single-stepping, we can distinguish most random SIGTRAPs from SSS SIGTRAPs, so: /* This must be a single-step trap. Record the insn and issue another step. */ ... the "must" here ends up being a bit too strong. I'd certainly understand ignoring this for simplicity or performance reasons though. Otherwise, looks good to me. -- Pedro Alves