Re: [PATCH 3/5] Add support for ARM breakpoint types in GDBServer.

[Antoine Tremblay <antoine dot tremblay at ericsson dot com>]

On 09/28/2015 06:29 AM, Yao Qi wrote:
> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
> > gdb/ChangeLog:
> > 	* Makefile.in: Add arm-common.o.
> > 	* arm-tdep.c (thumb_insn_size): Move to arm-common.h/c.
>
> "Move to arm-common.c".  Some macros are moved, we should mention the
> move here as well.

Done.

> > 	* common/arm-common.c: New file.
> > 	* common/arm-common.h: New file.
>
> arm-common.c is not a good file name to me, how about arm-insn.c?

This will contain more stuff as I post the next patch sets for single 
stepping etc.. I would like to keep a more general name.

It's basically all coming from arm-tdep.c but I don't want to name it 
common/arm-tdep.c to avoid confusion and Makefile problems.

arm-ctdep.c ?

> Please move this file to arch/ directory rather than common/

It seems weird to me to have common objects somewhere else then in 
common/ , if common becomes more a lib we don't want it all over the 
place no ?

Would creating a common/arch be acceptable ? I guess we would have to 
move things like x86-xstate.h etc.. there too ?

> > 	* configure.tgt: Add arm-common.o.
> > 	* gdbserver/Makefile.in: Add arm-common.c/o.
>
> ChangeLog entries for GDBserver should be written separately.

Oops my bad sorry about that.

> > 	* gdbserver/configure.srv: Add arm-common.o.
> > 	* gdbserver/linux-arm-low.c (arm_breakpoint_at): Adjust for
> >          breakpoint types.
> > 	(arm_breakpoint_from_pc): New function.
>
> arm_breakpoint_from_pc is not a new function, but arm_is_thumb_mode is.

Done.

> > diff --git a/gdb/common/arm-common.c b/gdb/common/arm-common.c
> > new file mode 100644
> > index 0000000..861b249
> > --- /dev/null
> > +++ b/gdb/common/arm-common.c
> > @@ -0,0 +1,32 @@
> > +/* Common code for generic ARM support.
> > +
> > +   Copyright (C) 2015 Free Software Foundation, Inc.
> > +
>
> Contents in file are moved from existing file, so we should still keep
> the year range of the original file.  It should be
>
>       Copyright (C) 1988-2015 Free Software Foundation, Inc.
OK np

> > diff --git a/gdb/configure.tgt b/gdb/configure.tgt
> > index c42b4df..e831f59 100644
> > --- a/gdb/configure.tgt
> > +++ b/gdb/configure.tgt
> > @@ -89,7 +89,7 @@ arm*-wince-pe | arm*-*-mingw32ce*)
> >   	;;
> >   arm*-*-linux*)
> >   	# Target: ARM based machine running GNU/Linux
> > -	gdb_target_obs="arm-tdep.o arm-linux-tdep.o glibc-tdep.o \
> > +	gdb_target_obs="arm-common.o arm-tdep.o arm-linux-tdep.o glibc-tdep.o \
> >   			solib-svr4.o symfile-mem.o linux-tdep.o linux-record.o"
> >   	build_gdbserver=yes
>
> since arm-common.o is moved out of arm-tdep.o, so it should be added to
> every target which uses arm-tdep.o, such as aarch64*-*-linux*,
> arm*-wince-pe, etc.

Even if they don't use it ? But ok.

> > diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
> > index 367c704..15ecb70 100644
> > --- a/gdb/gdbserver/linux-arm-low.c
> > +++ b/gdb/gdbserver/linux-arm-low.c
> > @@ -30,6 +30,8 @@
> >   #include "nat/gdb_ptrace.h"
> >   #include <signal.h>
> >
> > +#include "common/arm-common.h"
> > +
> >   /* Defined in auto-generated files.  */
> >   void init_registers_arm (void);
> >   extern const struct target_desc *tdesc_arm;
> > @@ -234,19 +236,28 @@ arm_set_pc (struct regcache *regcache, CORE_ADDR pc)
> >   }
> >
> >   /* Correct in either endianness.  */
> > -static const unsigned long arm_breakpoint = 0xef9f0001;
> > -#define arm_breakpoint_len 4
> > -static const unsigned short thumb_breakpoint = 0xde01;
> > -static const unsigned short thumb2_breakpoint[] = { 0xf7f0, 0xa000 };
> > +#define arm_abi_breakpoint 0xef9f0001UL
> >
> >   /* For new EABI binaries.  We recognize it regardless of which ABI
> >      is used for gdbserver, so single threaded debugging should work
> >      OK, but for multi-threaded debugging we only insert the current
> >      ABI's breakpoint instruction.  For now at least.  */
> > -static const unsigned long arm_eabi_breakpoint = 0xe7f001f0;
> > +#define arm_eabi_breakpoint 0xe7f001f0UL
> > +
> > +#ifndef __ARM_EABI__
> > +static const unsigned long arm_breakpoint = arm_abi_breakpoint;
> > +#else
> > +static const unsigned long arm_breakpoint = arm_eabi_breakpoint;
> > +#endif
> > +
> > +#define arm_breakpoint_len 4
> > +static const unsigned short thumb_breakpoint = 0xde01;
> > +#define thumb_breakpoint_len 2
> > +static const unsigned short thumb2_breakpoint[] = { 0xf7f0, 0xa000 };
> > +#define thumb2_breakpoint_len 4
>
> I am confused by your changes here.  Why do you change them?

