Re: [Patch 1/2] infrun.c support for MIPS hardware watchpoints.

[Pedro Alves <pedro at codesourcery dot 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