[PATCH] Fix displaced stepping for remote targets

[PATCH] Fix displaced stepping for remote targets

[Luis Gustavo]

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

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.

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.

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?

Luis

[-- Attachment #2: 0001-fix_remote_displaced_stepping.diff --]
[-- Type: text/x-patch, Size: 1885 bytes --]

2012-03-19  Luis Machado  <lgustavo@codesourcery.com>

	* infrun.c (resume): Change order of statements and
	update PC to the real PC in case of displaced stepping.

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;
+
           fprintf_unfiltered (gdb_stdlog, "displaced: run %s: ",
                               paddress (resume_gdbarch, actual_pc));
           read_memory (actual_pc, buf, sizeof (buf));
           displaced_step_dump_bytes (gdb_stdlog, buf, sizeof (buf));
         }
 
+      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;
+	}
+
       /* Install inferior's terminal modes.  */
       target_terminal_inferior ();
 

Re: [PATCH] Fix displaced stepping for remote targets

[Yao Qi]

On 03/20/2012 10:02 AM, Luis Gustavo wrote:
> 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.

On some arch, the original instruction may be translated to more than
one instructions, and copied to scratch pad.  So, we can't do
single-step in scratch pad.

Even the case that one instruction is generated/copied to scratch pad
can be optimized in your approach, but not sure it breaks
software-single-step arch or not.

-- 
Yao (齐尧)

Re: [PATCH] Fix displaced stepping for remote targets

[Luis Gustavo]

On 03/20/2012 01:34 AM, Yao Qi wrote:
> On 03/20/2012 10:02 AM, Luis Gustavo wrote:
>> 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.
>
> On some arch, the original instruction may be translated to more than
> one instructions, and copied to scratch pad.  So, we can't do
> single-step in scratch pad.

During a few tests x86 seems to do single-stepping in the scratch pad 
when using displaced stepping, or at least it sends the correct vCont;s 
packet.

>
> Even the case that one instruction is generated/copied to scratch pad
> can be optimized in your approach, but not sure it breaks
> software-single-step arch or not.
>

In my case GDB thinks there's a breakpoint right at PC since PC points 
to the old location. GDB just continues through vCont;c. This does not 
work as execution will just re-start from the scratch pad area and will 
cause unpredictable behavior.

Maybe we need a better mechanism for this then.

Luis

Re: [PATCH] Fix displaced stepping for remote targets

[Yao Qi]

On 03/20/2012 12:38 PM, Luis Gustavo wrote:
> On 03/20/2012 01:34 AM, Yao Qi wrote:
>> On 03/20/2012 10:02 AM, Luis Gustavo wrote:
>>> 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.
>>
>> On some arch, the original instruction may be translated to more than
>> one instructions, and copied to scratch pad.  So, we can't do
>> single-step in scratch pad.
> 
> During a few tests x86 seems to do single-stepping in the scratch pad
> when using displaced stepping, or at least it sends the correct vCont;s
> packet.
> 

GDB writes only one instruction in scratch pad on i386 and amd64.  That
may be the reason why no fail shows up.

-- 
Yao (齐尧)

Re: [PATCH] Fix displaced stepping for remote targets

[Luis Gustavo]

On 03/20/2012 03:53 AM, Yao Qi wrote:
> On 03/20/2012 12:38 PM, Luis Gustavo wrote:
>> On 03/20/2012 01:34 AM, Yao Qi wrote:
>>> On 03/20/2012 10:02 AM, Luis Gustavo wrote:
>>>> 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.
>>>
>>> On some arch, the original instruction may be translated to more than
>>> one instructions, and copied to scratch pad.  So, we can't do
>>> single-step in scratch pad.
>>
>> During a few tests x86 seems to do single-stepping in the scratch pad
>> when using displaced stepping, or at least it sends the correct vCont;s
>> packet.
>>
>
> GDB writes only one instruction in scratch pad on i386 and amd64.  That
> may be the reason why no fail shows up.
>

Yes, most probably. I think ppc does this as well.

If a target can't use hw single-stepping to execute instructions in the 
scratch pad, i think it should disallow it by returning 0 in 
gdbarch_displaced_step_hw_singlestep. Does it make sense?

Luis

Re: [PATCH] Fix displaced stepping for remote targets

[Yao Qi]

On 03/21/2012 04:33 AM, Luis Gustavo wrote:
> If a target can't use hw single-stepping to execute instructions in the
> scratch pad, i think it should disallow it by returning 0 in
> gdbarch_displaced_step_hw_singlestep. Does it make sense?

Yes, that is what gdbarch_displaced_step_hw_singlestep for, AFAIK.  I am
sorry I don't understand how gdbarch_displaced_step_hw_singlestep
related to your patch here.

-- 
Yao (齐尧)

