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

[Luis Machado <lgustavo@codesourcery.com>]

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.