Re: [PATCH 2/3] arm-tdep.c: Refactor arm_decode_dp_misc

[Yao Qi <qiyaoltc at gmail dot com>]

Simon Marchi <simon.marchi@ericsson.com> writes:

> Refactor arm_decode_dp_misc to make it more readable.  The new layout
> matches very closely the description in the ARM Architecture Reference
> Manual.  It uses the same order and same nomenclature.

As I mentioned in the reply to the patch cover letter, the manual may
adjust the layout in the future.  For example, the manual lists
instructions whose OP is 0 first, but it may change to list instructions
whose OP is 1 first in the future.  IMO, we don't have to 100% match the
code to the manual.

>
> gdb/ChangeLog:
>
> 	* arm-tdep.c (arm_decode_dp_misc): Refactor instruction decoding.
> ---
>  gdb/arm-tdep.c | 73 +++++++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 49 insertions(+), 24 deletions(-)
>
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 0a9c0f6..e17a1a4 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -6517,45 +6517,70 @@ arm_decode_dp_misc (struct gdbarch *gdbarch, uint32_t insn,
>  		    struct regcache *regs,
>  		    struct displaced_step_closure *dsc)
>  {
> -  if (bit (insn, 25))
> -    switch (bits (insn, 20, 24))
> -      {
> -      case 0x10:
> -	return arm_copy_unmodified (gdbarch, insn, "movw", dsc);
> -
> -      case 0x14:
> -	return arm_copy_unmodified (gdbarch, insn, "movt", dsc);
> +  uint8_t op = bit (insn, 25);
> +  uint8_t op1 = bits (insn, 20, 24);
> +  uint8_t op2 = bits (insn, 4, 7);
>  
> -      case 0x12: case 0x16:
> -	return arm_copy_unmodified (gdbarch, insn, "msr imm", dsc);
> -
> -      default:
> -	return arm_copy_alu_imm (gdbarch, insn, regs, dsc);
> -      }
> -  else
> +  if (op == 0)
>      {
> -      uint32_t op1 = bits (insn, 20, 24), op2 = bits (insn, 4, 7);
> -
>        if ((op1 & 0x19) != 0x10 && (op2 & 0x1) == 0x0)
> +	/* Data-processing (register)  */
>  	return arm_copy_alu_reg (gdbarch, insn, regs, dsc);
>        else if ((op1 & 0x19) != 0x10 && (op2 & 0x9) == 0x1)
> +	/* Data-processing (register-shifted register)  */
>  	return arm_copy_alu_shifted_reg (gdbarch, insn, regs, dsc);
>        else if ((op1 & 0x19) == 0x10 && (op2 & 0x8) == 0x0)
> +	/* Miscellaneous instructions  */
>  	return arm_decode_miscellaneous (gdbarch, insn, regs, dsc);
>        else if ((op1 & 0x19) == 0x10 && (op2 & 0x9) == 0x8)
> +	/* Halfword multiply and multiply accumulate  */
>  	return arm_copy_unmodified (gdbarch, insn, "halfword mul/mla", dsc);
>        else if ((op1 & 0x10) == 0x00 && op2 == 0x9)
> +	/* Multiply and multiply accumulate  */
>  	return arm_copy_unmodified (gdbarch, insn, "mul/mla", dsc);
>        else if ((op1 & 0x10) == 0x10 && op2 == 0x9)
> +	/* Synchronization primitives  */
>  	return arm_copy_unmodified (gdbarch, insn, "synch", dsc);

These added comments are helpful.

> -      else if (op2 == 0xb || (op2 & 0xd) == 0xd)
> -	/* 2nd arg means "unprivileged".  */
> -	return arm_copy_extra_ld_st (gdbarch, insn, (op1 & 0x12) == 0x02, regs,
> -				     dsc);
> +      else if ((op1 & 0x12) != 0x2 && op2 == 0xb)
> +	/* Extra load/store instructions  */
> +	return arm_copy_extra_ld_st (gdbarch, insn, 0, regs, dsc);
> +      else if ((op1 & 0x12) != 0x2 && (op2 & 0xd) == 0xd)
> +	/* Extra load/store instructions  */
> +	return arm_copy_extra_ld_st (gdbarch, insn, 0, regs, dsc);
> +      else if ((op1 & 0x13) == 0x2 && (op2 & 0xd) == 0xd)
> +	/* Extra load/store instructions  */
> +	return arm_copy_extra_ld_st (gdbarch, insn, 0, regs, dsc);
> +      else if ((op1 & 0x12) == 0x2 && op2 == 0xd)
> +	/* Extra load/store instructions, unprivileged  */
> +	return arm_copy_extra_ld_st (gdbarch, insn, 1, regs, dsc);
> +      else if ((op1 & 0x13) == 0x3 && (op2 & 0xd) == 0xd)
> +	/* Extra load/store instructions, unprivileged  */
> +	return arm_copy_extra_ld_st (gdbarch, insn, 1, regs, dsc);
> +      else
> +	return 1;

However, I don't see how helpful or useful the changes above are.

> +    }
> +  else
> +    {
> +      switch (op1)
> +        {
> +        default:
> +          /* Data-processing (immediate)  */
> +	  return arm_copy_alu_imm (gdbarch, insn, regs, dsc);
> +
> +        case 0x10:
> +          /* 16-bit immediate load, MOV (immediate)  */
> +          return arm_copy_unmodified (gdbarch, insn, "movw", dsc);
> +
> +        case 0x14:
> +          /* High halfword 16-bit immediate load, MOVT  */
> +	  return arm_copy_unmodified (gdbarch, insn, "movt", dsc);
> +
> +	case 0x12:
> +	case 0x16:
> +	  /* MSR (immediate), and hints  */
> +	  return arm_copy_unmodified (gdbarch, insn, "msr imm", dsc);
> +        }
>      }
> -
> -  /* Should be unreachable.  */
> -  return 1;
>  }

In short, I don't see how this patch improve the readability of the
code, and I feel hard mapping the code to the manual.

-- 
Yao (éå)