Bug 20362 - bug in arm-tdep.c
Summary: bug in arm-tdep.c
Status: RESOLVED FIXED
Alias: None
Product: gdb
Classification: Unclassified
Component: tdep (show other bugs)
Version: unknown
: P2 normal
Target Milestone: 8.2
Assignee: Tom Tromey
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-07-12 19:38 UTC by Tom Tromey
Modified: 2018-05-07 14:56 UTC (History)
0 users

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Tromey 2016-07-12 19:38:42 UTC
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.
Comment 1 Tom Tromey 2018-04-21 18:04:30 UTC
I finally looked up the ARM instruction encoding and now I have a patch.
Comment 2 Sourceware Commits 2018-05-07 14:55:14 UTC
The master branch has been updated by Tom Tromey <tromey@sourceware.org>:

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

commit ce887586b48acd02080c36d5495891116f886e8e
Author: Tom Tromey <tom@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  <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.
Comment 3 Tom Tromey 2018-05-07 14:56:40 UTC
Fixed.