[gdbserver] Disable conditional breakpoints on no-hardware-single-step targets

  From: Yao Qi <yao.qi@linaro.org>

GDBserver steps over breakpoint if the condition is false, but if target
doesn't support hardware single step, the step over is very simple, if
not incorrect, in linux-arm-low.c:

/* We only place breakpoints in empty marker functions, and thread locking
   is outside of the function.  So rather than importing software single-step,
   we can just run until exit.  */
static CORE_ADDR
arm_reinsert_addr (void)
{
  struct regcache *regcache = get_thread_regcache (current_thread, 1);
  unsigned long pc;
  collect_register_by_name (regcache, "lr", &pc);
  return pc;
}

and linux-mips-low.c does the same.  GDBserver sets a breakpoint at the
return address of the current function, resume and wait the program hits
the breakpoint in order to achieve "breakpoint step over".  What if
program hits other user breakponits during this "step over"?

It is worse if the arm/thumb interworking is considered.  Nowadays,
GDBserver arm backend unconditionally inserts arm breakpoint,

  /* Define an ARM-mode breakpoint; we only set breakpoints in the C
     library, which is most likely to be ARM.  If the kernel supports
     clone events, we will never insert a breakpoint, so even a Thumb
     C library will work; so will mixing EABI/non-EABI gdbserver and
     application.  */
#ifndef __ARM_EABI__
  (const unsigned char *) &arm_breakpoint,
#else
  (const unsigned char *) &arm_eabi_breakpoint,
#endif

note that the comments are no longer valid as C library can be compiled
in thumb mode.

When GDBserver steps over a breakpoint in arm mode function, which
returns to thumb mode, GDBserver will insert arm mode breakpoint by
mistake and the program will crash.  GDBserver alone is unable to
determine the arm/thumb mode given a PC address.  See how GDB does
it in arm-tdep.c:arm_pc_is_thumb.

After thinking about how to teach GDBserver inserting right breakpoint
(arm or thumb) for a while, I reconsider it from a different direction
that it may be unreasonable to run target-side conditional breakpoint for
targets without hardware single step.  Pedro also pointed this out here
https://sourceware.org/ml/gdb-patches/2015-04/msg00337.html

This patch is to add a new target_ops hook
supports_conditional_breakpoints, and only reply
";ConditionalBreakpoints+" if it is true.  On linux targets,
supports_conditional_breakpoints returns true if target has hardware
single step, on other targets, (win32, lynx, nto, spu), set it to NULL,
because conditional breakpoint is a linux-specific feature.

Regression tested on x86_64-linux, rebuild mingw32 gdbserver.  Don't
to rebuild gdbserver for lynx, nto and spu.

gdb/gdbserver:

2015-04-30  Yao Qi  <yao.qi@linaro.org>

	* linux-low.c (linux_supports_conditional_breakpoints): New
	function.
	(linux_target_ops): Install new target method.
	* lynx-low.c (lynx_target_ops): Install NULL hook for
	supports_conditional_breakpoints.
	* nto-low.c (nto_target_ops): Likewise.
	* spu-low.c (spu_target_ops): Likewise.
	* win32-low.c (win32_target_ops): Likewise.
	* server.c (handle_query): Check
	target_supports_conditional_breakpoints.
	* target.h (struct target_ops) <supports_conditional_breakpoints>:
	New field.
	(target_supports_conditional_breakpoints): New macro.
---
 gdb/gdbserver/linux-low.c | 14 ++++++++++++++
 gdb/gdbserver/lynx-low.c  |  3 +++
 gdb/gdbserver/nto-low.c   |  3 +++
 gdb/gdbserver/server.c    |  3 ++-
 gdb/gdbserver/spu-low.c   |  1 +
 gdb/gdbserver/target.h    |  8 ++++++++
 gdb/gdbserver/win32-low.c |  3 +++
 7 files changed, 34 insertions(+), 1 deletion(-)
  

On 04/30/2015 12:23 PM, Yao Qi wrote:
> From: Yao Qi <yao.qi@linaro.org>
>    /* Define an ARM-mode breakpoint; we only set breakpoints in the C
>       library, which is most likely to be ARM.  If the kernel supports
>       clone events, we will never insert a breakpoint, so even a Thumb
>       C library will work; so will mixing EABI/non-EABI gdbserver and
>       application.  */
> #ifndef __ARM_EABI__
>    (const unsigned char *) &arm_breakpoint,
> #else
>    (const unsigned char *) &arm_eabi_breakpoint,
> #endif
>
> note that the comments are no longer valid as C library can be compiled
> in thumb mode.

Could we update the comments at the same time ?...
>
> When GDBserver steps over a breakpoint in arm mode function, which
> returns to thumb mode, GDBserver will insert arm mode breakpoint by
> mistake and the program will crash.  GDBserver alone is unable to
> determine the arm/thumb mode given a PC address.  See how GDB does
> it in arm-tdep.c:arm_pc_is_thumb.
>
> After thinking about how to teach GDBserver inserting right breakpoint
> (arm or thumb) for a while, I reconsider it from a different direction
> that it may be unreasonable to run target-side conditional breakpoint for
> targets without hardware single step.  Pedro also pointed this out here
> https://sourceware.org/ml/gdb-patches/2015-04/msg00337.html
>