Re: [PATCH] Fix displaced stepping for remote targets

[Luis Gustavo]

On 04/09/2012 02:45 AM, Yao Qi wrote:
> On 03/21/2012 04:33 AM, Luis Gustavo wrote:
>> If a target can't use hw single-stepping to execute instructions in the
>> scratch pad, i think it should disallow it by returning 0 in
>> gdbarch_displaced_step_hw_singlestep. Does it make sense?
>
> Yes, that is what gdbarch_displaced_step_hw_singlestep for, AFAIK.  I am
> sorry I don't understand how gdbarch_displaced_step_hw_singlestep
> related to your patch here.
>

It is not directly related. I raised that one based on your comment 
about some targets not being able to HW single-step a group of 
instructions in the scratch pad. In that case, they should report 0 for 
gdbarch_displaced_step_hw_singlestep.

My target reports 1 for gdbarch_displaced_step_hw_singlestep, and thus 
HW single-stepping should be used when doing displaced stepping.

The patch addresses a different problem, namely the overriding of the 
STEP variable's value (originally containing 
gdbarch_displaced_step_hw_singlestep ()'s result) with this call...

if (step && breakpoint_inserted_here_p (aspace, pc))
             step = 0;

If we are trying to step-over a breakpoint using displaced stepping, 
breakpoint_inserted_here_p (aspace, pc) will obviously return true, thus 
we disable stepping by setting it to 0. This looks wrong.

This has the effect of issuing a vCont;c packet to the remote target 
instead of vCont;s.

By changing the order of the statements in the code, we address this in, 
hopefully, the correct way. Unless there is some magic going on there.

Regards,
Luis

Re: [PATCH] Fix displaced stepping for remote targets

[Luis Gustavo]

Ping?

On 03/19/2012 11:02 PM, 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.
>
> 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.
>
> 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?
>
> Luis

Re: [PATCH] Fix displaced stepping for remote targets

[Pedro Alves]

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

Re: [PATCH] Fix displaced stepping for remote targets

[Luis Gustavo]

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

On 04/13/2012 02:26 PM, Pedro Alves wrote:
> 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?
>

Doh! Of course... This block looked more functional than a simple debug 
output block. No surprise it works while debugging a displaced stepping 
issue. :-)

Thanks for spotting that.

How about the attached one?

Luis

[-- Attachment #2: 0001-fix_remote_displaced_stepping.diff --]
[-- Type: text/x-patch, Size: 831 bytes --]

2012-04-13  Luis Machado  <lgustavo@codesourcery.com>

	* infrun.c (resume): Update PC address to the real PC after
	preparing to do displaced stepping.

Index: HEAD-git/gdb/infrun.c
===================================================================
--- HEAD-git.orig/gdb/infrun.c	2012-04-13 16:01:25.592842215 -0300
+++ HEAD-git/gdb/infrun.c	2012-04-13 16:22:53.652909294 -0300
@@ -1789,6 +1789,10 @@ a command like `return' or `jump' to con
 	  return;
 	}
 
+      /* Update pc to reflect the new address from which we will execute
+	 instructions due to displaced stepping.  */
+      pc = regcache_read_pc (get_thread_regcache (inferior_ptid));
+
       displaced = get_displaced_stepping_state (ptid_get_pid (inferior_ptid));
       step = gdbarch_displaced_step_hw_singlestep (gdbarch,
 						   displaced->step_closure);

Re: [PATCH] Fix displaced stepping for remote targets

[Pedro Alves]

On 04/13/2012 08:45 PM, Luis Gustavo wrote:

>>
> 
> Doh! Of course... This block looked more functional than a simple debug output block. No surprise it works while debugging a displaced stepping issue. :-)
> 
> Thanks for spotting that.
> 
> How about the attached one?


This one's okay.  Thanks.

> 2012-04-13  Luis Machado  <lgustavo@codesourcery.com>
> 
> 	* infrun.c (resume): Update PC address to the real PC after
> 	preparing to do displaced stepping.



-- 
Pedro Alves

Re: [PATCH] Fix displaced stepping for remote targets

[Luis Gustavo]

On 04/13/2012 04:59 PM, Pedro Alves wrote:
> On 04/13/2012 08:45 PM, Luis Gustavo wrote:
>
>>>
>>
>> Doh! Of course... This block looked more functional than a simple debug output block. No surprise it works while debugging a displaced stepping issue. :-)
>>
>> Thanks for spotting that.
>>
>> How about the attached one?
>
>
> This one's okay.  Thanks.
>
>> 2012-04-13  Luis Machado<lgustavo@codesourcery.com>
>>
>> 	* infrun.c (resume): Update PC address to the real PC after
>> 	preparing to do displaced stepping.
>
>
>

Thanks, i've checked it in now.

Luis