[ARM] Make thumb2_breakpoint static again
Commit Message
This patch makes thumb2_breakpoint static. When writing this patch,
I find the only reason we keep thumb2_breakpoint extern is that it
is used as an argument passed to arm_gdbserver_get_next_pcs. However,
field arm_thumb2_breakpoint is only used in a null check in
thumb_get_next_pcs_raw, so I wonder why do need to pass thumb2_breakpoint
to arm_gdbserver_get_next_pcs.
thumb2_breakpoint was added by Daniel Jacobowitz in order to support
single-step IT block
https://sourceware.org/ml/gdb-patches/2010-01/msg00624.html the logic
there was if we have 32-bit thumb-2 breakpoint defined, we can safely
single-step IT block, otherwise, we can't. Daniel didn't want to use
16-bit thumb BKPT instruction, because it triggers even on instruction
which should be executed. Secondly, using 16-bit thumb illegal
instruction on top of 32-bit thumb instruction may break the meaning of
original IT blocks, because the other 16-bit can be regarded as an
instruction. See more explanations from Daniel's kernel patch
http://www.spinics.net/lists/arm-kernel/msg80476.html
Let us back to this patch, GDB/GDBserver can safely single step
IT block if thumb2_breakpoint is defined, but the single step logic
doesn't have to know the thumb-2 breakpoint instruction. Only
breakpoint insertion mechanism decides to use which breakpoint
instruction. In the software single step code, instead of pass
thumb2_breakpoint, we can pass a boolean variable
has_thumb2_breakpoint indicate whether the target has thumb-2
breakpoint defined, which is equivalent to the original code.
Regression tested on arm-linux. No regression.
gdb:
2016-01-13 Yao Qi <yao.qi@linaro.org>
* arch/arm-get-next-pcs.c (arm_get_next_pcs_ctor): Change
argument arm_thumb2_breakpoint to has_thumb2_breakpoint.
(thumb_get_next_pcs_raw): Check has_thumb2_breakpoint
instead.
* arch/arm-get-next-pcs.h (struct arm_get_next_pcs)
<arm_thumb2_breakpoint>: Remove.
<has_thumb2_breakpoint>: New field.
(arm_get_next_pcs_ctor): Update declaration.
* arm-linux-tdep.c (arm_linux_software_single_step): Pass
1 to arm_get_next_pcs_ctor.
* arm-tdep.c (arm_software_single_step): Pass 0 to
arm_get_next_pcs_ctor.
gdb/gdbserver:
2016-01-13 Yao Qi <yao.qi@linaro.org>
* linux-aarch32-low.c (thumb2_breakpoint): Make it static.
* linux-aarch32-low.h (thumb2_breakpoint): Remove declaration.
* linux-arm-low.c (arm_gdbserver_get_next_pcs): Pass 1 to
arm_get_next_pcs_ctor.
---
gdb/arch/arm-get-next-pcs.c | 6 +++---
gdb/arch/arm-get-next-pcs.h | 7 ++++---
gdb/arm-linux-tdep.c | 2 +-
gdb/arm-tdep.c | 2 +-
gdb/gdbserver/linux-aarch32-low.c | 2 +-
gdb/gdbserver/linux-aarch32-low.h | 2 --
gdb/gdbserver/linux-arm-low.c | 2 +-
7 files changed, 11 insertions(+), 12 deletions(-)
Comments
On 01/13/2016 03:45 AM, Yao Qi wrote:
> This patch makes thumb2_breakpoint static. When writing this patch,
> I find the only reason we keep thumb2_breakpoint extern is that it
> is used as an argument passed to arm_gdbserver_get_next_pcs. However,
> field arm_thumb2_breakpoint is only used in a null check in
> thumb_get_next_pcs_raw, so I wonder why do need to pass thumb2_breakpoint
> to arm_gdbserver_get_next_pcs.
>
> thumb2_breakpoint was added by Daniel Jacobowitz in order to support
> single-step IT block
> https://sourceware.org/ml/gdb-patches/2010-01/msg00624.html the logic
> there was if we have 32-bit thumb-2 breakpoint defined, we can safely
> single-step IT block, otherwise, we can't. Daniel didn't want to use
> 16-bit thumb BKPT instruction, because it triggers even on instruction
> which should be executed. Secondly, using 16-bit thumb illegal
> instruction on top of 32-bit thumb instruction may break the meaning of
> original IT blocks, because the other 16-bit can be regarded as an
> instruction. See more explanations from Daniel's kernel patch
> http://www.spinics.net/lists/arm-kernel/msg80476.html
>
> Let us back to this patch, GDB/GDBserver can safely single step
> IT block if thumb2_breakpoint is defined, but the single step logic
> doesn't have to know the thumb-2 breakpoint instruction. Only
> breakpoint insertion mechanism decides to use which breakpoint
> instruction. In the software single step code, instead of pass
> thumb2_breakpoint, we can pass a boolean variable
> has_thumb2_breakpoint indicate whether the target has thumb-2
> breakpoint defined, which is equivalent to the original code.
>
> Regression tested on arm-linux. No regression.
>
> gdb:
>
> 2016-01-13 Yao Qi <yao.qi@linaro.org>
>
> * arch/arm-get-next-pcs.c (arm_get_next_pcs_ctor): Change
> argument arm_thumb2_breakpoint to has_thumb2_breakpoint.
> (thumb_get_next_pcs_raw): Check has_thumb2_breakpoint
> instead.
> * arch/arm-get-next-pcs.h (struct arm_get_next_pcs)
> <arm_thumb2_breakpoint>: Remove.
> <has_thumb2_breakpoint>: New field.
> (arm_get_next_pcs_ctor): Update declaration.
> * arm-linux-tdep.c (arm_linux_software_single_step): Pass
> 1 to arm_get_next_pcs_ctor.
> * arm-tdep.c (arm_software_single_step): Pass 0 to
> arm_get_next_pcs_ctor.
>
> gdb/gdbserver:
>
> 2016-01-13 Yao Qi <yao.qi@linaro.org>
>
> * linux-aarch32-low.c (thumb2_breakpoint): Make it static.
> * linux-aarch32-low.h (thumb2_breakpoint): Remove declaration.
> * linux-arm-low.c (arm_gdbserver_get_next_pcs): Pass 1 to
> arm_get_next_pcs_ctor.
> ---
> gdb/arch/arm-get-next-pcs.c | 6 +++---
> gdb/arch/arm-get-next-pcs.h | 7 ++++---
> gdb/arm-linux-tdep.c | 2 +-
> gdb/arm-tdep.c | 2 +-
> gdb/gdbserver/linux-aarch32-low.c | 2 +-
> gdb/gdbserver/linux-aarch32-low.h | 2 --
> gdb/gdbserver/linux-arm-low.c | 2 +-
> 7 files changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/gdb/arch/arm-get-next-pcs.c b/gdb/arch/arm-get-next-pcs.c
> index aba45e3..5b25d58 100644
> --- a/gdb/arch/arm-get-next-pcs.c
> +++ b/gdb/arch/arm-get-next-pcs.c
> @@ -30,13 +30,13 @@ arm_get_next_pcs_ctor (struct arm_get_next_pcs *self,
> struct arm_get_next_pcs_ops *ops,
> int byte_order,
> int byte_order_for_code,
> - const gdb_byte *arm_thumb2_breakpoint,
> + int has_thumb2_breakpoint,
> struct regcache *regcache)
> {
> self->ops = ops;
> self->byte_order = byte_order;
> self->byte_order_for_code = byte_order_for_code;
> - self->arm_thumb2_breakpoint = arm_thumb2_breakpoint;
> + self->has_thumb2_breakpoint = has_thumb2_breakpoint;
> self->regcache = regcache;
> }
>
> @@ -297,7 +297,7 @@ thumb_get_next_pcs_raw (struct arm_get_next_pcs *self, CORE_ADDR pc)
> flags, affecting the execution of further instructions, we may
> need to set two breakpoints. */
>
> - if (self->arm_thumb2_breakpoint != NULL)
> + if (self->has_thumb2_breakpoint)
> {
> if ((inst1 & 0xff00) == 0xbf00 && (inst1 & 0x000f) != 0)
> {
> diff --git a/gdb/arch/arm-get-next-pcs.h b/gdb/arch/arm-get-next-pcs.h
> index 895e866..4a0fc16 100644
> --- a/gdb/arch/arm-get-next-pcs.h
> +++ b/gdb/arch/arm-get-next-pcs.h
> @@ -41,8 +41,9 @@ struct arm_get_next_pcs
> int byte_order;
> /* Byte order for code. */
> int byte_order_for_code;
> - /* Thumb2 breakpoint instruction. */
> - const gdb_byte *arm_thumb2_breakpoint;
> + /* Whether the target has 32-bit thumb-2 breakpoint defined or
> + not. */
> + int has_thumb2_breakpoint;
> /* Registry cache. */
> struct regcache *regcache;
> };
> @@ -52,7 +53,7 @@ void arm_get_next_pcs_ctor (struct arm_get_next_pcs *self,
> struct arm_get_next_pcs_ops *ops,
> int byte_order,
> int byte_order_for_code,
> - const gdb_byte *arm_thumb2_breakpoint,
> + int has_thumb2_breakpoint,
> struct regcache *regcache);
>
> /* Find the next possible PCs after the current instruction executes. */
> diff --git a/gdb/arm-linux-tdep.c b/gdb/arm-linux-tdep.c
> index 2870bd1..6050bdf 100644
> --- a/gdb/arm-linux-tdep.c
> +++ b/gdb/arm-linux-tdep.c
> @@ -931,7 +931,7 @@ arm_linux_software_single_step (struct frame_info *frame)
> &arm_linux_get_next_pcs_ops,
> gdbarch_byte_order (gdbarch),
> gdbarch_byte_order_for_code (gdbarch),
> - gdbarch_tdep (gdbarch)->thumb2_breakpoint,
> + 1,
> regcache);
>
> next_pcs = arm_get_next_pcs (&next_pcs_ctx, regcache_read_pc (regcache));
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 05d60bb..8874ec8 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -6183,7 +6183,7 @@ arm_software_single_step (struct frame_info *frame)
> &arm_get_next_pcs_ops,
> gdbarch_byte_order (gdbarch),
> gdbarch_byte_order_for_code (gdbarch),
> - gdbarch_tdep (gdbarch)->thumb2_breakpoint,
> + 0,
> regcache);
>
> next_pcs = arm_get_next_pcs (&next_pcs_ctx, regcache_read_pc (regcache));
> diff --git a/gdb/gdbserver/linux-aarch32-low.c b/gdb/gdbserver/linux-aarch32-low.c
> index ed66e08..2bbbb24 100644
> --- a/gdb/gdbserver/linux-aarch32-low.c
> +++ b/gdb/gdbserver/linux-aarch32-low.c
> @@ -45,7 +45,7 @@ static const unsigned long arm_breakpoint = arm_abi_breakpoint;
> #define arm_breakpoint_len 4
> static const unsigned short thumb_breakpoint = 0xde01;
> #define thumb_breakpoint_len 2
> -const unsigned short thumb2_breakpoint[] = { 0xf7f0, 0xa000 };
> +static const unsigned short thumb2_breakpoint[] = { 0xf7f0, 0xa000 };
> #define thumb2_breakpoint_len 4
>
> /* Some older versions of GNU/Linux and Android do not define
> diff --git a/gdb/gdbserver/linux-aarch32-low.h b/gdb/gdbserver/linux-aarch32-low.h
> index 5c68454..434a523 100644
> --- a/gdb/gdbserver/linux-aarch32-low.h
> +++ b/gdb/gdbserver/linux-aarch32-low.h
> @@ -15,8 +15,6 @@
> You should have received a copy of the GNU General Public License
> along with this program. If not, see <http://www.gnu.org/licenses/>. */
>
> -extern const unsigned short thumb2_breakpoint[];
> -
> extern struct regs_info regs_info_aarch32;
>
> void arm_fill_gregset (struct regcache *regcache, void *buf);
> diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
> index d967e58..927a6fa 100644
> --- a/gdb/gdbserver/linux-arm-low.c
> +++ b/gdb/gdbserver/linux-arm-low.c
> @@ -942,7 +942,7 @@ arm_gdbserver_get_next_pcs (CORE_ADDR pc, struct regcache *regcache)
> /* Byte order is ignored assumed as host. */
> 0,
> 0,
> - (const gdb_byte *) &thumb2_breakpoint,
> + 1,
> regcache);
>
> next_pcs = arm_get_next_pcs (&next_pcs_ctx, pc);
LGTM.
Thanks,
Antoine
Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>> gdb:
>>
>> 2016-01-13 Yao Qi <yao.qi@linaro.org>
>>
>> * arch/arm-get-next-pcs.c (arm_get_next_pcs_ctor): Change
>> argument arm_thumb2_breakpoint to has_thumb2_breakpoint.
>> (thumb_get_next_pcs_raw): Check has_thumb2_breakpoint
>> instead.
>> * arch/arm-get-next-pcs.h (struct arm_get_next_pcs)
>> <arm_thumb2_breakpoint>: Remove.
>> <has_thumb2_breakpoint>: New field.
>> (arm_get_next_pcs_ctor): Update declaration.
>> * arm-linux-tdep.c (arm_linux_software_single_step): Pass
>> 1 to arm_get_next_pcs_ctor.
>> * arm-tdep.c (arm_software_single_step): Pass 0 to
>> arm_get_next_pcs_ctor.
>>
>> gdb/gdbserver:
>>
>> 2016-01-13 Yao Qi <yao.qi@linaro.org>
>>
>> * linux-aarch32-low.c (thumb2_breakpoint): Make it static.
>> * linux-aarch32-low.h (thumb2_breakpoint): Remove declaration.
>> * linux-arm-low.c (arm_gdbserver_get_next_pcs): Pass 1 to
>> arm_get_next_pcs_ctor.
>
> LGTM.
Patch is pushed in.
@@ -30,13 +30,13 @@ arm_get_next_pcs_ctor (struct arm_get_next_pcs *self,
struct arm_get_next_pcs_ops *ops,
int byte_order,
int byte_order_for_code,
- const gdb_byte *arm_thumb2_breakpoint,
+ int has_thumb2_breakpoint,
struct regcache *regcache)
{
self->ops = ops;
self->byte_order = byte_order;
self->byte_order_for_code = byte_order_for_code;
- self->arm_thumb2_breakpoint = arm_thumb2_breakpoint;
+ self->has_thumb2_breakpoint = has_thumb2_breakpoint;
self->regcache = regcache;
}
@@ -297,7 +297,7 @@ thumb_get_next_pcs_raw (struct arm_get_next_pcs *self, CORE_ADDR pc)
flags, affecting the execution of further instructions, we may
need to set two breakpoints. */
- if (self->arm_thumb2_breakpoint != NULL)
+ if (self->has_thumb2_breakpoint)
{
if ((inst1 & 0xff00) == 0xbf00 && (inst1 & 0x000f) != 0)
{
@@ -41,8 +41,9 @@ struct arm_get_next_pcs
int byte_order;
/* Byte order for code. */
int byte_order_for_code;
- /* Thumb2 breakpoint instruction. */
- const gdb_byte *arm_thumb2_breakpoint;
+ /* Whether the target has 32-bit thumb-2 breakpoint defined or
+ not. */
+ int has_thumb2_breakpoint;
/* Registry cache. */
struct regcache *regcache;
};
@@ -52,7 +53,7 @@ void arm_get_next_pcs_ctor (struct arm_get_next_pcs *self,
struct arm_get_next_pcs_ops *ops,
int byte_order,
int byte_order_for_code,
- const gdb_byte *arm_thumb2_breakpoint,
+ int has_thumb2_breakpoint,
struct regcache *regcache);
/* Find the next possible PCs after the current instruction executes. */
@@ -931,7 +931,7 @@ arm_linux_software_single_step (struct frame_info *frame)
&arm_linux_get_next_pcs_ops,
gdbarch_byte_order (gdbarch),
gdbarch_byte_order_for_code (gdbarch),
- gdbarch_tdep (gdbarch)->thumb2_breakpoint,
+ 1,
regcache);
next_pcs = arm_get_next_pcs (&next_pcs_ctx, regcache_read_pc (regcache));
@@ -6183,7 +6183,7 @@ arm_software_single_step (struct frame_info *frame)
&arm_get_next_pcs_ops,
gdbarch_byte_order (gdbarch),
gdbarch_byte_order_for_code (gdbarch),
- gdbarch_tdep (gdbarch)->thumb2_breakpoint,
+ 0,
regcache);
next_pcs = arm_get_next_pcs (&next_pcs_ctx, regcache_read_pc (regcache));
@@ -45,7 +45,7 @@ static const unsigned long arm_breakpoint = arm_abi_breakpoint;
#define arm_breakpoint_len 4
static const unsigned short thumb_breakpoint = 0xde01;
#define thumb_breakpoint_len 2
-const unsigned short thumb2_breakpoint[] = { 0xf7f0, 0xa000 };
+static const unsigned short thumb2_breakpoint[] = { 0xf7f0, 0xa000 };
#define thumb2_breakpoint_len 4
/* Some older versions of GNU/Linux and Android do not define
@@ -15,8 +15,6 @@
You should have received a copy of the GNU General Public License
along with this program. If not, see <http://www.gnu.org/licenses/>. */
-extern const unsigned short thumb2_breakpoint[];
-
extern struct regs_info regs_info_aarch32;
void arm_fill_gregset (struct regcache *regcache, void *buf);
@@ -942,7 +942,7 @@ arm_gdbserver_get_next_pcs (CORE_ADDR pc, struct regcache *regcache)
/* Byte order is ignored assumed as host. */
0,
0,
- (const gdb_byte *) &thumb2_breakpoint,
+ 1,
regcache);
next_pcs = arm_get_next_pcs (&next_pcs_ctx, pc);