I'm looking into teaching gdbserver about inserting the right
breakpoint and possibly software single-step like this post : 
https://sourceware.org/ml/gdb/2012-10/msg00077.html wanted to do...

It would also fix the problem where we would step-over other breakpoints...

But it does seems very complex to say the least, I'm still early in my 
investigation but if you could share your thoughts on how you came to 
think of it as unreasonable to fix arm-mode and possibly the 
single-stepping it would be appreciated ?

Regards,

Antoine Tremblay
  

Antoine Tremblay <antoine.tremblay@ericsson.com> writes:

>> note that the comments are no longer valid as C library can be compiled
>> in thumb mode.
>
> Could we update the comments at the same time ?...

Yes, we can.  Everyone can post patches here to fix bugs and mistakes in
the source.  In fact, this part of code needs some changes, we start
to compile C library in thumb mode in the last several years, and use
the kernel new enough to support tracing clones.  IMO, that is why we
don't see anything wrong while the code is not 100% correct.

>>
>> When GDBserver steps over a breakpoint in arm mode function, which
>> returns to thumb mode, GDBserver will insert arm mode breakpoint by
>> mistake and the program will crash.  GDBserver alone is unable to
>> determine the arm/thumb mode given a PC address.  See how GDB does
>> it in arm-tdep.c:arm_pc_is_thumb.
>>
>> After thinking about how to teach GDBserver inserting right breakpoint
>> (arm or thumb) for a while, I reconsider it from a different direction
>> that it may be unreasonable to run target-side conditional breakpoint for
>> targets without hardware single step.  Pedro also pointed this out here
>> https://sourceware.org/ml/gdb-patches/2015-04/msg00337.html
>>
>
> I'm looking into teaching gdbserver about inserting the right
> breakpoint and possibly software single-step like this post :
> https://sourceware.org/ml/gdb/2012-10/msg00077.html wanted to do...
>
> It would also fix the problem where we would step-over other breakpoints...
>
> But it does seems very complex to say the least, I'm still early in my
> investigation but if you could share your thoughts on how you came to
> think of it as unreasonable to fix arm-mode and possibly the
> single-stepping it would be appreciated ?

I think we've got some conclusions in the url above that it is
complicated to do software-single step in GDBserver, not only calculating
the next pc, but also handling execution control after software single
step is involved.
  

On 04/30/2015 05:23 PM, Yao Qi wrote:
> From: Yao Qi <yao.qi@linaro.org>
> 
> GDBserver steps over breakpoint if the condition is false, but if target
> doesn't support hardware single step, the step over is very simple, if
> not incorrect, in linux-arm-low.c:
> 
> /* We only place breakpoints in empty marker functions, and thread locking
>    is outside of the function.  So rather than importing software single-step,
>    we can just run until exit.  */
> static CORE_ADDR
> arm_reinsert_addr (void)
> {
>   struct regcache *regcache = get_thread_regcache (current_thread, 1);
>   unsigned long pc;
>   collect_register_by_name (regcache, "lr", &pc);
>   return pc;
> }
> 
> and linux-mips-low.c does the same.  GDBserver sets a breakpoint at the
> return address of the current function, resume and wait the program hits
> the breakpoint in order to achieve "breakpoint step over".  What if
> program hits other user breakponits during this "step over"?
> 
> It is worse if the arm/thumb interworking is considered.  Nowadays,
> GDBserver arm backend unconditionally inserts arm breakpoint,
> 
>   /* Define an ARM-mode breakpoint; we only set breakpoints in the C
>      library, which is most likely to be ARM.  If the kernel supports
>      clone events, we will never insert a breakpoint, so even a Thumb
>      C library will work; so will mixing EABI/non-EABI gdbserver and
>      application.  */
> #ifndef __ARM_EABI__
>   (const unsigned char *) &arm_breakpoint,
> #else
>   (const unsigned char *) &arm_eabi_breakpoint,
> #endif
> 
> note that the comments are no longer valid as C library can be compiled
> in thumb mode.
> 
> When GDBserver steps over a breakpoint in arm mode function, which
> returns to thumb mode, GDBserver will insert arm mode breakpoint by
> mistake and the program will crash.  GDBserver alone is unable to
> determine the arm/thumb mode given a PC address.  See how GDB does
> it in arm-tdep.c:arm_pc_is_thumb.

Of a random PC address no, but in gdbserver's case, I think that it
would work, because we need it to step over a breakpoint that is
at the current PC.  So we could:

 #1 - Get the mode of the current PC from the thread's $cpsr register.

 #2 - Get the mode of the next PC by looking at the instruction that is
      about to be executed (at current PC).  If bx and blx, which change
      modes, check the thumb bit of the destination address.
      For all other instructions, same mode as the current PC.

