Re: [PATCH 2/2] arm-tdep.c: Refactor displaced stepping relocation functions

Luis Machado
2016-02-26T16:11:58Z

On 02/25/2016 12:47 PM, Simon Marchi wrote:
> A small refactor so that arm_process_displaced_insn is the only function
> specific to GDB. All functions called from this one will eventually be
> moved to common/, so they need to be free of anything GDB-specific. I
> also renamed those functions from "process_displaced" to "relocate",
> since they won't be used exclusively for displaced stepping anymore.
>
> The call tree ends up like this:
>
> - arm_process_displaced_insn
> - arm_relocate_insn_arm
> ...
> - arm_relocate_insn_thumb_32bit
> ...
> - arm_relocate_insn_thumb_16bit
> ...
>
> gdb/ChangeLog:
>
> * arm-tdep.c (thumb_process_displaced_16bit_insn): Rename to...
> (arm_relocate_insn_thumb_16bit): ... this, and add return error
> code.
> (thumb_process_displaced_32bit_insn): Rename to...
> (arm_relocate_insn_thumb_32bit): ... this, and add return error
> code.
> (thumb_process_displaced_insn): Remove.
> (arm_relocate_insn_arm): New function, extracted mostly from...
> (arm_process_displaced_insn): ... this. Refactor to adapt to
> other functions changes.
> ---
> gdb/arm-tdep.c | 122 ++++++++++++++++++++++++++++++---------------------------
> 1 file changed, 64 insertions(+), 58 deletions(-)
>
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 43b61c2..ef48a90 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -7111,9 +7111,8 @@ thumb_copy_pop_pc_16bit (uint16_t insn1, struct arm_insn_reloc_data *data)
> return 0;
> }
>
> -static void
> -thumb_process_displaced_16bit_insn (uint16_t insn1,
> - struct arm_insn_reloc_data *data)
> +static int
> +arm_relocate_insn_thumb_16bit (uint16_t insn1, struct arm_insn_reloc_data *data)
> {
> unsigned short op_bit_12_15 = bits (insn1, 12, 15);
> unsigned short op_bit_10_11 = bits (insn1, 10, 11);
> @@ -7202,9 +7201,7 @@ thumb_process_displaced_16bit_insn (uint16_t insn1,
> err = 1;
> }
>
> - if (err)
> - internal_error (__FILE__, __LINE__,
> - _("thumb_process_displaced_16bit_insn: Instruction decode error"));
> + return err;

Should we keep this internal error message under a different context
instead of exporting just an error code? Maybe the error code should
trigger this internal error for GDB?

> }
>
> static int
> @@ -7279,9 +7276,9 @@ decode_thumb_32bit_ld_mem_hints (uint16_t insn1, uint16_t insn2,
> return 0;
> }
>
> -static void
> -thumb_process_displaced_32bit_insn (uint16_t insn1, uint16_t insn2,
> - struct arm_insn_reloc_data *data)
> +static int
> +arm_relocate_insn_thumb_32bit (uint16_t insn1, uint16_t insn2,
> + struct arm_insn_reloc_data *data)
> {
> int err = 0;
> unsigned short op = bit (insn2, 15);
> @@ -7393,34 +7390,41 @@ thumb_process_displaced_32bit_insn (uint16_t insn1, uint16_t insn2,
> err = 1;
> }
>
> - if (err)
> - internal_error (__FILE__, __LINE__,
> - _("thumb_process_displaced_32bit_insn: Instruction decode error"));
> + return err;
>

The above one too?

Otherwise it looks mostly ok to me, though the patch is a little
convoluted due to code movement.