Re: [RFA 1/2] Fix decoding of ARM VFP instructions

[Omair Javaid <omair.javaid@linaro.org>]

On 22 April 2018 at 02:47, Tom Tromey <tom@tromey.com> wrote:
> -Wduplicated-cond pointed out that arm_record_vfp_data_proc_insn
> checks "opc1 == 0x0b" twice.  I filed this a while ago as
> PR tdep/20362.
>
> Based on the ARM instruction manual at
> https://www.scss.tcd.ie/~waldroj/3d1/arm_arm.pdf, I think the
> instruction decoding in this function has two bugs.
>
> First, opc1 is computed as:
>
>   opc1 = bits (arm_insn_r->arm_insn, 20, 23);
> [...]
>   opc1 = opc1 & 0x04;
>
> This means that tests like:
>
>   else if (opc1 == 0x01)
>
> can never be true.
>
> In the ARM manual, "opc1" corresponds to these bits:
>
>     name   bit
>     r      20
>     q      21
>     D      22
>     p      23
>
> ... where the D bit is not used for VFP instruction decoding.
>
> So, I believe this code should use ~0x04 instead.
>
> Second, VDIV is recognized by the bits "pqrs" being equal to "1000".
> This tranlates to opc1 == 0x08 -- not 0x0b.  Note that pqrs==1001 is
> an undefined encoding, which is probably why opc2 is not checked here;
> this code doesn't seem to really deal with undefined encodings in
> general, so I've left that as is.
>
> I don't have an ARM machine or any reasonable way to test this.
>
> ChangeLog
> 2018-04-21  Tom Tromey  <tom@tromey.com>
>
>         PR tdep/20362:
>         * arm-tdep.c (arm_record_vfp_data_proc_insn): Properly mask off D
>         bit.  Use correct value for VDIV.
> ---
>  gdb/ChangeLog  | 6 ++++++
>  gdb/arm-tdep.c | 5 +++--
>  2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index f64df4c574..98bbb0244c 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -11420,7 +11420,8 @@ arm_record_vfp_data_proc_insn (insn_decode_record *arm_insn_r)
>    opc3 = bits (arm_insn_r->arm_insn, 6, 7);
>    dp_op_sz = bit (arm_insn_r->arm_insn, 8);
>    bit_d = bit (arm_insn_r->arm_insn, 22);
> -  opc1 = opc1 & 0x04;
> +  /* Mask off the "D" bit.  */
> +  opc1 = opc1 & ~0x04;
>
>    /* Handle VMLA, VMLS.  */
>    if (opc1 == 0x00)
> @@ -11485,7 +11486,7 @@ arm_record_vfp_data_proc_insn (insn_decode_record *arm_insn_r)
>          }
>      }
>    /* Handle VDIV.  */
> -  else if (opc1 == 0x0b)
> +  else if (opc1 == 0x08)
>      {
>        if (dp_op_sz)
>          curr_insn_type = INSN_T1;
> --
> 2.13.6
>

Seems LGTM. Let me get back to you after running testsuite for regressions.