> 
> After thinking about how to teach GDBserver inserting right breakpoint
> (arm or thumb) for a while, I reconsider it from a different direction
> that it may be unreasonable to run target-side conditional breakpoint for
> targets without hardware single step.  Pedro also pointed this out here
> https://sourceware.org/ml/gdb-patches/2015-04/msg00337.html

In the end I was somewhat convinced that things ended up working.
But I certainly don't object to this patch.

> +  /* Although win32-i386 has hardware single step, still disable this
> +     feature for win32, because it is quite GNU/Linux specific.  */
> +  NULL, /* supports_conditional_breakpoints */

TBC, it's not that the feature is GNU/Linux specific (like something
related to system calls or some detail in glibc), but that the support
for conditional breakpoints is baked into linux-low.c instead of
in generic code.

>    win32_stopped_by_watchpoint,
>    win32_stopped_data_address,
>    NULL, /* read_offsets */


Thanks,
Pedro Alves
  

Pedro Alves <palves@redhat.com> writes:

> Of a random PC address no, but in gdbserver's case, I think that it
> would work, because we need it to step over a breakpoint that is
> at the current PC.  So we could:
>
>  #1 - Get the mode of the current PC from the thread's $cpsr register.
>
>  #2 - Get the mode of the next PC by looking at the instruction that is
>       about to be executed (at current PC).  If bx and blx, which change
>       modes, check the thumb bit of the destination address.
>       For all other instructions, same mode as the current PC.
>

We can know the mode of the next PC in this way, but we don't know the
address of the next PC.  In fact, we need to know the address of the
next PC first, and then determine the mode of the next PC.  Probably, we
need something as below,

 1. Teach GDBserver to compute the address of the next PC,
 2. Determine the mode of the next PC as you suggested,
 3. Add breakpoint_from_pc hook in target_ops, so that the right
    breakpoint instruction can be selected.

>> 
>> After thinking about how to teach GDBserver inserting right breakpoint
>> (arm or thumb) for a while, I reconsider it from a different direction
>> that it may be unreasonable to run target-side conditional breakpoint for
>> targets without hardware single step.  Pedro also pointed this out here
>> https://sourceware.org/ml/gdb-patches/2015-04/msg00337.html
>
> In the end I was somewhat convinced that things ended up working.
> But I certainly don't object to this patch.
>
>> +  /* Although win32-i386 has hardware single step, still disable this
>> +     feature for win32, because it is quite GNU/Linux specific.  */
>> +  NULL, /* supports_conditional_breakpoints */
>
> TBC, it's not that the feature is GNU/Linux specific (like something
> related to system calls or some detail in glibc), but that the support
> for conditional breakpoints is baked into linux-low.c instead of
> in generic code.

How about writing comments like this?

  /* Although win32-i386 has hardware single step, still disable this
     feature for win32, because it is implemented in linux-low.c instead
     of in generic code.  */
  

On 05/07/2015 06:47 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
>
>> Of a random PC address no, but in gdbserver's case, I think that it
>> would work, because we need it to step over a breakpoint that is
>> at the current PC.  So we could:
>>
>>   #1 - Get the mode of the current PC from the thread's $cpsr register.
>>
>>   #2 - Get the mode of the next PC by looking at the instruction that is
>>        about to be executed (at current PC).  If bx and blx, which change
>>        modes, check the thumb bit of the destination address.
>>        For all other instructions, same mode as the current PC.
>>
>
> We can know the mode of the next PC in this way, but we don't know the
> address of the next PC.  In fact, we need to know the address of the
> next PC first, and then determine the mode of the next PC.  Probably, we
> need something as below,
>
>   1. Teach GDBserver to compute the address of the next PC,
>   2. Determine the mode of the next PC as you suggested,
>   3. Add breakpoint_from_pc hook in target_ops, so that the right
>      breakpoint instruction can be selected.
>

Just fyi, I'm working on doing this at the moment, my investigation is 
still incomplete...

So far I mainly plan to port the arm_get_next code to gdbserver, to 
accomplish 1. , the code doesn't have so many deps so it should be ok
2. by looking at $cpsr
3. should be fine as 1 and 2 are done...

I don't know however yet the best strategy to share the code but I'm 
guessing I could make the parts that don't have any deps to gdbarch etc 
in a shared function with gdb/gdbserver... Any pointers on this are 
welcome...


>>>
>>> After thinking about how to teach GDBserver inserting right breakpoint
>>> (arm or thumb) for a while, I reconsider it from a different direction
>>> that it may be unreasonable to run target-side conditional breakpoint for
>>> targets without hardware single step.  Pedro also pointed this out here
>>> https://sourceware.org/ml/gdb-patches/2015-04/msg00337.html
>>
>> In the end I was somewhat convinced that things ended up working.
>> But I certainly don't object to this patch.
>>
>>> +  /* Although win32-i386 has hardware single step, still disable this
>>> +     feature for win32, because it is quite GNU/Linux specific.  */
>>> +  NULL, /* supports_conditional_breakpoints */
>>
>> TBC, it's not that the feature is GNU/Linux specific (like something
>> related to system calls or some detail in glibc), but that the support
>> for conditional breakpoints is baked into linux-low.c instead of
>> in generic code.
>
> How about writing comments like this?
>
>    /* Although win32-i386 has hardware single step, still disable this
>       feature for win32, because it is implemented in linux-low.c instead
>       of in generic code.  */
>
  

