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

[Hui Zhu <teawater@gmail.com>]

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