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

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

On 09/29/2015 04:32 AM, Yao Qi wrote:
> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
> > 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 ?
>
> How about arm.c?

There's already arm.h in there I think I can actually merge what is in 
there with what I have, and create arm.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 ?
>
> common/ was created in order to share code between GDB and GDBserver, see
> https://sourceware.org/gdb/wiki/Common After that, we find that we'll
> end up with moving most of gdb files into common/ and we'd better move
> different common things into different common directory.  Nowadays, we
> have nat/ common/ and arch/ directories for common things.  arm.c should
> be put in arch/ directory.

Ok I understand thanks for explaining.

> > > > 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.
>
> No, they use it.  This patch moves thumb_insn_size to arm-common.o, but
> thumb_insn_size is used by arm-tdep.c.  If we don't add arm-common.o to
> every target which uses arm-tdep.o, the build will fail on these targets
>
> build/gdb/aarch64/gdb/../../../../binutils-gdb/gdb/arm-tdep.c:846: undefined reference to `thumb_insn_size'
> build/gdb/aarch64/gdb/../../../../binutils-gdb/gdb/arm-tdep.c:846: undefined reference to `thumb_insn_size'
> arm-tdep.o: In function `arm_adjust_breakpoint_address':
> build/gdb/aarch64/gdb/../../../../binutils-gdb/gdb/arm-tdep.c:5417: undefined reference to `thumb_insn_size'
> build/gdb/aarch64/gdb/../../../../binutils-gdb/gdb/arm-tdep.c:5467: undefined reference to `thumb_insn_size'
> arm-tdep.o: In function `arm_breakpoint_from_pc':
> build/gdb/aarch64/gdb/../../../../binutils-gdb/gdb/arm-tdep.c:8858: undefined reference to `thumb_insn_size'

Indeed, thanks

> > > > 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...
>
> Please don't combine movement and refactoring.  This patch is about
> movement, so let us do movement only.  Refactor can be done separately.

Ok , would you find it acceptable still to include the refactoring in 
the series as the next patch or should I create it out of series ?