On 05/07/2015 11:47 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> Of a random PC address no, but in gdbserver's case, I think that it
>> would work, because we need it to step over a breakpoint that is
>> at the current PC.  So we could:
>>
>>  #1 - Get the mode of the current PC from the thread's $cpsr register.
>>
>>  #2 - Get the mode of the next PC by looking at the instruction that is
>>       about to be executed (at current PC).  If bx and blx, which change
>>       modes, check the thumb bit of the destination address.
>>       For all other instructions, same mode as the current PC.
>>
> 
> We can know the mode of the next PC in this way, but we don't know the
> address of the next PC.  In fact, we need to know the address of the
> next PC first, and then determine the mode of the next PC.  Probably, we
> need something as below,

Yes, certainly.  I was just replying to this part:

> When GDBserver steps over a breakpoint in arm mode function, which
> returns to thumb mode, GDBserver will insert arm mode breakpoint by
> mistake and the program will crash.  GDBserver alone is unable to
> determine the arm/thumb mode given a PC address.  See how GDB does
> it in arm-tdep.c:arm_pc_is_thumb.

>  1. Teach GDBserver to compute the address of the next PC,
>  2. Determine the mode of the next PC as you suggested,
>  3. Add breakpoint_from_pc hook in target_ops, so that the right
>     breakpoint instruction can be selected.

Not sure about #3.  We'd need some target method to get the breakpoint
opcode, like breakpoint_from_pc, but this wouldn't handle random
addresses.  I think we can instead use the size parameter passed to
the_target->insert_point already.  That parameter already has per-arch
meaning for breakpoint packets: 2=Thumb, 3=Thumb2, 4=ARM.
So we'd more likely end up with a breakpoint_from_size or something.

>> TBC, it's not that the feature is GNU/Linux specific (like something
>> related to system calls or some detail in glibc), but that the support
>> for conditional breakpoints is baked into linux-low.c instead of
>> in generic code.
> 
> How about writing comments like this?
> 
>   /* Although win32-i386 has hardware single step, still disable this
>      feature for win32, because it is implemented in linux-low.c instead
>      of in generic code.  */

Fine with me.

Thanks,
Pedro Alves
  

On 05/07/2015 12:45 PM, Antoine Tremblay wrote:

> Just fyi, I'm working on doing this at the moment, my investigation is 
> still incomplete...
> 
> So far I mainly plan to port the arm_get_next code to gdbserver, to 
> accomplish 1. , the code doesn't have so many deps so it should be ok
> 2. by looking at $cpsr
> 3. should be fine as 1 and 2 are done...
> 
> I don't know however yet the best strategy to share the code but I'm 
> guessing I could make the parts that don't have any deps to gdbarch etc 
> in a shared function with gdb/gdbserver... Any pointers on this are 
> welcome...

Yeah, sharing is good.

Maybe adding an abstraction layer object, like:

struct get_next_pc;

struct get_next_pc_ops
{
   void (*read_memory) (struct get_next_pc *self, ...);
   void (*read_register) (struct get_next_pc *self, ...);
   ...
};

struct get_next_pcs
{
   struct get_next_pc_ops *vtable;

   VEC(CORE_ADDR) *result;

   enum bfd_endian byte_order;
   enum bfd_endian byte_order_for_code;
   whatever_type whatever_other_context;
   ...
};

And then both GDB and GDBserver would instantiate
a struct get_next_pc object, like:

struct get_next_pc_ops gdb_get_next_pc_ops = {
   gdb_get_next_pc_read_memory,
   gdb_get_next_pc_read_register,
   ...
}

struct gdb_get_next_pcs
{
  struct get_next_pc base;

  // add whatever other context only gdb needs.
};

int
arm_software_single_step (struct frame_info *frame)
{
  struct gdbarch *gdbarch = get_frame_arch (frame);
  struct gdb_get_next_pc next_pc;
  CORE_ADDR pc;

  next_pc.vtable = gdb_get_next_pc_ops;
  next_pc.byte_order = gdbarch_byte_order (gdbarch);
  next_pc.byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);

  // arm_get_next_pcs is the existing gdb code adjusted to the
  // new interface.
  arm_get_next_pcs (&next_pc);

  // walk result vec (a VEC of CORE_ADDRs) and insert breakpoints.
  // alternatively add a insert_breakpoint callback to struct get_next_pc_ops
  // and insert breakpoints from within arm_get_next_pcs, as currently.
  for (i = 0;
       VEC_iterate (CORE_ADDR, next_pcs.result, i, pc);
       ++i)
    {
       arm_insert_single_step_breakpoint (gdbarch, aspace, pc);
    }

  return 1;
}

Thanks,
Pedro Alves
  

