[3/7] Force to insert software single step breakpoint
Commit Message
GDB doesn't insert software single step breakpoint if the instruction
branches to itself, so that the program can't stop after command "si".
(gdb) b 32
Breakpoint 2 at 0x8680: file git/gdb/testsuite/gdb.base/branch-to-self.c, line 32.
(gdb) c
Continuing.
Breakpoint 2, main () at gdb/git/gdb/testsuite/gdb.base/branch-to-self.c:32
32 asm (".Lhere: " BRANCH_INSN " .Lhere"); /* loop-line */
(gdb) si
infrun: clear_proceed_status_thread (Thread 3991.3991)
infrun: proceed (addr=0xffffffff, signal=GDB_SIGNAL_DEFAULT)
infrun: step-over queue now empty
infrun: resuming [Thread 3991.3991] for step-over
infrun: skipping breakpoint: stepping past insn at: 0x8680
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Sending packet: $Z0,8678,4#f3...Packet received: OK
infrun: skipping breakpoint: stepping past insn at: 0x8680
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Sending packet: $Z0,b6fe86c8,4#82...Packet received: OK
infrun: resume (step=1, signal=GDB_SIGNAL_0), trap_expected=1, current thread [Thread 3991.3991] at 0x868
breakpoint.c:should_be_inserted thinks the breakpoint shouldn't be
inserted, which is wrong. This patch restrict the condition that only
return false if breakpoint is NOT single step breakpoint.
gdb:
2016-03-23 Yao Qi <yao.qi@linaro.org>
* breakpoint.c (should_be_inserted): Don't return 0 if single
step breakpoint is inserted at the address we're stepping over.
* gdbarch.sh (software_single_step): Update comments.
* gdbarch.h: Regenerated.
---
gdb/breakpoint.c | 9 ++++++++-
gdb/gdbarch.h | 5 ++++-
gdb/gdbarch.sh | 5 ++++-
3 files changed, 16 insertions(+), 3 deletions(-)
Comments
On 03/23/2016 04:09 PM, Yao Qi wrote:
> GDB doesn't insert software single step breakpoint if the instruction
> branches to itself, so that the program can't stop after command "si".
>
> (gdb) b 32
> Breakpoint 2 at 0x8680: file git/gdb/testsuite/gdb.base/branch-to-self.c, line 32.
> (gdb) c
> Continuing.
>
> Breakpoint 2, main () at gdb/git/gdb/testsuite/gdb.base/branch-to-self.c:32
> 32 asm (".Lhere: " BRANCH_INSN " .Lhere"); /* loop-line */
> (gdb) si
> infrun: clear_proceed_status_thread (Thread 3991.3991)
> infrun: proceed (addr=0xffffffff, signal=GDB_SIGNAL_DEFAULT)
> infrun: step-over queue now empty
> infrun: resuming [Thread 3991.3991] for step-over
> infrun: skipping breakpoint: stepping past insn at: 0x8680
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Sending packet: $Z0,8678,4#f3...Packet received: OK
> infrun: skipping breakpoint: stepping past insn at: 0x8680
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Sending packet: $Z0,b6fe86c8,4#82...Packet received: OK
> infrun: resume (step=1, signal=GDB_SIGNAL_0), trap_expected=1, current thread [Thread 3991.3991] at 0x868
>
> breakpoint.c:should_be_inserted thinks the breakpoint shouldn't be
> inserted, which is wrong. This patch restrict the condition that only
> return false if breakpoint is NOT single step breakpoint.
>
> gdb:
>
> 2016-03-23 Yao Qi <yao.qi@linaro.org>
>
> * breakpoint.c (should_be_inserted): Don't return 0 if single
> step breakpoint is inserted at the address we're stepping over.
> * gdbarch.sh (software_single_step): Update comments.
> * gdbarch.h: Regenerated.
> ---
> gdb/breakpoint.c | 9 ++++++++-
> gdb/gdbarch.h | 5 ++++-
> gdb/gdbarch.sh | 5 ++++-
> 3 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index f99a7ab..9ecfb07 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -2219,9 +2219,16 @@ should_be_inserted (struct bp_location *bl)
> return 0;
>
> /* Don't insert a breakpoint if we're trying to step past its
> - location. */
> + location except single step breakpoint, because the single step
> + breakpoint may be inserted at the location we're trying to step
> + if the instruction branches to itself. However, the instruction
> + won't be executed at all and it may break the semantics of the
> + instruction, for example, the instruction is a conditional
> + branch or updates some flags. We can't fix it unless GDB is able
> + to emulate the instruction or switch to displaced stepping. */
> if ((bl->loc_type == bp_loc_software_breakpoint
> || bl->loc_type == bp_loc_hardware_breakpoint)
> + && bl->owner->type != bp_single_step
> && stepping_past_instruction_at (bl->pspace->aspace,
> bl->address))
Another scenario occurred to me:
- Thread A is software single-stepping.
- Thread B hits single-step breakpoint of thread A.
- We pause all threads and set thread B stepping past the
single-step breakpoint of thread A.
But if the single-step breakpoint is for another thread, then
we won't actually manage to have thread B step past it, resulting
in spurious re-hits and no-guaranteed forward progress. See
e.g., non-stop-fair-events.exp:
# On software single-step targets that don't support displaced
# stepping, threads keep hitting each others' single-step
# breakpoints, and then GDB needs to pause all threads to step
# past those. The end result is that progress in the main
# thread will be slower and it may take a bit longer for the
# signal to be queued; bump the timeout.
Sounds like we may need to look at the single-step breakpoint's thread
id, and only insert it if it is for the thread that is going to be
doing the step-over? We may need to record that in step_over_info and
pass more info to stepping_past_instruction_at.
> --- a/gdb/gdbarch.sh
> +++ b/gdb/gdbarch.sh
> @@ -609,7 +609,10 @@ m:CORE_ADDR:addr_bits_remove:CORE_ADDR addr:addr::core_addr_identity::0
> # target can single step. If not, then implement single step using breakpoints.
> #
> # A return value of 1 means that the software_single_step breakpoints
> -# were inserted; 0 means they were not.
> +# were inserted; 0 means they were not. Multiple breakpoints may be
> +# inserted for some instructions such as conditional branch. However,
> +# each implementation must always evaluate the condition and only put
> +# the breakpoint at the branch destination if the condition is true.
I'd add:
(...) condition is true, so that we ensure forward progress when
stepping past a conditional branch to self.
This will help porters evaluate whether that's really necessary
for their ports.
Thanks,
Pedro Alves
Pedro Alves <palves@redhat.com> writes:
> Another scenario occurred to me:
>
> - Thread A is software single-stepping.
> - Thread B hits single-step breakpoint of thread A.
> - We pause all threads and set thread B stepping past the
> single-step breakpoint of thread A.
>
> But if the single-step breakpoint is for another thread, then
> we won't actually manage to have thread B step past it, resulting
> in spurious re-hits and no-guaranteed forward progress. See
> e.g., non-stop-fair-events.exp:
>
> # On software single-step targets that don't support displaced
> # stepping, threads keep hitting each others' single-step
> # breakpoints, and then GDB needs to pause all threads to step
> # past those. The end result is that progress in the main
> # thread will be slower and it may take a bit longer for the
> # signal to be queued; bump the timeout.
I finally managed to reproduce that thread id in step_over_info is
different from the thread id of the single-step breakpoint.
GDB now gives the high priority to finishing step over, to avoid
"threads keep hitting each others' single-step breakpoint". With my
patch applied, single-step breakpoint (of threads other than we are
stepping over) is still inserted even we try to step past the location,
so the step-over can't be finished.
>
> Sounds like we may need to look at the single-step breakpoint's thread
> id, and only insert it if it is for the thread that is going to be
> doing the step-over? We may need to record that in step_over_info and
> pass more info to stepping_past_instruction_at.
Yes, after I added a new field 'struct thread_info *thread' in
'struct step_over_info', I realize that IWBN to convert 'step_over_info'
to 'the thread we are stepping over', so the fields 'aspace' and 'address'
can be replaced by 'thread', like this,
struct step_over_info
{
struct thread_info *thread;
int nonsteppable_watchpoint_p;
};
Is it a good idea? If there is nothing obviously wrong, I'll post
patches to do this.
Pedro Alves <palves@redhat.com> writes:
> Another scenario occurred to me:
>
> - Thread A is software single-stepping.
> - Thread B hits single-step breakpoint of thread A.
> - We pause all threads and set thread B stepping past the
> single-step breakpoint of thread A.
>
> But if the single-step breakpoint is for another thread, then
> we won't actually manage to have thread B step past it, resulting
> in spurious re-hits and no-guaranteed forward progress. See
> e.g., non-stop-fair-events.exp:
>
> # On software single-step targets that don't support displaced
> # stepping, threads keep hitting each others' single-step
> # breakpoints, and then GDB needs to pause all threads to step
> # past those. The end result is that progress in the main
> # thread will be slower and it may take a bit longer for the
> # signal to be queued; bump the timeout.
>
> Sounds like we may need to look at the single-step breakpoint's thread
> id, and only insert it if it is for the thread that is going to be
> doing the step-over? We may need to record that in step_over_info and
> pass more info to stepping_past_instruction_at.
I think this is about any thread specific breakpoint, instead of
only single-step breakpoint (single-step breakpoint is thread specific
too). If we are doing step-over for thread A, do we need to insert any
breakpoints specific to other threads? (my answer is No).
On 04/19/2016 03:54 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
>
>> Another scenario occurred to me:
>>
>> - Thread A is software single-stepping.
>> - Thread B hits single-step breakpoint of thread A.
>> - We pause all threads and set thread B stepping past the
>> single-step breakpoint of thread A.
>>
>> But if the single-step breakpoint is for another thread, then
>> we won't actually manage to have thread B step past it, resulting
>> in spurious re-hits and no-guaranteed forward progress. See
>> e.g., non-stop-fair-events.exp:
>>
>> # On software single-step targets that don't support displaced
>> # stepping, threads keep hitting each others' single-step
>> # breakpoints, and then GDB needs to pause all threads to step
>> # past those. The end result is that progress in the main
>> # thread will be slower and it may take a bit longer for the
>> # signal to be queued; bump the timeout.
>>
>> Sounds like we may need to look at the single-step breakpoint's thread
>> id, and only insert it if it is for the thread that is going to be
>> doing the step-over? We may need to record that in step_over_info and
>> pass more info to stepping_past_instruction_at.
>
> I think this is about any thread specific breakpoint, instead of
> only single-step breakpoint (single-step breakpoint is thread specific
> too). If we are doing step-over for thread A, do we need to insert any
> breakpoints specific to other threads? (my answer is No).
Right, we don't need to insert them, because other threads
will remain stopped while thread A is doing the step-over.
However, given that gdb does not remove/re-insert all breakpoints on
internal stops nowadays, removing thread-specific breakpoints of others
threads will be less efficient than leaving them be, I think. I mean,
you'll get more z0/Z0 traffic than if you leave them inserted.
Thanks,
Pedro Alves
@@ -2219,9 +2219,16 @@ should_be_inserted (struct bp_location *bl)
return 0;
/* Don't insert a breakpoint if we're trying to step past its
- location. */
+ location except single step breakpoint, because the single step
+ breakpoint may be inserted at the location we're trying to step
+ if the instruction branches to itself. However, the instruction
+ won't be executed at all and it may break the semantics of the
+ instruction, for example, the instruction is a conditional
+ branch or updates some flags. We can't fix it unless GDB is able
+ to emulate the instruction or switch to displaced stepping. */
if ((bl->loc_type == bp_loc_software_breakpoint
|| bl->loc_type == bp_loc_hardware_breakpoint)
+ && bl->owner->type != bp_single_step
&& stepping_past_instruction_at (bl->pspace->aspace,
bl->address))
{
@@ -650,7 +650,10 @@ extern void set_gdbarch_addr_bits_remove (struct gdbarch *gdbarch, gdbarch_addr_
target can single step. If not, then implement single step using breakpoints.
A return value of 1 means that the software_single_step breakpoints
- were inserted; 0 means they were not. */
+ were inserted; 0 means they were not. Multiple breakpoints may be
+ inserted for some instructions such as conditional branch. However,
+ each implementation must always evaluate the condition and only put
+ the breakpoint at the branch destination if the condition is true. */
extern int gdbarch_software_single_step_p (struct gdbarch *gdbarch);
@@ -609,7 +609,10 @@ m:CORE_ADDR:addr_bits_remove:CORE_ADDR addr:addr::core_addr_identity::0
# target can single step. If not, then implement single step using breakpoints.
#
# A return value of 1 means that the software_single_step breakpoints
-# were inserted; 0 means they were not.
+# were inserted; 0 means they were not. Multiple breakpoints may be
+# inserted for some instructions such as conditional branch. However,
+# each implementation must always evaluate the condition and only put
+# the breakpoint at the branch destination if the condition is true.
F:int:software_single_step:struct frame_info *frame:frame
# Return non-zero if the processor is executing a delay slot and a