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

Message ID 20180421214721.7232-2-tom@tromey.com
State New
Headers show
Series
  • Add -Wduplicated-cond
Related show

Commit Message

Tom Tromey April 21, 2018, 9:47 p.m.
-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(-)

-- 
2.13.6

Comments

Omair Javaid April 24, 2018, 9:09 p.m. | #1
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.
Tom Tromey May 4, 2018, 5:06 p.m. | #2
>>>>> "Omair" == Omair Javaid <omair.javaid@linaro.org> writes:


>> 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.

[...]

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

Did this work out?

Tom
Pedro Alves May 7, 2018, 2:35 p.m. | #3
On 05/04/2018 06:06 PM, Tom Tromey wrote:
>>>>>> "Omair" == Omair Javaid <omair.javaid@linaro.org> writes:

> 

>>> 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.

> [...]

> 

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

> 

> Did this work out?


FAOD, this is OK if there are no regressions.

Thanks,
Pedro Alves

Patch

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;