From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25418 invoked by alias); 14 Apr 2009 01:23:36 -0000 Received: (qmail 25405 invoked by uid 22791); 14 Apr 2009 01:23:34 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from smtp2.caviumnetworks.com (HELO smtp2.caviumnetworks.com) (209.113.159.134) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 14 Apr 2009 01:23:26 +0000 Received: from exch4.caveonetworks.com (Not Verified[192.168.16.23]) by smtp2.caviumnetworks.com with MailMarshal (v6,2,2,3503) id ; Mon, 13 Apr 2009 21:23:18 -0400 Received: from exch4.caveonetworks.com ([192.168.16.23]) by exch4.caveonetworks.com with Microsoft SMTPSVC(6.0.3790.3959); Mon, 13 Apr 2009 18:16:52 -0700 Received: from dd1.caveonetworks.com ([64.169.86.201]) by exch4.caveonetworks.com with Microsoft SMTPSVC(6.0.3790.3959); Mon, 13 Apr 2009 18:16:52 -0700 Message-ID: <49E3E404.6@caviumnetworks.com> Date: Tue, 14 Apr 2009 01:23:00 -0000 From: David Daney User-Agent: Thunderbird 2.0.0.21 (X11/20090320) MIME-Version: 1.0 To: Pedro Alves , gdb-patches@sourceware.org Subject: Re: [Patch 1/2] infrun.c support for MIPS hardware watchpoints. References: <49D9A5E7.3000003@gmail.com> <200904111817.09949.pedro@codesourcery.com> In-Reply-To: <200904111817.09949.pedro@codesourcery.com> Content-Type: multipart/mixed; boundary="------------030403050802050803050708" 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/msg00245.txt.bz2 This is a multi-part message in MIME format. --------------030403050802050803050708 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-length: 2386 Pedro Alves wrote: > 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. > OK. >> +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'? OK > 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. > We went with: maybe_software_singlestep (). >> + /* 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. I was incorrect, now fixed. >> >> /* 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. > OK. > > OK with the above changes. > This is what I committed. --------------030403050802050803050708 Content-Type: text/plain; name="infrun.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="infrun.patch" Content-length: 2898 2009-04-13 David Daney * infrun.c (maybe_software_singlestep): New function. (resume): Call maybe_software_singlestep. (handle_inferior_event): Same. Index: infrun.c =================================================================== RCS file: /cvs/src/src/gdb/infrun.c,v retrieving revision 1.367 diff -u -p -r1.367 infrun.c --- infrun.c 25 Mar 2009 21:53:10 -0000 1.367 +++ infrun.c 14 Apr 2009 00:56:16 -0000 @@ -950,6 +950,29 @@ set_schedlock_func (char *args, int from } } +/* 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 +maybe_software_singlestep (struct gdbarch *gdbarch, CORE_ADDR pc) +{ + int hw_step = 1; + + if (gdbarch_software_single_step_p (gdbarch) + && gdbarch_software_single_step (gdbarch, get_current_frame ())) + { + hw_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 hw_step; +} /* Resume the inferior, but allow a QUIT. This is useful if the user wants to interrupt some lengthy single-stepping operation @@ -1031,20 +1054,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 = maybe_software_singlestep (gdbarch, pc); /* If there were any forks/vforks/execs that were caught and are now to be followed, then do so. */ @@ -2826,11 +2838,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 hw_step = 1; + if (!HAVE_STEPPABLE_WATCHPOINT) remove_breakpoints (); registers_changed (); - target_resume (ecs->ptid, 1, TARGET_SIGNAL_0); /* Single step */ + /* Single step */ + hw_step = maybe_software_singlestep (current_gdbarch, read_pc ()); + target_resume (ecs->ptid, hw_step, TARGET_SIGNAL_0); waiton_ptid = ecs->ptid; if (HAVE_STEPPABLE_WATCHPOINT) infwait_state = infwait_step_watch_state; --------------030403050802050803050708--