This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [Patch 1/2] infrun.c support for MIPS hardware watchpoints.
- From: Pedro Alves <pedro at codesourcery dot com>
- To: gdb-patches at sourceware dot org
- Cc: David Daney <david dot s dot daney at gmail dot com>, David Daney <ddaney at caviumnetworks dot com>
- Date: Sat, 11 Apr 2009 18:17:09 +0100
- Subject: Re: [Patch 1/2] infrun.c support for MIPS hardware watchpoints.
- References: <49D9A5E7.3000003@gmail.com>
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