On 05/08/2015 07:50 AM, Pedro Alves wrote:
> On 05/07/2015 12:45 PM, Antoine Tremblay wrote:
>
>> Just fyi, I'm working on doing this at the moment, my investigation is
>> still incomplete...
>>
>> So far I mainly plan to port the arm_get_next code to gdbserver, to
>> accomplish 1. , the code doesn't have so many deps so it should be ok
>> 2. by looking at $cpsr
>> 3. should be fine as 1 and 2 are done...
>>
>> I don't know however yet the best strategy to share the code but I'm
>> guessing I could make the parts that don't have any deps to gdbarch etc
>> in a shared function with gdb/gdbserver... Any pointers on this are
>> welcome...
>
> Yeah, sharing is good.
>
> Maybe adding an abstraction layer object, like:
>
> struct get_next_pc;
>
> struct get_next_pc_ops
> {
>     void (*read_memory) (struct get_next_pc *self, ...);
>     void (*read_register) (struct get_next_pc *self, ...);
>     ...
> };
>
> struct get_next_pcs
> {
>     struct get_next_pc_ops *vtable;
>
>     VEC(CORE_ADDR) *result;
>
>     enum bfd_endian byte_order;
>     enum bfd_endian byte_order_for_code;
>     whatever_type whatever_other_context;
>     ...
> };
>
> And then both GDB and GDBserver would instantiate
> a struct get_next_pc object, like:
>
> struct get_next_pc_ops gdb_get_next_pc_ops = {
>     gdb_get_next_pc_read_memory,
>     gdb_get_next_pc_read_register,
>     ...
> }
>
> struct gdb_get_next_pcs
> {
>    struct get_next_pc base;
>
>    // add whatever other context only gdb needs.
> };
>
> int
> arm_software_single_step (struct frame_info *frame)
> {
>    struct gdbarch *gdbarch = get_frame_arch (frame);
>    struct gdb_get_next_pc next_pc;
>    CORE_ADDR pc;
>
>    next_pc.vtable = gdb_get_next_pc_ops;
>    next_pc.byte_order = gdbarch_byte_order (gdbarch);
>    next_pc.byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);
>
>    // arm_get_next_pcs is the existing gdb code adjusted to the
>    // new interface.
>    arm_get_next_pcs (&next_pc);
>
>    // walk result vec (a VEC of CORE_ADDRs) and insert breakpoints.
>    // alternatively add a insert_breakpoint callback to struct get_next_pc_ops
>    // and insert breakpoints from within arm_get_next_pcs, as currently.
>    for (i = 0;
>         VEC_iterate (CORE_ADDR, next_pcs.result, i, pc);
>         ++i)
>      {
>         arm_insert_single_step_breakpoint (gdbarch, aspace, pc);
>      }
>
>    return 1;
> }
>

This looks very nice thanks! , but I do have one question , why is the 
result a VEC ?

 From the context and current code won't we have only one next instruction ?

Also, if you may,file structure wise, where would be a good place for 
this abstration layer in your view ?

Thanks,
Antoine
  

On 04/30/2015 02:10 PM, Antoine Tremblay wrote:
>
>
> On 04/30/2015 12:23 PM, Yao Qi wrote:
>> From: Yao Qi <yao.qi@linaro.org>
>>    /* Define an ARM-mode breakpoint; we only set breakpoints in the C
>>       library, which is most likely to be ARM.  If the kernel supports
>>       clone events, we will never insert a breakpoint, so even a Thumb
>>       C library will work; so will mixing EABI/non-EABI gdbserver and
>>       application.  */
>> #ifndef __ARM_EABI__
>>    (const unsigned char *) &arm_breakpoint,
>> #else
>>    (const unsigned char *) &arm_eabi_breakpoint,
>> #endif
>>
>> note that the comments are no longer valid as C library can be compiled
>> in thumb mode.
>
> Could we update the comments at the same time ?...
>>
>> When GDBserver steps over a breakpoint in arm mode function, which
>> returns to thumb mode, GDBserver will insert arm mode breakpoint by
>> mistake and the program will crash.  GDBserver alone is unable to
>> determine the arm/thumb mode given a PC address.  See how GDB does
>> it in arm-tdep.c:arm_pc_is_thumb.
>>
>> After thinking about how to teach GDBserver inserting right breakpoint
>> (arm or thumb) for a while, I reconsider it from a different direction
>> that it may be unreasonable to run target-side conditional breakpoint for
>> targets without hardware single step.  Pedro also pointed this out here
>> https://sourceware.org/ml/gdb-patches/2015-04/msg00337.html
>>
>
> I'm looking into teaching gdbserver about inserting the right
> breakpoint and possibly software single-step like this post :
> https://sourceware.org/ml/gdb/2012-10/msg00077.html wanted to do...
>
> It would also fix the problem where we would step-over other breakpoints...
>
> But it does seems very complex to say the least, I'm still early in my
> investigation but if you could share your thoughts on how you came to
> think of it as unreasonable to fix arm-mode and possibly the
> single-stepping it would be appreciated ?

I like the idea of having gdbserver learn how to properly 
software-single-step, allowing us to share the knowledge GDB already has.

