From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8055 invoked by alias); 11 Apr 2009 17:16:23 -0000 Received: (qmail 8046 invoked by uid 22791); 11 Apr 2009 17:16:22 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 11 Apr 2009 17:16:17 +0000 Received: (qmail 16604 invoked from network); 11 Apr 2009 17:16:15 -0000 Received: from unknown (HELO orlando.local) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 11 Apr 2009 17:16:15 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [Patch 1/2] infrun.c support for MIPS hardware watchpoints. Date: Sat, 11 Apr 2009 17:16:00 -0000 User-Agent: KMail/1.9.10 Cc: David Daney , David Daney References: <49D9A5E7.3000003@gmail.com> In-Reply-To: <49D9A5E7.3000003@gmail.com> MIME-Version: 1.0 Content-Disposition: inline Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Message-Id: <200904111817.09949.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: 2009-04/txt/msg00209.txt.bz2 On Monday 06 April 2009 07:49:11, David Daney wrote: > +/* Try to setup for software single stepping over the specified location. > + Return 1 if target_resume() should use hardware single step. > + > + GDBARCH the current gdbarch. > + PC the location to step over. */ > +static int Add an empty line between comment and function, please. > +set_for_singlestep (struct gdbarch *gdbarch, CORE_ADDR pc) > +{ > + int step = 1; > + > + if (gdbarch_software_single_step_p (gdbarch) > + && gdbarch_software_single_step (gdbarch, get_current_frame ())) > + { > + step = 0; > + /* Do not pull these breakpoints until after a `wait' in > + `wait_for_inferior' */ > + singlestep_breakpoints_inserted_p = 1; > + singlestep_ptid = inferior_ptid; > + singlestep_pc = pc; > + } > + return step; > +} Because I'm dumb, while reading this, 'set_for_singlestep' and 'int step' didn't cross my bridge. How about renaming the local variable 'hw_step'? I don't like the "set_for_singlestep" name much. It isn't much self-describing, which is something very important in infrun.c, since it contains horrible, huge, full-of-states, hard to read code --- it is write-once, read-and-debug-millions-of-times code. But, I tried to come up with a descriptive name to suggest for it, and I didn't come up with something that made me happy, so, ... that's OK. > > /* Resume the inferior, but allow a QUIT. This is useful if the user > wants to interrupt some lengthy single-stepping operation > @@ -1031,20 +1053,9 @@ a command like `return' or `jump' to con > } > } > > - if (step && gdbarch_software_single_step_p (gdbarch)) > - { > - /* Do it the hard way, w/temp breakpoints */ > - if (gdbarch_software_single_step (gdbarch, get_current_frame ())) > - { > - /* ...and don't ask hardware to do it. */ > - step = 0; > - /* and do not pull these breakpoints until after a `wait' in > - `wait_for_inferior' */ > - singlestep_breakpoints_inserted_p = 1; > - singlestep_ptid = inferior_ptid; > - singlestep_pc = pc; > - } > - } > + /* Do we need to do it the hard way, w/temp breakpoints? */ > + if (step) > + step = set_for_singlestep (gdbarch, pc); ^ something looks strange with the indentation here. > > /* If there were any forks/vforks/execs that were caught and are > now to be followed, then do so. */ > @@ -2826,11 +2837,14 @@ targets should add new threads to the th > the inferior over it. If we have non-steppable watchpoints, > we must disable the current watchpoint; it's simplest to > disable all watchpoints and breakpoints. */ > - > + int step_over_watchpoint = 1; Similarly, rename this to hw_step. Even if this is false, we're still doing a a high-level step-over-watchpoint. > + > if (!HAVE_STEPPABLE_WATCHPOINT) > remove_breakpoints (); > registers_changed (); > - target_resume (ecs->ptid, 1, TARGET_SIGNAL_0); /* Single step */ > + /* Single step */ > + step_over_watchpoint = set_for_singlestep (current_gdbarch, read_pc ()); > + target_resume (ecs->ptid, step_over_watchpoint, TARGET_SIGNAL_0); > waiton_ptid = ecs->ptid; > if (HAVE_STEPPABLE_WATCHPOINT) > infwait_state = infwait_step_watch_state; OK with the above changes. -- Pedro Alves