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

[Pedro Alves <pedro at codesourcery dot com>]

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