Disabling a feature, on the other hand, sound like a backward movement.

People with knowledge on each architecture can probably help fine tune 
those to their needs.
  

On 05/08/2015 01:12 PM, Antoine Tremblay wrote:

> This looks very nice thanks! , but I do have one question , why is the 
> result a VEC ?
> 
>  From the context and current code won't we have only one next instruction ?

Nope.  Most frequent case is conditional branches where we don't know
where the program will end up.  Might be the destination of the branch,
if the instruction evals true, or after the branch, if the condition evals false.
Even though the arm code manages to evaluate most conditions itself upfront,
there are still some cases where it can't.  The way we handle it currently
is that the get_next_pc functions call insert extra single-step breakpoints
themselves, like e.g., in thumb_get_next_pc_raw:

	  else
	    {
	      int cond_negated;

	      /* There are conditional instructions after this one.
		 If this instruction modifies the flags, then we can
		 not predict what the next executed instruction will
		 be.  Fortunately, this instruction is architecturally
		 forbidden to branch; we know it will fall through.
		 Start by skipping past it.  */
	      pc += thumb_insn_size (inst1);
	      itstate = thumb_advance_itstate (itstate);

	      /* Set a breakpoint on the following instruction.  */
	      gdb_assert ((itstate & 0x0f) != 0);
	      arm_insert_single_step_breakpoint (gdbarch, aspace,
						 MAKE_THUMB_ADDR (pc));
	      cond_negated = (itstate >> 4) & 1;


So you see how this is a misleading/surprising interface, naturally
something that grew organically instead of being designed for
multiple potential destinations.

Another case where the ARM code (and others like PPC) need more than
one "next pc" is when dealing with atomic sequences.   See e.g.,
arm_deal_with_atomic_sequence_raw. gdbserver needs all that
atomic sequence code too.

> 
> Also, if you may,file structure wise, where would be a good place for 
> this abstration layer in your view ?

Good question.  Maybe a new gdb/arch/ directory.  But I'd be fine
with putting it in gdb/common/ for now.

Thanks,
Pedro Alves
  

On 05/08/2015 08:29 AM, Pedro Alves wrote:
> On 05/08/2015 01:12 PM, Antoine Tremblay wrote:
>
>> This looks very nice thanks! , but I do have one question , why is the
>> result a VEC ?
>>
>>   From the context and current code won't we have only one next instruction ?
>
> Nope.  Most frequent case is conditional branches where we don't know
> where the program will end up.  Might be the destination of the branch,
> if the instruction evals true, or after the branch, if the condition evals false.
> Even though the arm code manages to evaluate most conditions itself upfront,
> there are still some cases where it can't.  The way we handle it currently
> is that the get_next_pc functions call insert extra single-step breakpoints
> themselves, like e.g., in thumb_get_next_pc_raw:
>
> 	  else
> 	    {
> 	      int cond_negated;
>
> 	      /* There are conditional instructions after this one.
> 		 If this instruction modifies the flags, then we can
> 		 not predict what the next executed instruction will
> 		 be.  Fortunately, this instruction is architecturally
> 		 forbidden to branch; we know it will fall through.
> 		 Start by skipping past it.  */
> 	      pc += thumb_insn_size (inst1);
> 	      itstate = thumb_advance_itstate (itstate);
>
> 	      /* Set a breakpoint on the following instruction.  */
> 	      gdb_assert ((itstate & 0x0f) != 0);
> 	      arm_insert_single_step_breakpoint (gdbarch, aspace,
> 						 MAKE_THUMB_ADDR (pc));
> 	      cond_negated = (itstate >> 4) & 1;
>
>
> So you see how this is a misleading/surprising interface, naturally
> something that grew organically instead of being designed for
> multiple potential destinations.
>

Hooo , right I really though it could evaluate it upfront...seems like I 
had not read the function in enough detail yet :)

> Another case where the ARM code (and others like PPC) need more than
> one "next pc" is when dealing with atomic sequences.   See e.g.,
> arm_deal_with_atomic_sequence_raw. gdbserver needs all that
> atomic sequence code too.
>
Humm ok I will take a look into this too.

>>
>> Also, if you may,file structure wise, where would be a good place for
>> this abstration layer in your view ?
>
> Good question.  Maybe a new gdb/arch/ directory.  But I'd be fine
> with putting it in gdb/common/ for now.
>

Good, thanks a lot for your help, really saved me a quite a few hours!  :)

Regards,
Antoine
  

Luis Machado <lgustavo@codesourcery.com> writes:

> I like the idea of having gdbserver learn how to properly
> software-single-step, allowing us to share the knowledge GDB already
> has.

Yes, I like this idea too, but I am still unsure how complicated
execution control in GDBserver will be after software-single-step is
added in GDBserver.

>
> Disabling a feature, on the other hand, sound like a backward movement.
>

I disable this feature on non-hardware-single-step targets, because
these targets don't know how to do precise software single step.  I
don't know when GDBserver can do software single step, IMO, it's better
to disable this feature.

> People with knowledge on each architecture can probably help fine tune
> those to their needs.

