[1/9] Decide whether we may have removed breakpoints based on step_over_info
Commit Message
... instead of trap_expected.
Gets rid of one singlestep_breakpoints_inserted_p reference, and is
generally more to the point.
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.
---
gdb/infrun.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
Comments
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?
> +/* 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?
> /* 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.
> target_pass_signals (0, NULL);
> else
> target_pass_signals ((int) GDB_SIGNAL_LAST, signal_pass);
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
Pedro Alves <palves@redhat.com> writes:
> 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?
Yes, it makes sense to me.
I've reviewed the rest of patches, and they are good to me. I've tested
the whole patch set with the changes I suggested in patch 3/9 on
arm-linux-gnueabi target. No regression.
On 10/06/2014 02:02 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
>
>> 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?
>
> Yes, it makes sense to me.
>
> I've reviewed the rest of patches, and they are good to me. I've tested
> the whole patch set with the changes I suggested in patch 3/9 on
> arm-linux-gnueabi target. No regression.
Excellent. Thank you very much, Yao.
Thanks,
Pedro Alves
@@ -1038,6 +1038,14 @@ stepping_past_instruction_at (struct address_space *aspace,
step_over_info.address));
}
+/* Returns true if step-over info is valid. */
+
+static int
+step_over_info_valid_p (void)
+{
+ return (step_over_info.aspace != NULL);
+}
+
/* Displaced stepping. */
@@ -1903,7 +1911,8 @@ a command like `return' or `jump' to continue execution."));
once we arrive back at the step-resume breakpoint, actually step
over the breakpoint we originally wanted to step over. */
if (singlestep_breakpoints_inserted_p
- && tp->control.trap_expected && sig != GDB_SIGNAL_0)
+ && sig != GDB_SIGNAL_0
+ && step_over_info_valid_p ())
{
/* If we have nested signals or a pending signal is delivered
immediately after a handler returns, might might already have
@@ -1997,13 +2006,10 @@ a command like `return' or `jump' to continue execution."));
tp->suspend.stop_signal = GDB_SIGNAL_0;
/* 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 ())
target_pass_signals (0, NULL);
else
target_pass_signals ((int) GDB_SIGNAL_LAST, signal_pass);