Re: [RFC] Add support of software single step to process record

[Hui Zhu <teawater@gmail.com>]

[-- 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;
 		    }