Yes, we can do improvements in this way, however, in this case, the
feature is somewhat broken on some targets, we either fix it or disable it.
  

On Wed, 6 May 2015, Pedro Alves wrote:

> > It is worse if the arm/thumb interworking is considered.  Nowadays,
> > GDBserver arm backend unconditionally inserts arm breakpoint,
> > 
> >   /* Define an ARM-mode breakpoint; we only set breakpoints in the C
> >      library, which is most likely to be ARM.  If the kernel supports
> >      clone events, we will never insert a breakpoint, so even a Thumb
> >      C library will work; so will mixing EABI/non-EABI gdbserver and
> >      application.  */
> > #ifndef __ARM_EABI__
> >   (const unsigned char *) &arm_breakpoint,
> > #else
> >   (const unsigned char *) &arm_eabi_breakpoint,
> > #endif
> > 
> > note that the comments are no longer valid as C library can be compiled
> > in thumb mode.
> > 
> > When GDBserver steps over a breakpoint in arm mode function, which
> > returns to thumb mode, GDBserver will insert arm mode breakpoint by
> > mistake and the program will crash.  GDBserver alone is unable to
> > determine the arm/thumb mode given a PC address.  See how GDB does
> > it in arm-tdep.c:arm_pc_is_thumb.
> 
> Of a random PC address no, but in gdbserver's case, I think that it
> would work, because we need it to step over a breakpoint that is
> at the current PC.  So we could:
> 
>  #1 - Get the mode of the current PC from the thread's $cpsr register.
> 
>  #2 - Get the mode of the next PC by looking at the instruction that is
>       about to be executed (at current PC).  If bx and blx, which change
>       modes, check the thumb bit of the destination address.
>       For all other instructions, same mode as the current PC.

 A similar issue exists for the three MIPS ISA modes and gdbserver will 
not have enough data to determine which of the two of the MIPS16 and 
microMIPS instruction sets to use for the compressed mode.  Only GDB knows 
that, at the last resort having been told by the user.

  Maciej
  

On 05/10/2015 02:04 AM, Maciej W. Rozycki wrote:
> On Wed, 6 May 2015, Pedro Alves wrote:
> 
>>> It is worse if the arm/thumb interworking is considered.  Nowadays,
>>> GDBserver arm backend unconditionally inserts arm breakpoint,
>>>
>>>   /* Define an ARM-mode breakpoint; we only set breakpoints in the C
>>>      library, which is most likely to be ARM.  If the kernel supports
>>>      clone events, we will never insert a breakpoint, so even a Thumb
>>>      C library will work; so will mixing EABI/non-EABI gdbserver and
>>>      application.  */
>>> #ifndef __ARM_EABI__
>>>   (const unsigned char *) &arm_breakpoint,
>>> #else
>>>   (const unsigned char *) &arm_eabi_breakpoint,
>>> #endif
>>>
>>> note that the comments are no longer valid as C library can be compiled
>>> in thumb mode.
>>>
>>> When GDBserver steps over a breakpoint in arm mode function, which
>>> returns to thumb mode, GDBserver will insert arm mode breakpoint by
>>> mistake and the program will crash.  GDBserver alone is unable to
>>> determine the arm/thumb mode given a PC address.  See how GDB does
>>> it in arm-tdep.c:arm_pc_is_thumb.
>>
>> Of a random PC address no, but in gdbserver's case, I think that it
>> would work, because we need it to step over a breakpoint that is
>> at the current PC.  So we could:
>>
>>  #1 - Get the mode of the current PC from the thread's $cpsr register.
>>
>>  #2 - Get the mode of the next PC by looking at the instruction that is
>>       about to be executed (at current PC).  If bx and blx, which change
>>       modes, check the thumb bit of the destination address.
>>       For all other instructions, same mode as the current PC.
> 
>  A similar issue exists for the three MIPS ISA modes and gdbserver will 
> not have enough data to determine which of the two of the MIPS16 and 
> microMIPS instruction sets to use for the compressed mode.  Only GDB knows 
> that, at the last resort having been told by the user.

For breakpoints (z0/z1), GDB tells GDBserver the mode of instruction is
encoded in the breakpoint's size.  The tracepoint creation packets are
older than that and only carry the address.  They'll need to be
extended to include the tracepoint's size as well.  With that,
when stepping past a gdb-set breakpoint/tracepoint, gdbserver can tell
the mode of the instruction under the breakpoint/tracepoint from the
breakpoint/tracepoint's size, as that's information that came from GDB.
I assume that mode switches on MIPS are similar to ARM, with special
branch instruction with mode encoded in in destination address?  If so,
starting from knowing the mode at PC, gdbserver should be able to
determine the mode of all the potential next instructions on its own.

Thanks,
Pedro Alves
  

On Mon, 11 May 2015, Pedro Alves wrote:

