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.