[Bug tdep/20362] New: bug in arm-tdep.c

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=20362

Bug ID: 20362
Summary: bug in arm-tdep.c
Product: gdb
Version: unknown
Status: NEW
Severity: normal
Priority: P2
Component: tdep
Assignee: unassigned at sourceware dot org
Reporter: tromey at sourceware dot org
Target Milestone: ---

I tried compiling gdb with -Wduplicated-cond.
gcc complained about arm_record_vfp_data_proc_insn.
I think there are two bugs here.

First, gcc complains:

../../binutils-gdb/gdb/arm-tdep.c:11364:8: error: duplicated ‘if’ condition
[-Werror=duplicated-cond]
else if (opc1 == 0x0b)
^~
../../binutils-gdb/gdb/arm-tdep.c:11356:17: note: previously used here
else if (opc1 == 0x0b)
~~~~~^~~~~~~


Second, and IMO more seriously, most of these cases can't be taken
at all AFAICT. In that function:

opc1 = opc1 & 0x04;
...
else if (opc1 == 0x01)
...
else if (opc1 == 0x02 && !(opc3 & 0x01))
...
else if (opc1 == 0x03)
...
else if (opc1 == 0x0b)


I think none of these can possibly be true.

I'll file a bug about gcc not warning for these.

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=20362

Tom Tromey <tromey at sourceware dot org> changed:

What |Removed |Added
----------------------------------------------------------------------------
Assignee|unassigned at sourceware dot org |tromey at sourceware dot org

--- Comment #1 from Tom Tromey <tromey at sourceware dot org> ---
I finally looked up the ARM instruction encoding and now I have a patch.

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=20362

--- Comment #2 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Tom Tromey <***@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=ce887586b48acd02080c36d5495891116f886e8e

commit ce887586b48acd02080c36d5495891116f886e8e
Author: Tom Tromey <***@tromey.com>
Date: Sat Apr 21 11:51:34 2018 -0600

Fix decoding of ARM VFP instructions

-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-05-07 Tom Tromey <***@tromey.com>

PR tdep/20362:
* arm-tdep.c (arm_record_vfp_data_proc_insn): Properly mask off D
bit. Use correct value for VDIV.

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=20362

Tom Tromey <tromey at sourceware dot org> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED
Target Milestone|--- |8.2

--- Comment #3 from Tom Tromey <tromey at sourceware dot org> ---
Fixed.