> >  A similar issue exists for the three MIPS ISA modes and gdbserver will 
> > not have enough data to determine which of the two of the MIPS16 and 
> > microMIPS instruction sets to use for the compressed mode.  Only GDB knows 
> > that, at the last resort having been told by the user.
> 
> For breakpoints (z0/z1), GDB tells GDBserver the mode of instruction is
> encoded in the breakpoint's size.  The tracepoint creation packets are
> older than that and only carry the address.  They'll need to be
> extended to include the tracepoint's size as well.  With that,
> when stepping past a gdb-set breakpoint/tracepoint, gdbserver can tell
> the mode of the instruction under the breakpoint/tracepoint from the
> breakpoint/tracepoint's size, as that's information that came from GDB.

 Correct, that's not the issue.

> I assume that mode switches on MIPS are similar to ARM, with special
> branch instruction with mode encoded in in destination address?  If so,
> starting from knowing the mode at PC, gdbserver should be able to
> determine the mode of all the potential next instructions on its own.

 And that's where the issue is.  Assuming that you're in the standard ISA 
mode, you can determine that the next instruction will switch the mode to 
one of the compressed ISAs, either by checking the opcode (immediate jump, 
JALX) or by reading the PC to be jumped to (register jumps, JALR and JR).  
What you can't determine is which of the two compressed ISAs, either 
MIPS16 or microMIPS one, the instruction will switch to.

 Given that the MIPS16 and the microMIPS mode cannot be implemented by a 
processor both at a time you can will know which of the two is being used 
once you have seen a breakpoint request for one of them.  However it may 
be that none has been used so far and then you have no way to know, in the 
current state of affairs.

  Maciej
  

On 05/11/2015 01:37 PM, Maciej W. Rozycki wrote:
> On Mon, 11 May 2015, Pedro Alves wrote:

>> I assume that mode switches on MIPS are similar to ARM, with special
>> branch instruction with mode encoded in in destination address?  If so,
>> starting from knowing the mode at PC, gdbserver should be able to
>> determine the mode of all the potential next instructions on its own.
> 
>  And that's where the issue is.  Assuming that you're in the standard ISA 
> mode, you can determine that the next instruction will switch the mode to 
> one of the compressed ISAs, either by checking the opcode (immediate jump, 
> JALX) or by reading the PC to be jumped to (register jumps, JALR and JR).  
> What you can't determine is which of the two compressed ISAs, either 
> MIPS16 or microMIPS one, the instruction will switch to.
> 
>  Given that the MIPS16 and the microMIPS mode cannot be implemented by a 
> processor both at a time you can will know which of the two is being used 
> once you have seen a breakpoint request for one of them.  However it may 
> be that none has been used so far and then you have no way to know, in the 
> current state of affairs.

That sounds solvable though: as that's a static property of the
target system/process, maybe gdbserver can fetch that info from elsewhere,
like somewhere under /proc, or from the auvx, or some bit set in the elf
binary (found at /proc/pid/exe).  Failing that, we can have gdb tell
gdbserver early with some new packet.

Thanks,
Pedro Alves
  

On Mon, 11 May 2015, Pedro Alves wrote:

> >  And that's where the issue is.  Assuming that you're in the standard ISA 
> > mode, you can determine that the next instruction will switch the mode to 
> > one of the compressed ISAs, either by checking the opcode (immediate jump, 
> > JALX) or by reading the PC to be jumped to (register jumps, JALR and JR).  
> > What you can't determine is which of the two compressed ISAs, either 
> > MIPS16 or microMIPS one, the instruction will switch to.
> > 
> >  Given that the MIPS16 and the microMIPS mode cannot be implemented by a 
> > processor both at a time you can will know which of the two is being used 
> > once you have seen a breakpoint request for one of them.  However it may 
> > be that none has been used so far and then you have no way to know, in the 
> > current state of affairs.
> 
> That sounds solvable though: as that's a static property of the
> target system/process, maybe gdbserver can fetch that info from elsewhere,
> like somewhere under /proc, or from the auvx, or some bit set in the elf
> binary (found at /proc/pid/exe).  Failing that, we can have gdb tell
> gdbserver early with some new packet.

 Good point, thanks for the hint!  Gdbserver itself can indeed peek at the 
executable, and binaries that contain microMIPS code will have that noted 
in the ELF header, in `e_flags'.  Conversely, the lack of such annotation 
implies the MIPS16 mode -- if ever requested, that is, as the lack of 
microMIPS annotation does not mean any MIPS16 code is actually present, it 
may in fact be a plain MIPS binary.

 So there's still the corner case of a MIPS16 binary accidentally run on 
microMIPS hardware, as that cannot be detected until a switch to the 
compressed mode has been made.  In which case the debuggee will likely 
crash anyway, so it might not be an interesting case to handle; otherwise 
perhaps the presence of support for the MIPS16 or microMIPS instruction 
set is something worth exporting via AT_HWCAP.  I don't know offhand how 
the two microMIPS software breakpoint instruction encodings used by GDB 
decode in the MIPS16 instruction set.

 Another issue is AFAIK the Linux kernel does not check the microMIPS flag 
in `e_flags' either and will happily attempt to run such a binary on 
MIPS16 or plain MIPS hardware.  But if anything, that's a kernel bug.

  Maciej