Re: [PATCH 1/9] Decide whether we may have removed breakpoints based on step_over_info

[Pedro Alves <palves@redhat.com>]

On 09/28/2014 01:48 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> gdb/
>> 2014-09-22  Pedro Alves  <palves@redhat.com>
>>
>> 	* infrun.c (step_over_info_valid_p): New function.
>> 	(resume): Use step_over_info_valid_p instead of checking the
>> 	threads's trap_expected flag.  Add debug output.
>                                        ^^^^^^^^^^^^^^^^
> I don't see any debug output added by the code.  Maybe a staled changelog entry?

Yeah, I ended up removing that code.  Will fix, thanks.

> 
>> +/* Returns true if step-over info is valid.  */
>> +
>> +static int
>> +step_over_info_valid_p (void)
>> +{
>> +  return (step_over_info.aspace != NULL);
>> +}
>> +
> 
> How about replace "step_over_info.aspace != NULL" in
> stepping_past_instruction_at with step_over_info_valid_p too?

See patch 2/9.  With that, step_over_info_valid_p will return
true if we're stepping over a watchpoint, but aspace will
be NULL.

> 
>>    /* Advise target which signals may be handled silently.  If we have
>> -     removed breakpoints because we are stepping over one (which can
>> -     happen only if we are not using displaced stepping), we need to
>> +     removed breakpoints because we are stepping over one, we need to
>>       receive all signals to avoid accidentally skipping a breakpoint
>>       during execution of a signal handler.  */
>> -  if ((step || singlestep_breakpoints_inserted_p)
>> -      && tp->control.trap_expected
>> -      && !use_displaced_stepping (gdbarch))
>> +  if (step_over_info_valid_p ())
> 
> Why do we remove condition (step || singlestep_breakpoints_inserted_p)?
> I understand that step_over_info_valid_p is equivalent to
> "tp->control.trap_expected && !use_displaced_stepping (gdbarch)", so I
> don't know why (step || singlestep_breakpoints_inserted_p) is removed
> too.

It ends up unnecessary, and it seems to me to be more to the
point.

I.e., if we have step-over info, then something, somewhere wants a
breakpoint lifted out of the target.  No matter whether we're
stepping or continuing the target at this point, we need to receive
all signals so that if the signal handler calls the code that
would trigger the breakpoint/watchpoint, we don't miss it.

Removing this check now avoids having tweak it when
singlestep_breakpoints_inserted_p check global ends up
eliminated by a later patch in the series.

Does that make sense?

> 
>>      target_pass_signals (0, NULL);
>>    else
>>      target_pass_signals ((int) GDB_SIGNAL_LAST, signal_pass);

Thanks,
Pedro Alves