Re: [PATCH] Fix displaced stepping for remote targets

[Pedro Alves <palves@redhat.com>]

On 03/20/2012 02:02 AM, Luis Gustavo wrote:

> Hi,
> 
> While debugging a remote target that supports hardware single-stepping with a GDB that supports displaced stepping, i've ran into the following problem...
> 
> When reaching a breakpoint, GDB should take care of relocating the instruction underneath that breakpoint to the scratch space where it will be executed out-of-line.
> 
> If a target supports hw single-stepping for displaced stepping, GDB should just send a vCont;s packet to tell the target to step a single instruction. In my case, GDB was always sending a vCont;c instead.
> 
> I've tracked it down to infrun.c:resume, where we have this check:
> 
>       if (gdbarch_cannot_step_breakpoint (gdbarch))
>         {
>           /* Most targets can step a breakpoint instruction, thus
>              executing it normally.  But if this one cannot, just
>              continue and we will hit it anyway.  */
>           if (step && breakpoint_inserted_here_p (aspace, pc))
>             step = 0;
>         }
> 
> My target can't step breakpoints and, if we're doing displaced stepping, it's because we're trying to step off a breakpoint, thus breakpoint_inserted_here_p returns true, and we disable single-stepping by setting step to 0.


That is key.  Most targets can step breakpoints.  The issue is reproducible on x86_64 GNU/Linux
with this:

diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
index da22c1c..c98773e 100644
--- a/gdb/amd64-linux-tdep.c
+++ b/gdb/amd64-linux-tdep.c
@@ -1300,6 +1300,8 @@ amd64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   /* Reserve a number for orig_rax.  */
   set_gdbarch_num_regs (gdbarch, AMD64_LINUX_NUM_REGS);

+  set_gdbarch_cannot_step_breakpoint (gdbarch, 1);
+
   if (! tdesc_has_registers (tdesc))
     tdesc = tdesc_amd64_linux;
   tdep->tdesc = tdesc;


> 
> It seems to me we need to update the PC prior to calling breakpoint_inserted_here_p since the displaced stepping machinery adjusted the old PC to point to the space in the scratch area. That way we can properly command the target to step the displaced instruction and we can check for breakpoints at the real execution place.
> 


Yeah.

> The following patch fixes this by pushing the if block further down in the code and taking care of updating PC if displaced stepping is being used.
> 
> I've regtested this on x86 and everything looks OK. This also makes GDB send vCont;s now.
> 
> Ok?


No...

> Index: HEAD-git/gdb/infrun.c
> ===================================================================
> --- HEAD-git.orig/gdb/infrun.c	2012-03-19 20:40:57.589621232 -0300
> +++ HEAD-git/gdb/infrun.c	2012-03-19 22:12:08.729898472 -0300
> @@ -1884,15 +1884,6 @@ a command like `return' or `jump' to con
>  	  resume_ptid = inferior_ptid;
>  	}
>  
> -      if (gdbarch_cannot_step_breakpoint (gdbarch))
> -	{
> -	  /* Most targets can step a breakpoint instruction, thus
> -	     executing it normally.  But if this one cannot, just
> -	     continue and we will hit it anyway.  */
> -	  if (step && breakpoint_inserted_here_p (aspace, pc))
> -	    step = 0;
> -	}
> -
>        if (debug_displaced
>            && use_displaced_stepping (gdbarch)
>            && tp->control.trap_expected)
> @@ -1902,12 +1893,25 @@ a command like `return' or `jump' to con
>            CORE_ADDR actual_pc = regcache_read_pc (resume_regcache);
>            gdb_byte buf[4];
>  
> +	  /* Update pc to reflect the new address from which we will execute
> +	     instructions due to displaced stepping.  */
> +	  pc = actual_pc;


Notice the condition just above:

      if (debug_displaced
          ^^^^^^^^^^^^^^^
          && use_displaced_stepping (gdbarch)
          && tp->control.trap_expected)
        {
	  struct regcache *resume_regcache = get_thread_regcache (resume_ptid);
	  struct gdbarch *resume_gdbarch = get_regcache_arch (resume_regcache);
          CORE_ADDR actual_pc = regcache_read_pc (resume_regcache);
          gdb_byte buf[4];

	  /* Update pc to reflect the new address from which we will execute
	     instructions due to displaced stepping.  */
	  pc = actual_pc;

This this part of the fix is only working if you do "set debug displaced on".
Not good.  :-)  Best do this right after the displaced_step_prepare call?

-- 
Pedro Alves