* Re: [RFC] Add support of software single step to process record
[not found] ` <200912241738.19780.pedro@codesourcery.com>
@ 2010-01-04 14:23 ` Hui Zhu
2010-01-08 16:24 ` Pedro Alves
0 siblings, 1 reply; 8+ messages in thread
From: Hui Zhu @ 2010-01-04 14:23 UTC (permalink / raw)
To: Pedro Alves, shuchang zhou
Cc: gdb-patches, Joel Brobecker, Michael Snyder, paawan oza, Tom Tromey
[-- Attachment #1: Type: text/plain, Size: 4587 bytes --]
Sorry guys, the prev patch is so ugly.
Thanks for teach me clear about the gdbarch_software_single_step, Pedro.
I did some extend with your idea. Because record_wait need
record_resume_step point out this resume is signal step or continue.
if (!step)
{
/* This is not hard single step. */
if (!gdbarch_software_single_step_p (gdbarch))
{
/* This is a normal continue. */
step = 1;
}
else
{
/* This arch support soft sigle step. */
if (single_step_breakpoints_inserted ())
{
/* This is a soft single step. */
record_resume_step = 1;
}
else
{
/* This is a continue.
Try to insert a soft single step breakpoint. */
if (!gdbarch_software_single_step (gdbarch,
get_current_frame ()))
{
/* This system don't want use soft single step.
Use hard sigle step. */
step = 1;
}
}
}
}
Shuchuang, please help me try this patch. Thanks.
Best regards,
Hui
2010-01-04 Hui Zhu <teawater@gmail.com>
* breakpoint.c (single_step_breakpoints_inserted): New
function.
* breakpoint.h (single_step_breakpoints_inserted): Extern.
* record.c (record_resume): Add code for software single step.
(record_wait): Ditto.
On Fri, Dec 25, 2009 at 01:38, Pedro Alves <pedro@codesourcery.com> wrote:
> On Wednesday 23 December 2009 09:23:21, Hui Zhu wrote:
>> + struct gdbarch *gdbarch = target_thread_architecture (ptid);
>> +
>> record_message (get_current_regcache (), signal);
>
>> record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
>> signal);
>
> Why is this resume call still present?
>
>> +
>> + if (gdbarch_software_single_step_p (gdbarch))
>> + {
>> + if (!inserted_single_step_breakpoint_p ())
>
> Isn't this naming stale? I thought you had renamed this.
>
>> + gdbarch_software_single_step (gdbarch, get_current_frame ());
>> + record_beneath_to_resume (record_beneath_to_resume_ops,
>> + ptid, step, signal);
>> + record_resume_step = 0;
>> + }
>> + else
>> + record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
>> + signal);
>> }
>>
>
> You've got the predicates a bit mixed up.
>
> - gdbarch_software_single_step_p purpose is only "is there or
> not a gdbarch_software_single_step callback registered in
> this gdbarch"? It returning true does not mean that
> software single-step should be used for that single-step.
>
> - gdbarch_software_single_step can return false, meaning,
> no software single-step needs to be used.
>
> This is how stepping over atomic sequences is handled
> currently (grep for deal_with_atomic_sequence):
> gdbarch_software_single_step_p returns true, but
> gdbarch_software_single_step returns false most
> of the times. See also infrun.c:maybe_software_singlestep.
>
> I think you want this:
>
> if (!step
> && gdbarch_software_single_step_p (gdbarch)
> && !single_step_breakpoints_inserted ()
> && gdbarch_software_single_step (gdbarch, get_current_frame ()))
> record_resume_step = 0;
> else
> record_resume_step = 1;
>
> record_beneath_to_resume (record_beneath_to_resume_ops, ptid,
> record_resume_step, signal);
>
> If `step' is true when record_resume is called, and so is
> gdbarch_software_single_step_p, then it must be that infrun.c
> already determined that gdbarch_software_single_step returns
> false, otherwise, `step' would be false (maybe_software_singlestep).
>
> If `step' is false (the user is requesting a continue), and
> no single-step breakpoints are inserted yet, but,
> gdbarch_software_single_step returns false, we have ourselves
> an arch/target combo that only wants software single-stepping
> for atomic sequences, e.g., MIPS (non-linux), or PPC. If
> so, we should force hardware single-step in the target
> beneath (set record_resume_step to 1).
>
> --
> Pedro Alves
>
[-- Attachment #2: prec_software_single_step.txt --]
[-- Type: text/plain, Size: 4105 bytes --]
---
breakpoint.c | 10 ++++++++++
breakpoint.h | 1 +
record.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++---
3 files changed, 59 insertions(+), 3 deletions(-)
--- a/breakpoint.c
+++ b/breakpoint.c
@@ -9646,6 +9646,16 @@ insert_single_step_breakpoint (struct gd
paddress (gdbarch, next_pc));
}
+/* Check if the breakpoints used for software single stepping
+ were inserted or not. */
+
+int
+single_step_breakpoints_inserted (void)
+{
+ return (single_step_breakpoints[0] != NULL
+ || single_step_breakpoints[1] != NULL);
+}
+
/* Remove and delete any breakpoints used for software single step. */
void
--- a/breakpoint.h
+++ b/breakpoint.h
@@ -944,6 +944,7 @@ extern int remove_hw_watchpoints (void);
twice before remove is called. */
extern void insert_single_step_breakpoint (struct gdbarch *,
struct address_space *, CORE_ADDR);
+extern int single_step_breakpoints_inserted (void);
extern void remove_single_step_breakpoints (void);
/* Manage manual breakpoints, separate from the normal chain of
--- a/record.c
+++ b/record.c
@@ -1002,9 +1002,43 @@ record_resume (struct target_ops *ops, p
if (!RECORD_IS_REPLAY)
{
+ struct gdbarch *gdbarch = target_thread_architecture (ptid);
+
record_message (get_current_regcache (), signal);
- record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
- signal);
+
+ if (!step)
+ {
+ /* This is not hard single step. */
+ if (!gdbarch_software_single_step_p (gdbarch))
+ {
+ /* This is a normal continue. */
+ step = 1;
+ }
+ else
+ {
+ /* This arch support soft sigle step. */
+ if (single_step_breakpoints_inserted ())
+ {
+ /* This is a soft single step. */
+ record_resume_step = 1;
+ }
+ else
+ {
+ /* This is a continue.
+ Try to insert a soft single step breakpoint. */
+ if (!gdbarch_software_single_step (gdbarch,
+ get_current_frame ()))
+ {
+ /* This system don't want use soft single step.
+ Use hard sigle step. */
+ step = 1;
+ }
+ }
+ }
+ }
+
+ record_beneath_to_resume (record_beneath_to_resume_ops,
+ ptid, step, signal);
}
}
@@ -1077,6 +1111,7 @@ record_wait (struct target_ops *ops,
/* This is not a single step. */
ptid_t ret;
CORE_ADDR tmp_pc;
+ struct gdbarch *gdbarch = target_thread_architecture (inferior_ptid);
while (1)
{
@@ -1099,6 +1134,9 @@ record_wait (struct target_ops *ops,
tmp_pc = regcache_read_pc (regcache);
aspace = get_regcache_aspace (regcache);
+ if (gdbarch_software_single_step_p (gdbarch))
+ remove_single_step_breakpoints ();
+
if (target_stopped_by_watchpoint ())
{
/* Always interested in watchpoints. */
@@ -1121,6 +1159,8 @@ record_wait (struct target_ops *ops,
{
/* This must be a single-step trap. Record the
insn and issue another step. */
+ int step = 1;
+
if (!record_message_wrapper_safe (regcache,
TARGET_SIGNAL_0))
{
@@ -1129,8 +1169,13 @@ record_wait (struct target_ops *ops,
break;
}
+ if (gdbarch_software_single_step_p (gdbarch)
+ && gdbarch_software_single_step (gdbarch,
+ get_current_frame ()))
+ step = 0;
+
record_beneath_to_resume (record_beneath_to_resume_ops,
- ptid, 1,
+ ptid, step,
TARGET_SIGNAL_0);
continue;
}
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] Add support of software single step to process record
2010-01-04 14:23 ` [RFC] Add support of software single step to process record Hui Zhu
@ 2010-01-08 16:24 ` Pedro Alves
2010-05-25 5:14 ` Hui Zhu
0 siblings, 1 reply; 8+ messages in thread
From: Pedro Alves @ 2010-01-08 16:24 UTC (permalink / raw)
To: Hui Zhu
Cc: shuchang zhou, gdb-patches, Joel Brobecker, Michael Snyder,
paawan oza, Tom Tromey
On Monday 04 January 2010 14:23:21, Hui Zhu wrote:
> Sorry guys, the prev patch is so ugly.
:-)
> Thanks for teach me clear about the gdbarch_software_single_step, Pedro.
> I did some extend with your idea. Because record_wait need
> record_resume_step point out this resume is signal step or continue.
>
> if (!step)
> {
> /* This is not hard single step. */
> if (!gdbarch_software_single_step_p (gdbarch))
> {
> /* This is a normal continue. */
> step = 1;
> }
> else
> {
> /* This arch support soft sigle step. */
> if (single_step_breakpoints_inserted ())
> {
> /* This is a soft single step. */
> record_resume_step = 1;
> }
> else
> {
> /* This is a continue.
> Try to insert a soft single step breakpoint. */
> if (!gdbarch_software_single_step (gdbarch,
> get_current_frame ()))
> {
> /* This system don't want use soft single step.
> Use hard sigle step. */
> step = 1;
> }
> }
> }
> }
Cool, this looks pretty clear to me now. Thanks.
> @@ -1077,6 +1111,7 @@ record_wait (struct target_ops *ops,
> /* This is not a single step. */
> ptid_t ret;
> CORE_ADDR tmp_pc;
> + struct gdbarch *gdbarch = target_thread_architecture (inferior_ptid);
>
> while (1)
> {
> @@ -1099,6 +1134,9 @@ record_wait (struct target_ops *ops,
> tmp_pc = regcache_read_pc (regcache);
> aspace = get_regcache_aspace (regcache);
>
> + if (gdbarch_software_single_step_p (gdbarch))
> + remove_single_step_breakpoints ();
This will gdb_assert inside remove_single_step_breakpoints
if SSS bkpts are not inserted, but gdbarch_software_single_step_p
returns true. This instead is safer:
if (single_step_breakpoints_inserted ())
remove_single_step_breakpoints ();
But, what if it was infrun that had inserted the single-step
breakpoints, for a "next" or "step", etc.? Shouldn't you check
for record_resume_step too?
if (!record_resume_step && single_step_breakpoints_inserted ())
remove_single_step_breakpoints ();
Otherwise, the check below for
else if (breakpoint_inserted_here_p (aspace, tmp_pc))
{
/* There is a breakpoint here. Let the core
handle it. */
if (software_breakpoint_inserted_here_p (aspace, tmp_pc))
{
would fail, and the finished single-step wouldn't be reported to the
core, right?
Lastly, you may also want to confirm that the SSS bkpt managed by record.d itself explains the SIGTRAP before removing before issueing another
single-step. If any unexplainable SIGTRAP happens for any reason while
single-stepping, you should report it to infrun instead. In other words:
With software single-stepping, we can distinguish most random
SIGTRAPs from SSS SIGTRAPs, so:
/* This must be a single-step trap. Record the
insn and issue another step. */
... the "must" here ends up being a bit too strong. I'd certainly
understand ignoring this for simplicity or performance reasons though.
Otherwise, looks good to me.
--
Pedro Alves
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] Add support of software single step to process record
2010-01-08 16:24 ` Pedro Alves
@ 2010-05-25 5:14 ` Hui Zhu
2010-05-27 6:51 ` Hui Zhu
0 siblings, 1 reply; 8+ messages in thread
From: Hui Zhu @ 2010-05-25 5:14 UTC (permalink / raw)
To: Pedro Alves, ping huang, shuchang zhou
Cc: gdb-patches, Joel Brobecker, Michael Snyder, paawan oza, Tom Tromey
[-- Attachment #1: Type: text/plain, Size: 5109 bytes --]
Thanks Pedro.
On Sat, Jan 9, 2010 at 00:24, Pedro Alves <pedro@codesourcery.com> wrote:
> On Monday 04 January 2010 14:23:21, Hui Zhu wrote:
>> Sorry guys, the prev patch is so ugly.
>
> :-)
>
>> Thanks for teach me clear about the gdbarch_software_single_step, Pedro.
>> I did some extend with your idea. Because record_wait need
>> record_resume_step point out this resume is signal step or continue.
>>
>> if (!step)
>> {
>> /* This is not hard single step. */
>> if (!gdbarch_software_single_step_p (gdbarch))
>> {
>> /* This is a normal continue. */
>> step = 1;
>> }
>> else
>> {
>> /* This arch support soft sigle step. */
>> if (single_step_breakpoints_inserted ())
>> {
>> /* This is a soft single step. */
>> record_resume_step = 1;
>> }
>> else
>> {
>> /* This is a continue.
>> Try to insert a soft single step breakpoint. */
>> if (!gdbarch_software_single_step (gdbarch,
>> get_current_frame ()))
>> {
>> /* This system don't want use soft single step.
>> Use hard sigle step. */
>> step = 1;
>> }
>> }
>> }
>> }
>
> Cool, this looks pretty clear to me now. Thanks.
>
>
>
>> @@ -1077,6 +1111,7 @@ record_wait (struct target_ops *ops,
>> /* This is not a single step. */
>> ptid_t ret;
>> CORE_ADDR tmp_pc;
>> + struct gdbarch *gdbarch = target_thread_architecture (inferior_ptid);
>>
>> while (1)
>> {
>> @@ -1099,6 +1134,9 @@ record_wait (struct target_ops *ops,
>> tmp_pc = regcache_read_pc (regcache);
>> aspace = get_regcache_aspace (regcache);
>>
>> + if (gdbarch_software_single_step_p (gdbarch))
>> + remove_single_step_breakpoints ();
>
> This will gdb_assert inside remove_single_step_breakpoints
> if SSS bkpts are not inserted, but gdbarch_software_single_step_p
> returns true. This instead is safer:
>
> if (single_step_breakpoints_inserted ())
> remove_single_step_breakpoints ();
OK. I will fix it.
>
> But, what if it was infrun that had inserted the single-step
> breakpoints, for a "next" or "step", etc.? Shouldn't you check
> for record_resume_step too?
>
> if (!record_resume_step && single_step_breakpoints_inserted ())
> remove_single_step_breakpoints ();
>
> Otherwise, the check below for
>
> else if (breakpoint_inserted_here_p (aspace, tmp_pc))
> {
> /* There is a breakpoint here. Let the core
> handle it. */
> if (software_breakpoint_inserted_here_p (aspace, tmp_pc))
> {
>
> would fail, and the finished single-step wouldn't be reported to the
> core, right?
I think this single step will be handle by line:
if (record_resume_step)
{
/* This is a single step. */
return record_beneath_to_wait (record_beneath_to_wait_ops,
ptid, status, options);
}
>
>
> Lastly, you may also want to confirm that the SSS bkpt managed by record.d itself explains the SIGTRAP before removing before issueing another
> single-step. If any unexplainable SIGTRAP happens for any reason while
> single-stepping, you should report it to infrun instead. In other words:
>
> With software single-stepping, we can distinguish most random
> SIGTRAPs from SSS SIGTRAPs, so:
>
> /* This must be a single-step trap. Record the
> insn and issue another step. */
>
> ... the "must" here ends up being a bit too strong. I'd certainly
> understand ignoring this for simplicity or performance reasons though.
Ah. Looks we didn't have good way to handle it. I change this comment to:
/* This is a single-step trap. Record the
insn and issue another step.
FIXME: this part can be a random SIGTRAP too.
But GDB cannot handle it. */
Shuchang, could you try your code just use command si and
reverse-xxx. If that part OK. Please help me try this patch.
Ping, please help me test this patch. And about hellogcc, you can find us in:
https://groups.google.com/group/hellogcc
https://webchat.freenode.net/ #hellogcc
Thanks,
Hui
2010-05-25 Hui Zhu <teawater@gmail.com>
* breakpoint.c (single_step_breakpoints_inserted): New
function.
* breakpoint.h (single_step_breakpoints_inserted): Extern.
* record.c (record_resume): Add code for software single step.
(record_wait): Ditto.
[-- Attachment #2: prec_software_single_step.txt --]
[-- Type: text/plain, Size: 4349 bytes --]
---
breakpoint.c | 10 ++++++++++
breakpoint.h | 1 +
record.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
3 files changed, 63 insertions(+), 5 deletions(-)
--- a/breakpoint.c
+++ b/breakpoint.c
@@ -10293,6 +10293,16 @@ insert_single_step_breakpoint (struct gd
paddress (gdbarch, next_pc));
}
+/* Check if the breakpoints used for software single stepping
+ were inserted or not. */
+
+int
+single_step_breakpoints_inserted (void)
+{
+ return (single_step_breakpoints[0] != NULL
+ || single_step_breakpoints[1] != NULL);
+}
+
/* Remove and delete any breakpoints used for software single step. */
void
--- a/breakpoint.h
+++ b/breakpoint.h
@@ -983,6 +983,7 @@ extern int remove_hw_watchpoints (void);
twice before remove is called. */
extern void insert_single_step_breakpoint (struct gdbarch *,
struct address_space *, CORE_ADDR);
+extern int single_step_breakpoints_inserted (void);
extern void remove_single_step_breakpoints (void);
/* Manage manual breakpoints, separate from the normal chain of
--- a/record.c
+++ b/record.c
@@ -1007,9 +1007,43 @@ record_resume (struct target_ops *ops, p
if (!RECORD_IS_REPLAY)
{
+ struct gdbarch *gdbarch = target_thread_architecture (ptid);
+
record_message (get_current_regcache (), signal);
- record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
- signal);
+
+ if (!step)
+ {
+ /* This is not hard single step. */
+ if (!gdbarch_software_single_step_p (gdbarch))
+ {
+ /* This is a normal continue. */
+ step = 1;
+ }
+ else
+ {
+ /* This arch support soft sigle step. */
+ if (single_step_breakpoints_inserted ())
+ {
+ /* This is a soft single step. */
+ record_resume_step = 1;
+ }
+ else
+ {
+ /* This is a continue.
+ Try to insert a soft single step breakpoint. */
+ if (!gdbarch_software_single_step (gdbarch,
+ get_current_frame ()))
+ {
+ /* This system don't want use soft single step.
+ Use hard sigle step. */
+ step = 1;
+ }
+ }
+ }
+ }
+
+ record_beneath_to_resume (record_beneath_to_resume_ops,
+ ptid, step, signal);
}
}
@@ -1082,6 +1116,7 @@ record_wait (struct target_ops *ops,
/* This is not a single step. */
ptid_t ret;
CORE_ADDR tmp_pc;
+ struct gdbarch *gdbarch = target_thread_architecture (inferior_ptid);
while (1)
{
@@ -1104,6 +1139,9 @@ record_wait (struct target_ops *ops,
tmp_pc = regcache_read_pc (regcache);
aspace = get_regcache_aspace (regcache);
+ if (single_step_breakpoints_inserted ())
+ remove_single_step_breakpoints ();
+
if (target_stopped_by_watchpoint ())
{
/* Always interested in watchpoints. */
@@ -1124,8 +1162,12 @@ record_wait (struct target_ops *ops,
}
else
{
- /* This must be a single-step trap. Record the
- insn and issue another step. */
+ /* This is a single-step trap. Record the
+ insn and issue another step.
+ FIXME: this part can be a random SIGTRAP too.
+ But GDB cannot handle it. */
+ int step = 1;
+
if (!record_message_wrapper_safe (regcache,
TARGET_SIGNAL_0))
{
@@ -1134,8 +1176,13 @@ record_wait (struct target_ops *ops,
break;
}
+ if (gdbarch_software_single_step_p (gdbarch)
+ && gdbarch_software_single_step (gdbarch,
+ get_current_frame ()))
+ step = 0;
+
record_beneath_to_resume (record_beneath_to_resume_ops,
- ptid, 1,
+ ptid, step,
TARGET_SIGNAL_0);
continue;
}
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] Add support of software single step to process record
2010-05-25 5:14 ` Hui Zhu
@ 2010-05-27 6:51 ` Hui Zhu
2010-06-11 13:55 ` Pedro Alves
0 siblings, 1 reply; 8+ messages in thread
From: Hui Zhu @ 2010-05-27 6:51 UTC (permalink / raw)
To: Pedro Alves, ping huang, shuchang zhou
Cc: gdb-patches, Joel Brobecker, Michael Snyder, paawan oza, Tom Tromey
[-- Attachment #1: Type: text/plain, Size: 7361 bytes --]
On Tue, May 25, 2010 at 11:04, Hui Zhu <teawater@gmail.com> wrote:
> Thanks Pedro.
>
> On Sat, Jan 9, 2010 at 00:24, Pedro Alves <pedro@codesourcery.com> wrote:
>> On Monday 04 January 2010 14:23:21, Hui Zhu wrote:
>>> Sorry guys, the prev patch is so ugly.
>>
>> :-)
>>
>>> Thanks for teach me clear about the gdbarch_software_single_step, Pedro.
>>> I did some extend with your idea. Because record_wait need
>>> record_resume_step point out this resume is signal step or continue.
>>>
>>> if (!step)
>>> {
>>> /* This is not hard single step. */
>>> if (!gdbarch_software_single_step_p (gdbarch))
>>> {
>>> /* This is a normal continue. */
>>> step = 1;
>>> }
>>> else
>>> {
>>> /* This arch support soft sigle step. */
>>> if (single_step_breakpoints_inserted ())
>>> {
>>> /* This is a soft single step. */
>>> record_resume_step = 1;
>>> }
>>> else
>>> {
>>> /* This is a continue.
>>> Try to insert a soft single step breakpoint. */
>>> if (!gdbarch_software_single_step (gdbarch,
>>> get_current_frame ()))
>>> {
>>> /* This system don't want use soft single step.
>>> Use hard sigle step. */
>>> step = 1;
>>> }
>>> }
>>> }
>>> }
>>
>> Cool, this looks pretty clear to me now. Thanks.
>>
>>
>>
>>> @@ -1077,6 +1111,7 @@ record_wait (struct target_ops *ops,
>>> /* This is not a single step. */
>>> ptid_t ret;
>>> CORE_ADDR tmp_pc;
>>> + struct gdbarch *gdbarch = target_thread_architecture (inferior_ptid);
>>>
>>> while (1)
>>> {
>>> @@ -1099,6 +1134,9 @@ record_wait (struct target_ops *ops,
>>> tmp_pc = regcache_read_pc (regcache);
>>> aspace = get_regcache_aspace (regcache);
>>>
>>> + if (gdbarch_software_single_step_p (gdbarch))
>>> + remove_single_step_breakpoints ();
>>
>> This will gdb_assert inside remove_single_step_breakpoints
>> if SSS bkpts are not inserted, but gdbarch_software_single_step_p
>> returns true. This instead is safer:
>>
>> if (single_step_breakpoints_inserted ())
>> remove_single_step_breakpoints ();
>
> OK. I will fix it.
>
>>
>> But, what if it was infrun that had inserted the single-step
>> breakpoints, for a "next" or "step", etc.? Shouldn't you check
>> for record_resume_step too?
>>
>> if (!record_resume_step && single_step_breakpoints_inserted ())
>> remove_single_step_breakpoints ();
>>
>> Otherwise, the check below for
>>
>> else if (breakpoint_inserted_here_p (aspace, tmp_pc))
>> {
>> /* There is a breakpoint here. Let the core
>> handle it. */
>> if (software_breakpoint_inserted_here_p (aspace, tmp_pc))
>> {
>>
>> would fail, and the finished single-step wouldn't be reported to the
>> core, right?
>
> I think this single step will be handle by line:
> if (record_resume_step)
> {
> /* This is a single step. */
> return record_beneath_to_wait (record_beneath_to_wait_ops,
> ptid, status, options);
> }
>
>>
>>
>> Lastly, you may also want to confirm that the SSS bkpt managed by record.d itself explains the SIGTRAP before removing before issueing another
>> single-step. If any unexplainable SIGTRAP happens for any reason while
>> single-stepping, you should report it to infrun instead. In other words:
>>
>> With software single-stepping, we can distinguish most random
>> SIGTRAPs from SSS SIGTRAPs, so:
>>
>> /* This must be a single-step trap. Record the
>> insn and issue another step. */
>>
>> ... the "must" here ends up being a bit too strong. I'd certainly
>> understand ignoring this for simplicity or performance reasons though.
>
> Ah. Looks we didn't have good way to handle it. I change this comment to:
> /* This is a single-step trap. Record the
> insn and issue another step.
> FIXME: this part can be a random SIGTRAP too.
> But GDB cannot handle it. */
>
>
> Shuchang, could you try your code just use command si and
> reverse-xxx. If that part OK. Please help me try this patch.
>
> Ping, please help me test this patch. And about hellogcc, you can find us in:
> https://groups.google.com/group/hellogcc
> https://webchat.freenode.net/ #hellogcc
>
> Thanks,
> Hui
>
> 2010-05-25 Hui Zhu <teawater@gmail.com>
>
> * breakpoint.c (single_step_breakpoints_inserted): New
> function.
> * breakpoint.h (single_step_breakpoints_inserted): Extern.
> * record.c (record_resume): Add code for software single step.
> (record_wait): Ditto.
>
Hello,
After do some test with Ping, I found some trouble and fixed them.
1. Add following:
@@ -1134,8 +1176,20 @@ record_wait (struct target_ops *ops,
break;
}
+ if (gdbarch_software_single_step_p (gdbarch))
+ {
+ /* Try to insert the software single step breakpoint.
+ If insert success, set step to 0. */
+ set_executing (inferior_ptid, 0);
+ reinit_frame_cache ();
+ if (gdbarch_software_single_step (gdbarch,
+
get_current_frame ()))
+ step = 0;
+ set_executing (inferior_ptid, 1);
+ }
This is because in record_wait, we cannot call get_current_frame ()
directly. And the frame message need refresh each exec cycle.
2. Ping found that reverse-exec cannot single step in RISC board.
That is because "gdbarch_software_single_step" just can insert the
breakpoint to the next addr. So I add following:
@@ -1436,7 +1436,8 @@ maybe_software_singlestep (struct gdbarc
{
int hw_step = 1;
- if (gdbarch_software_single_step_p (gdbarch)
+ if (execution_direction == EXEC_FORWARD
+ && gdbarch_software_single_step_p (gdbarch)
&& gdbarch_software_single_step (gdbarch, get_current_frame ()))
If reverse, gdb will not user sss breakpoint to single step.
3. Ping got some gdb_assert in sometime. And I am not close to his
board. So I didn't know what happen. So I add following:
@@ -1534,7 +1535,8 @@ a command like `return' or `jump' to con
/* If STEP is set, it's a request to use hardware stepping
facilities. But in that case, we should never
use singlestep breakpoint. */
- gdb_assert (!(singlestep_breakpoints_inserted_p && step));
+ gdb_assert (!(execution_direction == EXEC_FORWARD
+ && singlestep_breakpoints_inserted_p && step));
The lost one still need be test.
Thanks,
Hui
[-- Attachment #2: prec_software_single_step.txt --]
[-- Type: text/plain, Size: 5450 bytes --]
---
breakpoint.c | 10 +++++++++
breakpoint.h | 1
infrun.c | 6 +++--
record.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
4 files changed, 74 insertions(+), 7 deletions(-)
--- a/breakpoint.c
+++ b/breakpoint.c
@@ -10293,6 +10293,16 @@ insert_single_step_breakpoint (struct gd
paddress (gdbarch, next_pc));
}
+/* Check if the breakpoints used for software single stepping
+ were inserted or not. */
+
+int
+single_step_breakpoints_inserted (void)
+{
+ return (single_step_breakpoints[0] != NULL
+ || single_step_breakpoints[1] != NULL);
+}
+
/* Remove and delete any breakpoints used for software single step. */
void
--- a/breakpoint.h
+++ b/breakpoint.h
@@ -983,6 +983,7 @@ extern int remove_hw_watchpoints (void);
twice before remove is called. */
extern void insert_single_step_breakpoint (struct gdbarch *,
struct address_space *, CORE_ADDR);
+extern int single_step_breakpoints_inserted (void);
extern void remove_single_step_breakpoints (void);
/* Manage manual breakpoints, separate from the normal chain of
--- a/infrun.c
+++ b/infrun.c
@@ -1436,7 +1436,8 @@ maybe_software_singlestep (struct gdbarc
{
int hw_step = 1;
- if (gdbarch_software_single_step_p (gdbarch)
+ if (execution_direction == EXEC_FORWARD
+ && gdbarch_software_single_step_p (gdbarch)
&& gdbarch_software_single_step (gdbarch, get_current_frame ()))
{
hw_step = 0;
@@ -1534,7 +1535,8 @@ a command like `return' or `jump' to con
/* If STEP is set, it's a request to use hardware stepping
facilities. But in that case, we should never
use singlestep breakpoint. */
- gdb_assert (!(singlestep_breakpoints_inserted_p && step));
+ gdb_assert (!(execution_direction == EXEC_FORWARD
+ && singlestep_breakpoints_inserted_p && step));
/* Decide the set of threads to ask the target to resume. Start
by assuming everything will be resumed, than narrow the set
--- a/record.c
+++ b/record.c
@@ -1007,9 +1007,43 @@ record_resume (struct target_ops *ops, p
if (!RECORD_IS_REPLAY)
{
+ struct gdbarch *gdbarch = target_thread_architecture (ptid);
+
record_message (get_current_regcache (), signal);
- record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
- signal);
+
+ if (!step)
+ {
+ /* This is not hard single step. */
+ if (!gdbarch_software_single_step_p (gdbarch))
+ {
+ /* This is a normal continue. */
+ step = 1;
+ }
+ else
+ {
+ /* This arch support soft sigle step. */
+ if (single_step_breakpoints_inserted ())
+ {
+ /* This is a soft single step. */
+ record_resume_step = 1;
+ }
+ else
+ {
+ /* This is a continue.
+ Try to insert a soft single step breakpoint. */
+ if (!gdbarch_software_single_step (gdbarch,
+ get_current_frame ()))
+ {
+ /* This system don't want use soft single step.
+ Use hard sigle step. */
+ step = 1;
+ }
+ }
+ }
+ }
+
+ record_beneath_to_resume (record_beneath_to_resume_ops,
+ ptid, step, signal);
}
}
@@ -1082,12 +1116,16 @@ record_wait (struct target_ops *ops,
/* This is not a single step. */
ptid_t ret;
CORE_ADDR tmp_pc;
+ struct gdbarch *gdbarch = target_thread_architecture (inferior_ptid);
while (1)
{
ret = record_beneath_to_wait (record_beneath_to_wait_ops,
ptid, status, options);
+ if (single_step_breakpoints_inserted ())
+ remove_single_step_breakpoints ();
+
/* Is this a SIGTRAP? */
if (status->kind == TARGET_WAITKIND_STOPPED
&& status->value.sig == TARGET_SIGNAL_TRAP)
@@ -1124,8 +1162,12 @@ record_wait (struct target_ops *ops,
}
else
{
- /* This must be a single-step trap. Record the
- insn and issue another step. */
+ /* This is a single-step trap. Record the
+ insn and issue another step.
+ FIXME: this part can be a random SIGTRAP too.
+ But GDB cannot handle it. */
+ int step = 1;
+
if (!record_message_wrapper_safe (regcache,
TARGET_SIGNAL_0))
{
@@ -1134,8 +1176,20 @@ record_wait (struct target_ops *ops,
break;
}
+ if (gdbarch_software_single_step_p (gdbarch))
+ {
+ /* Try to insert the software single step breakpoint.
+ If insert success, set step to 0. */
+ set_executing (inferior_ptid, 0);
+ reinit_frame_cache ();
+ if (gdbarch_software_single_step (gdbarch,
+ get_current_frame ()))
+ step = 0;
+ set_executing (inferior_ptid, 1);
+ }
+
record_beneath_to_resume (record_beneath_to_resume_ops,
- ptid, 1,
+ ptid, step,
TARGET_SIGNAL_0);
continue;
}
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] Add support of software single step to process record
2010-05-27 6:51 ` Hui Zhu
@ 2010-06-11 13:55 ` Pedro Alves
2010-06-20 7:29 ` Hui Zhu
0 siblings, 1 reply; 8+ messages in thread
From: Pedro Alves @ 2010-06-11 13:55 UTC (permalink / raw)
To: Hui Zhu
Cc: ping huang, shuchang zhou, gdb-patches, Joel Brobecker,
Michael Snyder, paawan oza, Tom Tromey
Hi Hui,
> 3. Ping got some gdb_assert in sometime. And I am not close to his
> board. So I didn't know what happen. So I add following:
> @@ -1534,7 +1535,8 @@ a command like `return' or `jump' to con
> /* If STEP is set, it's a request to use hardware stepping
> facilities. But in that case, we should never
> use singlestep breakpoint. */
> - gdb_assert (!(singlestep_breakpoints_inserted_p && step));
> + gdb_assert (!(execution_direction == EXEC_FORWARD
> + && singlestep_breakpoints_inserted_p && step));
>
> The lost one still need be test.
I'm felling a bit dense, and I don't see what is that actually
catching. If going backwards, the assertion always ends up
evaled as true, nomatter if sofware single-steps are inserted
or not, or whether `step' is set. Did you mean to assert
that when going backwards, there shouldn't ever be software
single-step breakpoints inserted?
This patch is okay otherwise. Thanks.
--
Pedro Alves
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] Add support of software single step to process record
2010-06-11 13:55 ` Pedro Alves
@ 2010-06-20 7:29 ` Hui Zhu
2010-06-22 10:13 ` Pedro Alves
0 siblings, 1 reply; 8+ messages in thread
From: Hui Zhu @ 2010-06-20 7:29 UTC (permalink / raw)
To: Pedro Alves
Cc: ping huang, shuchang zhou, gdb-patches, Joel Brobecker,
Michael Snyder, paawan oza, Tom Tromey
On Fri, Jun 11, 2010 at 21:55, Pedro Alves <pedro@codesourcery.com> wrote:
> Hi Hui,
>
>> 3. Ping got some gdb_assert in sometime. And I am not close to his
>> board. So I didn't know what happen. So I add following:
>> @@ -1534,7 +1535,8 @@ a command like `return' or `jump' to con
>> /* If STEP is set, it's a request to use hardware stepping
>> facilities. But in that case, we should never
>> use singlestep breakpoint. */
>> - gdb_assert (!(singlestep_breakpoints_inserted_p && step));
>> + gdb_assert (!(execution_direction == EXEC_FORWARD
>> + && singlestep_breakpoints_inserted_p && step));
>>
>> The lost one still need be test.
>
> I'm felling a bit dense, and I don't see what is that actually
> catching. If going backwards, the assertion always ends up
> evaled as true, nomatter if sofware single-steps are inserted
> or not, or whether `step' is set. Did you mean to assert
> that when going backwards, there shouldn't ever be software
> single-step breakpoints inserted?
>
> This patch is okay otherwise. Thanks.
>
> --
Thanks Pedro.
I was also confused by this issue too. I thought it will never happen
too. But Ping said he got this issue. And I didn't have the risc
board to test. So I gived up and put this patch to him.
So I think this patch is not very hurry to checked in until some one
post a risc prec support patch. At that time, I will make this issue
clear.
Best regards,
Hui
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] Add support of software single step to process record
2010-06-20 7:29 ` Hui Zhu
@ 2010-06-22 10:13 ` Pedro Alves
2010-07-19 7:58 ` Hui Zhu
0 siblings, 1 reply; 8+ messages in thread
From: Pedro Alves @ 2010-06-22 10:13 UTC (permalink / raw)
To: Hui Zhu
Cc: ping huang, shuchang zhou, gdb-patches, Joel Brobecker,
Michael Snyder, paawan oza, Tom Tromey
Hi Hui,
On Sunday 20 June 2010 08:28:40, Hui Zhu wrote:
> On Fri, Jun 11, 2010 at 21:55, Pedro Alves <pedro@codesourcery.com> wrote:
> > I'm felling a bit dense, and I don't see what is that actually
> > catching. If going backwards, the assertion always ends up
> > evaled as true, nomatter if sofware single-steps are inserted
> > or not, or whether `step' is set. Did you mean to assert
> > that when going backwards, there shouldn't ever be software
> > single-step breakpoints inserted?
> >
> > This patch is okay otherwise. Thanks.
>
> Thanks Pedro.
> I was also confused by this issue too. I thought it will never happen
> too. But Ping said he got this issue. And I didn't have the risc
> board to test. So I gived up and put this patch to him.
>
> So I think this patch is not very hurry to checked in until some one
> post a risc prec support patch. At that time, I will make this issue
> clear.
I'd be fine with putting the patch in now, but without the change to
that gdb_assert. It looked like a step in the right direction,
and we can fix any left issues later.
--
Pedro Alves
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] Add support of software single step to process record
2010-06-22 10:13 ` Pedro Alves
@ 2010-07-19 7:58 ` Hui Zhu
0 siblings, 0 replies; 8+ messages in thread
From: Hui Zhu @ 2010-07-19 7:58 UTC (permalink / raw)
To: Pedro Alves
Cc: ping huang, shuchang zhou, gdb-patches, Joel Brobecker,
Michael Snyder, paawan oza, Tom Tromey
On Tue, Jun 22, 2010 at 18:12, Pedro Alves <pedro@codesourcery.com> wrote:
> Hi Hui,
>
> On Sunday 20 June 2010 08:28:40, Hui Zhu wrote:
>> On Fri, Jun 11, 2010 at 21:55, Pedro Alves <pedro@codesourcery.com> wrote:
>> > I'm felling a bit dense, and I don't see what is that actually
>> > catching. If going backwards, the assertion always ends up
>> > evaled as true, nomatter if sofware single-steps are inserted
>> > or not, or whether `step' is set. Did you mean to assert
>> > that when going backwards, there shouldn't ever be software
>> > single-step breakpoints inserted?
>> >
>> > This patch is okay otherwise. Thanks.
>>
>> Thanks Pedro.
>> I was also confused by this issue too. I thought it will never happen
>> too. But Ping said he got this issue. And I didn't have the risc
>> board to test. So I gived up and put this patch to him.
>>
>> So I think this patch is not very hurry to checked in until some one
>> post a risc prec support patch. At that time, I will make this issue
>> clear.
>
> I'd be fine with putting the patch in now, but without the change to
> that gdb_assert. It looked like a step in the right direction,
> and we can fix any left issues later.
>
> --
> Pedro Alves
>
Agree with you.
I delay this patch some days because I want make it check in after 7.2.
Now, following patch checked in.
Thanks,
Hui
2010-07-19 Hui Zhu <teawater@gmail.com>
* breakpoint.c (single_step_breakpoints_inserted): New
function.
* breakpoint.h (single_step_breakpoints_inserted): Extern.
* infrun.c (maybe_software_singlestep): Add check code.
* record.c (record_resume): Add code for software single step.
(record_wait): Ditto.
---
breakpoint.c | 10 +++++++++
breakpoint.h | 1
infrun.c | 3 +-
record.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
4 files changed, 72 insertions(+), 6 deletions(-)
--- a/breakpoint.c
+++ b/breakpoint.c
@@ -10468,6 +10468,16 @@ insert_single_step_breakpoint (struct gd
paddress (gdbarch, next_pc));
}
+/* Check if the breakpoints used for software single stepping
+ were inserted or not. */
+
+int
+single_step_breakpoints_inserted (void)
+{
+ return (single_step_breakpoints[0] != NULL
+ || single_step_breakpoints[1] != NULL);
+}
+
/* Remove and delete any breakpoints used for software single step. */
void
--- a/breakpoint.h
+++ b/breakpoint.h
@@ -984,6 +984,7 @@ extern int remove_hw_watchpoints (void);
twice before remove is called. */
extern void insert_single_step_breakpoint (struct gdbarch *,
struct address_space *, CORE_ADDR);
+extern int single_step_breakpoints_inserted (void);
extern void remove_single_step_breakpoints (void);
/* Manage manual breakpoints, separate from the normal chain of
--- a/infrun.c
+++ b/infrun.c
@@ -1515,7 +1515,8 @@ maybe_software_singlestep (struct gdbarc
{
int hw_step = 1;
- if (gdbarch_software_single_step_p (gdbarch)
+ if (execution_direction == EXEC_FORWARD
+ && gdbarch_software_single_step_p (gdbarch)
&& gdbarch_software_single_step (gdbarch, get_current_frame ()))
{
hw_step = 0;
--- a/record.c
+++ b/record.c
@@ -1011,9 +1011,43 @@ record_resume (struct target_ops *ops, p
if (!RECORD_IS_REPLAY)
{
+ struct gdbarch *gdbarch = target_thread_architecture (ptid);
+
record_message (get_current_regcache (), signal);
- record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
- signal);
+
+ if (!step)
+ {
+ /* This is not hard single step. */
+ if (!gdbarch_software_single_step_p (gdbarch))
+ {
+ /* This is a normal continue. */
+ step = 1;
+ }
+ else
+ {
+ /* This arch support soft sigle step. */
+ if (single_step_breakpoints_inserted ())
+ {
+ /* This is a soft single step. */
+ record_resume_step = 1;
+ }
+ else
+ {
+ /* This is a continue.
+ Try to insert a soft single step breakpoint. */
+ if (!gdbarch_software_single_step (gdbarch,
+ get_current_frame ()))
+ {
+ /* This system don't want use soft single step.
+ Use hard sigle step. */
+ step = 1;
+ }
+ }
+ }
+ }
+
+ record_beneath_to_resume (record_beneath_to_resume_ops,
+ ptid, step, signal);
}
}
@@ -1089,12 +1123,16 @@ record_wait (struct target_ops *ops,
/* This is not a single step. */
ptid_t ret;
CORE_ADDR tmp_pc;
+ struct gdbarch *gdbarch = target_thread_architecture (inferior_ptid);
while (1)
{
ret = record_beneath_to_wait (record_beneath_to_wait_ops,
ptid, status, options);
+ if (single_step_breakpoints_inserted ())
+ remove_single_step_breakpoints ();
+
if (record_resume_step)
return ret;
@@ -1134,8 +1172,12 @@ record_wait (struct target_ops *ops,
}
else
{
- /* This must be a single-step trap. Record the
- insn and issue another step. */
+ /* This is a single-step trap. Record the
+ insn and issue another step.
+ FIXME: this part can be a random SIGTRAP too.
+ But GDB cannot handle it. */
+ int step = 1;
+
if (!record_message_wrapper_safe (regcache,
TARGET_SIGNAL_0))
{
@@ -1144,8 +1186,20 @@ record_wait (struct target_ops *ops,
break;
}
+ if (gdbarch_software_single_step_p (gdbarch))
+ {
+ /* Try to insert the software single step breakpoint.
+ If insert success, set step to 0. */
+ set_executing (inferior_ptid, 0);
+ reinit_frame_cache ();
+ if (gdbarch_software_single_step (gdbarch,
+
get_current_frame ()))
+ step = 0;
+ set_executing (inferior_ptid, 1);
+ }
+
record_beneath_to_resume (record_beneath_to_resume_ops,
- ptid, 1,
+ ptid, step,
TARGET_SIGNAL_0);
continue;
}
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2010-07-19 7:58 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <daef60380912180021h5d029e55k7dc4e3f4c8d33b36@mail.gmail.com>
[not found] ` <20091223065141.GT2788@adacore.com>
[not found] ` <daef60380912230123v3cc9bf79qcb0cd3faa84753f3@mail.gmail.com>
[not found] ` <200912241738.19780.pedro@codesourcery.com>
2010-01-04 14:23 ` [RFC] Add support of software single step to process record Hui Zhu
2010-01-08 16:24 ` Pedro Alves
2010-05-25 5:14 ` Hui Zhu
2010-05-27 6:51 ` Hui Zhu
2010-06-11 13:55 ` Pedro Alves
2010-06-20 7:29 ` Hui Zhu
2010-06-22 10:13 ` Pedro Alves
2010-07-19 7:58 ` Hui Zhu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).