Before arm_breakpoint_from_pc would be :

#ifndef __ARM_EABI__
  return (const unsigned char *) &arm_breakpoint;
#else
-  return (const unsigned char *) &arm_eabi_breakpoint;
#endif

And arm_breakpoint_at would check for the arm_breakpoint || 
arm_eabi_breakpoint.

Thus the selected arm_breakpoint would be what that function returned 
and arm_breakpoint was arm_abi_breakpoint.

It felt more clear to me to name those for what they are : 2 separate 
entities arm_abi_breakpoint and arm_eabi_breakpoint and set the current 
used one as arm_breakpoint.

This allows a cleaner breakpoint_from_pc as it just returns 
arm_breakpoint rather than having the #ifdef in that function.

Any other reference to the arm_breakpoint would also be clear of #ifdefs...


> >   static int
> > -arm_breakpoint_at (CORE_ADDR where)
> > +arm_is_thumb_mode (void)
> >   {
> >     struct regcache *regcache = get_thread_regcache (current_thread, 1);
> >     unsigned long cpsr;
> > @@ -254,6 +265,17 @@ arm_breakpoint_at (CORE_ADDR where)
> >     collect_register_by_name (regcache, "cpsr", &cpsr);
> >
> >     if (cpsr & 0x20)
> > +      return 1;
> > +  else
> > +      return 0;
>
> Indentation looks odd.

Done.

> > +}
> > +
> > +/* Returns 1 if there is a software breakpoint at location.  */
> > +
> > +static int
> > +arm_breakpoint_at (CORE_ADDR where)
> > +{
> > +  if (arm_is_thumb_mode ())
> >       {
> >         /* Thumb mode.  */
> >         unsigned short insn;
> > @@ -275,7 +297,7 @@ arm_breakpoint_at (CORE_ADDR where)
> >         unsigned long insn;
> >
> >         (*the_target->read_memory) (where, (unsigned char *) &insn, 4);
> > -      if (insn == arm_breakpoint)
> > +      if (insn == arm_abi_breakpoint)
> >   	return 1;
> >
> >         if (insn == arm_eabi_breakpoint)
> > @@ -285,6 +307,53 @@ arm_breakpoint_at (CORE_ADDR where)
> >     return 0;
> >   }
> >
> > +/* Determine the type and size of breakpoint to insert at PCPTR.  Uses
> > +   the program counter value to determine whether a 16-bit or 32-bit
> > +   breakpoint should be used.  It returns a pointer to a string of
> > +   bytes that encode a breakpoint instruction, stores the length of
> > +   the string to *lenptr, and adjusts the program counter (if
> > +   necessary) to point to the actual memory location where the
> > +   breakpoint should be inserted.  */
> > +
> > +static const unsigned char *
> > +arm_breakpoint_from_pc (CORE_ADDR *pcptr, int *lenptr)
> > +{
> > +  /* Default if no pc is set to arm breakpoint.  */
> > +  if (pcptr == NULL)
>
> Such NULL checking is no longer needed, right?

Indeed this is gone based on comments in patch 1/5.