[3/5] Add support for ARM breakpoint types in GDBServer.
Commit Message
This patch is in preparation for software breakpoints on ARM in
GDBServer.
ARM can have multiple breakpoint types based on the instruction set
it's currently in: arm, thumb or thumb2.
GDBServer needs to know what breakpoint is to be inserted at location
when inserting a breakpoint.
This is handled by the breakpoint_from_pc target ops introduced in a
previous patch, this patch adds the arm_breakpoint_from_pc
implementation so that the proper breakpoint type is returned based on
the current pc and register values.
Also in order to share some code with GDB new files called
arm-common.h/c have been introduced in common/.
While these files do not contain much yet future patches will add more
to them thus the inclusion at this stage.
No regressions on Ubuntu 14.04 on ARMv7 and x86.
With gdbserver-{native,extended} / { -marm -mthumb }
gdb/ChangeLog:
* Makefile.in: Add arm-common.o.
* arm-tdep.c (thumb_insn_size): Move to arm-common.h/c.
* common/arm-common.c: New file.
* common/arm-common.h: New file.
* configure.tgt: Add arm-common.o.
* gdbserver/Makefile.in: Add arm-common.c/o.
* 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.
---
gdb/Makefile.in | 8 +++-
gdb/arm-tdep.c | 21 +---------
gdb/common/arm-common.c | 32 ++++++++++++++
gdb/common/arm-common.h | 33 +++++++++++++++
gdb/configure.tgt | 2 +-
gdb/gdbserver/Makefile.in | 6 ++-
gdb/gdbserver/configure.srv | 1 +
gdb/gdbserver/linux-arm-low.c | 98 +++++++++++++++++++++++++++++++++----------
8 files changed, 156 insertions(+), 45 deletions(-)
create mode 100644 gdb/common/arm-common.c
create mode 100644 gdb/common/arm-common.h
Comments
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.
> * 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?
Please move this file to arch/ directory rather than common/
> * configure.tgt: Add arm-common.o.
> * gdbserver/Makefile.in: Add arm-common.c/o.
ChangeLog entries for GDBserver should be written separately.
> * 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.
> 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.
> 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.
> 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?
>
> 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.
> +}
> +
> +/* 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?
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.
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?
>> 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.
>>> 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'
>>
>>> 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.
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 ?
Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
> 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 ?
Either is OK to me. My point is don't combine movement and refactoring
in a single patch.
@@ -657,7 +657,7 @@ ALL_64_TARGET_OBS = \
# All other target-dependent objects files (used with --enable-targets=all).
ALL_TARGET_OBS = \
- armbsd-tdep.o arm-linux-tdep.o arm-symbian-tdep.o \
+ armbsd-tdep.o arm-common.o arm-linux-tdep.o arm-symbian-tdep.o \
armnbsd-tdep.o armobsd-tdep.o \
arm-tdep.o arm-wince-tdep.o \
avr-tdep.o \
@@ -1660,6 +1660,7 @@ ALLDEPFILES = \
amd64-dicos-tdep.c \
amd64-linux-nat.c amd64-linux-tdep.c \
amd64-sol2-tdep.c \
+ arm-common.c \
arm-linux-nat.c arm-linux-tdep.c arm-symbian-tdep.c arm-tdep.c \
armnbsd-nat.c armbsd-tdep.c armnbsd-tdep.c armobsd-tdep.c \
avr-tdep.c \
@@ -2265,6 +2266,11 @@ btrace-common.o: ${srcdir}/common/btrace-common.c
fileio.o: ${srcdir}/common/fileio.c
$(COMPILE) $(srcdir)/common/fileio.c
$(POSTCOMPILE)
+
+arm-common.o: ${srcdir}/common/arm-common.c
+ $(COMPILE) $(srcdir)/common/arm-common.c
+ $(POSTCOMPILE)
+
#
# gdb/target/ dependencies
#
@@ -45,6 +45,7 @@
#include "user-regs.h"
#include "observer.h"
+#include "common/arm-common.h"
#include "arm-tdep.h"
#include "gdb/sim-arm.h"
@@ -235,8 +236,6 @@ static void arm_neon_quad_write (struct gdbarch *gdbarch,
struct regcache *regcache,
int regnum, const gdb_byte *buf);
-static int thumb_insn_size (unsigned short inst1);
-
struct arm_prologue_cache
{
/* The stack pointer at the time this frame was created; i.e. the
@@ -267,12 +266,6 @@ static CORE_ADDR arm_analyze_prologue (struct gdbarch *gdbarch,
#define DISPLACED_STEPPING_ARCH_VERSION 5
-/* Addresses for calling Thumb functions have the bit 0 set.
- Here are some macros to test, set, or clear bit 0 of addresses. */
-#define IS_THUMB_ADDR(addr) ((addr) & 1)
-#define MAKE_THUMB_ADDR(addr) ((addr) | 1)
-#define UNMAKE_THUMB_ADDR(addr) ((addr) & ~1)
-
/* Set to true if the 32-bit mode is in use. */
int arm_apcs_32 = 1;
@@ -4361,18 +4354,6 @@ bitcount (unsigned long val)
return nbits;
}
-/* Return the size in bytes of the complete Thumb instruction whose
- first halfword is INST1. */
-
-static int
-thumb_insn_size (unsigned short inst1)
-{
- if ((inst1 & 0xe000) == 0xe000 && (inst1 & 0x1800) != 0)
- return 4;
- else
- return 2;
-}
-
static int
thumb_advance_itstate (unsigned int itstate)
{
new file mode 100644
@@ -0,0 +1,32 @@
+/* Common code for generic ARM support.
+
+ Copyright (C) 2015 Free Software Foundation, Inc.
+
+ This file is part of GDB.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#include "arm-common.h"
+
+/* Return the size in bytes of the complete Thumb instruction whose
+ first halfword is INST1. */
+
+int
+thumb_insn_size (unsigned short inst1)
+{
+ if ((inst1 & 0xe000) == 0xe000 && (inst1 & 0x1800) != 0)
+ return 4;
+ else
+ return 2;
+}
new file mode 100644
@@ -0,0 +1,33 @@
+/* Common code for generic ARM support.
+
+ Copyright (C) 2015 Free Software Foundation, Inc.
+
+ This file is part of GDB.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#ifndef ARM_COMMON_H
+#define ARM_COMMON_H 1
+
+/* Addresses for calling Thumb functions have the bit 0 set.
+ Here are some macros to test, set, or clear bit 0 of addresses. */
+#define IS_THUMB_ADDR(addr) ((addr) & 1)
+#define MAKE_THUMB_ADDR(addr) ((addr) | 1)
+#define UNMAKE_THUMB_ADDR(addr) ((addr) & ~1)
+
+/* Return the size in bytes of the complete Thumb instruction whose
+ first halfword is INST1. */
+int thumb_insn_size (unsigned short inst1);
+
+#endif /* ARM_COMMON_H */
@@ -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
;;
@@ -180,7 +180,8 @@ SFILES= $(srcdir)/gdbreplay.c $(srcdir)/inferiors.c $(srcdir)/dll.c \
$(srcdir)/common/common-debug.c $(srcdir)/common/cleanups.c \
$(srcdir)/common/common-exceptions.c $(srcdir)/symbol.c \
$(srcdir)/common/btrace-common.c \
- $(srcdir)/common/fileio.c $(srcdir)/nat/linux-namespaces.c
+ $(srcdir)/common/fileio.c $(srcdir)/nat/linux-namespaces.c \
+ $(srcdir)/common/arm-common.c
DEPFILES = @GDBSERVER_DEPFILES@
@@ -576,6 +577,9 @@ waitstatus.o: ../target/waitstatus.c
fileio.o: ../common/fileio.c
$(COMPILE) $<
$(POSTCOMPILE)
+arm-common.o: ../common/arm-common.c
+ $(COMPILE) $<
+ $(POSTCOMPILE)
# Native object files rules from ../nat
@@ -68,6 +68,7 @@ case "${target}" in
srv_regobj="${srv_regobj} arm-with-neon.o"
srv_tgtobj="$srv_linux_obj linux-arm-low.o"
srv_tgtobj="$srv_tgtobj linux-aarch32-low.o"
+ srv_tgtobj="${srv_tgtobj} arm-common.o"
srv_xmlfiles="arm-with-iwmmxt.xml"
srv_xmlfiles="${srv_xmlfiles} arm-with-vfpv2.xml"
srv_xmlfiles="${srv_xmlfiles} arm-with-vfpv3.xml"
@@ -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
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;
+}
+
+/* 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)
+ {
+ *lenptr = arm_breakpoint_len;
+ return (unsigned char *) &arm_breakpoint;
+ }
+
+ if (IS_THUMB_ADDR (*pcptr))
+ {
+ gdb_byte buf[2];
+
+ *pcptr = UNMAKE_THUMB_ADDR (*pcptr);
+
+ /* Check whether we are replacing a thumb2 32-bit instruction. */
+ if ((*the_target->read_memory) (*pcptr, buf, 2) == 0)
+ {
+ unsigned short inst1;
+
+ (*the_target->read_memory) (*pcptr, (gdb_byte *) &inst1, 2);
+ if (thumb_insn_size (inst1) == 4)
+ {
+ *lenptr = thumb2_breakpoint_len;
+ return (unsigned char *) &thumb2_breakpoint;
+ }
+ }
+
+ *lenptr = thumb_breakpoint_len;
+ return (unsigned char *) &thumb_breakpoint;
+ }
+ else
+ {
+ *lenptr = arm_breakpoint_len;
+ return (unsigned char *) &arm_breakpoint;
+ }
+}
+
/* 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. */
@@ -913,21 +982,6 @@ arm_regs_info (void)
return ®s_info_arm;
}
-static const unsigned char *
-arm_breakpoint_from_pc (CORE_ADDR *pcptr, int *len)
-{
- *len = arm_breakpoint_len;
- /* 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__
- return (const unsigned char *) &arm_breakpoint;
-#else
- return (const unsigned char *) &arm_eabi_breakpoint;
-#endif
-}
struct linux_target_ops the_low_target = {
arm_arch_setup,
arm_regs_info,