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

[Hui Zhu <teawater@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1782 bytes --]

Hi guys,

I make a new patch according to your comment.

Shuchang, please help me try this patch?

Joel, I remove the current_gdbarch() from this patch, but for
get_current_frame, I cannot find any function that fit for replace it.
 Could you help me with it?

Thanks,
Hui

On Sun, Dec 20, 2009 at 21:30, Joel Brobecker <brobecker@adacore.com> wrote:
> Hui:
>
>> >2009-12-18  Hui Zhu  <teawater@gmail.com>
>> >
>> >     * breakpoint.c (inserted_single_step_breakpoint_p): New
>> >     function.
>> >     * breakpoint.h (inserted_single_step_breakpoint_p): Extern.
>> >     * record.c (record_resume): Add code for software single step.
>> >     (record_wait): Ditto.
>
> I understand Michael's answer as approval.  I do not see any problem
> with it, but my knowledge of the target stack in the resume/wait area
> is pretty sketchy.
>
> Just a stylistic comment on the patch:
>
>> >+/* Check if the breakpoints used for software single stepping
>> >inserted or not.  */
>
> Formatting and missing "are".
>
> /* Check if the breakpoints used for software single stepping
>   are inserted or not.  */
>
>> >+int
>> >+inserted_single_step_breakpoint_p (void)
>
> Can you rename this function to:
>
>        single_step_breakpoints_inserted
>
> The "inserted" already conveys the idea of a condition/predicate,
> so the _p is superfluous in this case.
>
> I'm also a little worried about the code adding calls to functions
> that in essence return a global variable. For instance, you are
> introducing calls to get_current_frame or current_gdbarch().
> Ulrich is one of our specialists in this area, whereas I'm not sure,
> but I am wondering if we are introducing any latent issue by using
> these routines...
>
> --
> Joel
>

[-- Attachment #2: prec_software_single_step.txt --]
[-- Type: text/plain, Size: 3450 bytes --]

---
 breakpoint.c |   13 +++++++++++++
 breakpoint.h |    1 +
 record.c     |   33 ++++++++++++++++++++++++++++++---
 3 files changed, 44 insertions(+), 3 deletions(-)

--- a/breakpoint.c
+++ b/breakpoint.c
@@ -9624,6 +9624,19 @@ 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)
+{
+  if (single_step_breakpoints[0] != NULL
+      || single_step_breakpoints[1] != NULL)
+    return 1;
+
+  return 0;
+}
+
 /* 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,23 @@ 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 (gdbarch_software_single_step_p (gdbarch))
+         {
+           if (!inserted_single_step_breakpoint_p ())
+             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);
     }
 }
 
@@ -1077,6 +1091,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 +1114,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.  */
@@ -1129,9 +1147,18 @@ record_wait (struct target_ops *ops,
                            break;
   			}
 
-		      record_beneath_to_resume (record_beneath_to_resume_ops,
-						ptid, 1,
-						TARGET_SIGNAL_0);
+                      if (gdbarch_software_single_step_p (gdbarch))
+                        {
+                          gdbarch_software_single_step (gdbarch,
+                                                        get_current_frame ());
+                          record_beneath_to_resume (record_beneath_to_resume_ops,
+                                                    ptid, 0,
+                                                    TARGET_SIGNAL_0);
+                        }
+                      else
+		        record_beneath_to_resume (record_beneath_to_resume_ops,
+						  ptid, 1,
+						  TARGET_SIGNAL_0);
 		      continue;
 		    }
 		}