[RFA 2/2] Add -Wduplicated-cond

[RFA 2/2] Add -Wduplicated-cond

[Tom Tromey]

This adds -Wduplicated-cond to warnings.m4.  This caught one bug.

I tried adding -Wduplicated-branches as well, but it results in some
spurious failures from code like this in cgen.h:

    #define CGEN_ATTR_TYPE(n) \
    struct { unsigned int bool_; \
	     CGEN_ATTR_VALUE_TYPE nonbool[(n) ? (n) : 1]; }

This will trigger a warning if passed n==1, which seems like a
perfectly valid thing to do; and there were other issues like this as
well.

2018-04-21  Tom Tromey  <tom@tromey.com>

	* configure: Rebuild.
	* warning.m4 (AM_GDB_WARNINGS): Add -Wduplicated-cond.

gdbserver/ChangeLog
2018-04-21  Tom Tromey  <tom@tromey.com>

	* configure: Rebuild.
---
 gdb/ChangeLog           |  5 +++++
 gdb/configure           | 27 ++++++++++++++-------------
 gdb/gdbserver/ChangeLog |  4 ++++
 gdb/gdbserver/configure | 32 ++++++++++++++++++++------------
 gdb/warning.m4          |  3 ++-
 5 files changed, 45 insertions(+), 26 deletions(-)

diff --git a/gdb/configure b/gdb/configure
index b313152018..29efb4b81c 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -4463,9 +4463,9 @@ else
 fi
 
   if test "$plugins" = "yes"; then
-    { $as_echo "$as_me:${as_lineno-$LINENO}: checking for library containing dlopen" >&5
-$as_echo_n "checking for library containing dlopen... " >&6; }
-if test "${ac_cv_search_dlopen+set}" = set; then :
+    { $as_echo "$as_me:${as_lineno-$LINENO}: checking for library containing dlsym" >&5
+$as_echo_n "checking for library containing dlsym... " >&6; }
+if test "${ac_cv_search_dlsym+set}" = set; then :
   $as_echo_n "(cached) " >&6
 else
   ac_func_search_save_LIBS=$LIBS
@@ -4478,11 +4478,11 @@ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
 #ifdef __cplusplus
 extern "C"
 #endif
-char dlopen ();
+char dlsym ();
 int
 main ()
 {
-return dlopen ();
+return dlsym ();
   ;
   return 0;
 }
@@ -4495,25 +4495,25 @@ for ac_lib in '' dl; do
     LIBS="-l$ac_lib  $ac_func_search_save_LIBS"
   fi
   if ac_fn_c_try_link "$LINENO"; then :
-  ac_cv_search_dlopen=$ac_res
+  ac_cv_search_dlsym=$ac_res
 fi
 rm -f core conftest.err conftest.$ac_objext \
     conftest$ac_exeext
-  if test "${ac_cv_search_dlopen+set}" = set; then :
+  if test "${ac_cv_search_dlsym+set}" = set; then :
   break
 fi
 done
-if test "${ac_cv_search_dlopen+set}" = set; then :
+if test "${ac_cv_search_dlsym+set}" = set; then :
 
 else
-  ac_cv_search_dlopen=no
+  ac_cv_search_dlsym=no
 fi
 rm conftest.$ac_ext
 LIBS=$ac_func_search_save_LIBS
 fi
-{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_search_dlopen" >&5
-$as_echo "$ac_cv_search_dlopen" >&6; }
-ac_res=$ac_cv_search_dlopen
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_search_dlsym" >&5
+$as_echo "$ac_cv_search_dlsym" >&6; }
+ac_res=$ac_cv_search_dlsym
 if test "$ac_res" != no; then :
   test "$ac_res" = "none required" || LIBS="$ac_res $LIBS"
 
@@ -15365,7 +15365,8 @@ build_warnings="-Wall -Wpointer-arith \
 -Wempty-body -Wunused-but-set-parameter -Wunused-but-set-variable \
 -Wno-sign-compare -Wno-narrowing -Wno-error=maybe-uninitialized \
 -Wno-mismatched-tags \
--Wno-error=deprecated-register"
+-Wno-error=deprecated-register \
+-Wduplicated-cond"
 
 case "${host}" in
   *-*-mingw32*)
diff --git a/gdb/gdbserver/configure b/gdb/gdbserver/configure
index ab09261946..48af8310d9 100755
--- a/gdb/gdbserver/configure
+++ b/gdb/gdbserver/configure
@@ -4658,7 +4658,7 @@ program_transform_name=`$as_echo "$program_transform_name" | sed "$ac_script"`
 # We require a C++11 compiler.  Check if one is available, and if
 # necessary, set CXX_DIALECT to some -std=xxx switch.
 
-      ax_cxx_compile_cxx11_required=true
+  ax_cxx_compile_alternatives="11 0x"    ax_cxx_compile_cxx11_required=true
   ac_ext=cpp
 ac_cpp='$CXXCPP $CPPFLAGS'
 ac_compile='$CXX -c $CXXFLAGS $CPPFLAGS conftest.$ac_ext >&5'
@@ -4974,7 +4974,8 @@ $as_echo "$ax_cv_cxx_compile_cxx11" >&6; }
   fi
 
     if test x$ac_success = xno; then
-    for switch in -std=gnu++11 -std=gnu++0x; do
+    for alternative in ${ax_cxx_compile_alternatives}; do
+      switch="-std=gnu++${alternative}"
       cachevar=`$as_echo "ax_cv_cxx_compile_cxx11_$switch" | $as_tr_sh`
       { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether $CXX supports C++11 features with $switch" >&5
 $as_echo_n "checking whether $CXX supports C++11 features with $switch... " >&6; }
@@ -5292,16 +5293,17 @@ $as_echo "$ac_res" >&6; }
   fi
 
     if test x$ac_success = xno; then
-                for switch in -std=c++11 -std=c++0x +std=c++11 "-h std=c++11"; do
-      cachevar=`$as_echo "ax_cv_cxx_compile_cxx11_$switch" | $as_tr_sh`
-      { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether $CXX supports C++11 features with $switch" >&5
+                for alternative in ${ax_cxx_compile_alternatives}; do
+      for switch in -std=c++${alternative} +std=c++${alternative} "-h std=c++${alternative}"; do
+        cachevar=`$as_echo "ax_cv_cxx_compile_cxx11_$switch" | $as_tr_sh`
+        { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether $CXX supports C++11 features with $switch" >&5
 $as_echo_n "checking whether $CXX supports C++11 features with $switch... " >&6; }
 if { as_var=$cachevar; eval "test \"\${$as_var+set}\" = set"; }; then :
   $as_echo_n "(cached) " >&6
 else
   ac_save_CXX="$CXX"
-         CXX="$CXX $switch"
-         cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+           CXX="$CXX $switch"
+           cat confdefs.h - <<_ACEOF >conftest.$ac_ext
 /* end confdefs.h.  */
 
 
@@ -5596,14 +5598,18 @@ else
   eval $cachevar=no
 fi
 rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
-         CXX="$ac_save_CXX"
+           CXX="$ac_save_CXX"
 fi
 eval ac_res=\$$cachevar
 	       { $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_res" >&5
 $as_echo "$ac_res" >&6; }
-      if eval test x\$$cachevar = xyes; then
-        CXX_DIALECT="$switch"
-        ac_success=yes
+        if eval test x\$$cachevar = xyes; then
+          CXX_DIALECT="$switch"
+          ac_success=yes
+          break
+        fi
+      done
+      if test x$ac_success = xyes; then
         break
       fi
     done
@@ -7165,7 +7171,9 @@ build_warnings="-Wall -Wpointer-arith \
 -Wno-switch -Wno-char-subscripts \
 -Wempty-body -Wunused-but-set-parameter -Wunused-but-set-variable \
 -Wno-sign-compare -Wno-narrowing -Wno-error=maybe-uninitialized \
--Wno-mismatched-tags"
+-Wno-mismatched-tags \
+-Wno-error=deprecated-register \
+-Wduplicated-cond"
 
 case "${host}" in
   *-*-mingw32*)
diff --git a/gdb/warning.m4 b/gdb/warning.m4
index 3cfae65e78..2c85933c8d 100644
--- a/gdb/warning.m4
+++ b/gdb/warning.m4
@@ -42,7 +42,8 @@ build_warnings="-Wall -Wpointer-arith \
 -Wempty-body -Wunused-but-set-parameter -Wunused-but-set-variable \
 -Wno-sign-compare -Wno-narrowing -Wno-error=maybe-uninitialized \
 -Wno-mismatched-tags \
--Wno-error=deprecated-register"
+-Wno-error=deprecated-register \
+-Wduplicated-cond"
 
 case "${host}" in
   *-*-mingw32*)
-- 
2.13.6

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

[Tom Tromey]

-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

[RFA 0/2] Add -Wduplicated-cond

[Tom Tromey]

This adds -Wduplicated-cond to the gdb build.

This warning found a bug in arm-tdep.c, which I've attempted to fix
here.

Tested by the buildbot, though of course that doesn't have an ARM
builder.  This seems like a good thing to set up, either on hardware
or perhaps via qemu.

Tom

Re: [RFA 2/2] Add -Wduplicated-cond

[Pedro Alves]

On 04/21/2018 10:47 PM, Tom Tromey wrote:

> diff --git a/gdb/configure b/gdb/configure
> index b313152018..29efb4b81c 100755
> --- a/gdb/configure
> +++ b/gdb/configure
> @@ -4463,9 +4463,9 @@ else
>  fi
>  
>    if test "$plugins" = "yes"; then
> -    { $as_echo "$as_me:${as_lineno-$LINENO}: checking for library containing dlopen" >&5
> -$as_echo_n "checking for library containing dlopen... " >&6; }
> -if test "${ac_cv_search_dlopen+set}" = set; then :
> +    { $as_echo "$as_me:${as_lineno-$LINENO}: checking for library containing dlsym" >&5
> +$as_echo_n "checking for library containing dlsym... " >&6; }
> +if test "${ac_cv_search_dlsym+set}" = set; then :
>    $as_echo_n "(cached) " >&6
>  else
>    ac_func_search_save_LIBS=$LIBS
> @@ -4478,11 +4478,11 @@ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
>  #ifdef __cplusplus
>  extern "C"
>  #endif
> -char dlopen ();
> +char dlsym ();
>  int
>  main ()
>  {
> -return dlopen ();
> +return dlsym ();
>    ;
>    return 0;
>  }
> @@ -4495,25 +4495,25 @@ for ac_lib in '' dl; do
>      LIBS="-l$ac_lib  $ac_func_search_save_LIBS"
>    fi
>    if ac_fn_c_try_link "$LINENO"; then :
> -  ac_cv_search_dlopen=$ac_res
> +  ac_cv_search_dlsym=$ac_res
>  fi
>  rm -f core conftest.err conftest.$ac_objext \
>      conftest$ac_exeext
> -  if test "${ac_cv_search_dlopen+set}" = set; then :
> +  if test "${ac_cv_search_dlsym+set}" = set; then :
>    break
>  fi
>  done
> -if test "${ac_cv_search_dlopen+set}" = set; then :
> +if test "${ac_cv_search_dlsym+set}" = set; then :
>  
>  else
> -  ac_cv_search_dlopen=no
> +  ac_cv_search_dlsym=no
>  fi
>  rm conftest.$ac_ext
>  LIBS=$ac_func_search_save_LIBS
>  fi
> -{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_search_dlopen" >&5
> -$as_echo "$ac_cv_search_dlopen" >&6; }
> -ac_res=$ac_cv_search_dlopen
> +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_search_dlsym" >&5
> +$as_echo "$ac_cv_search_dlsym" >&6; }
> +ac_res=$ac_cv_search_dlsym
>  if test "$ac_res" != no; then :
>    test "$ac_res" = "none required" || LIBS="$ac_res $LIBS"
>  
> @@ -15365,7 +15365,8 @@ build_warnings="-Wall -Wpointer-arith \
>  -Wempty-body -Wunused-but-set-parameter -Wunused-but-set-variable \
>  -Wno-sign-compare -Wno-narrowing -Wno-error=maybe-uninitialized \
>  -Wno-mismatched-tags \
> --Wno-error=deprecated-register"
> +-Wno-error=deprecated-register \
> +-Wduplicated-cond"
>  
>  case "${host}" in
>    *-*-mingw32*)
> diff --git a/gdb/gdbserver/configure b/gdb/gdbserver/configure
> index ab09261946..48af8310d9 100755
> --- a/gdb/gdbserver/configure
> +++ b/gdb/gdbserver/configure
> @@ -4658,7 +4658,7 @@ program_transform_name=`$as_echo "$program_transform_name" | sed "$ac_script"`
>  # We require a C++11 compiler.  Check if one is available, and if
>  # necessary, set CXX_DIALECT to some -std=xxx switch.
>  
> -      ax_cxx_compile_cxx11_required=true
> +  ax_cxx_compile_alternatives="11 0x"    ax_cxx_compile_cxx11_required=true
>    ac_ext=cpp
>  ac_cpp='$CXXCPP $CPPFLAGS'
>  ac_compile='$CXX -c $CXXFLAGS $CPPFLAGS conftest.$ac_ext >&5'
> @@ -4974,7 +4974,8 @@ $as_echo "$ax_cv_cxx_compile_cxx11" >&6; }
>    fi
>  
>      if test x$ac_success = xno; then
> -    for switch in -std=gnu++11 -std=gnu++0x; do
> +    for alternative in ${ax_cxx_compile_alternatives}; do
> +      switch="-std=gnu++${alternative}"
>        cachevar=`$as_echo "ax_cv_cxx_compile_cxx11_$switch" | $as_tr_sh`
>        { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether $CXX supports C++11 features with $switch" >&5
>  $as_echo_n "checking whether $CXX supports C++11 features with $switch... " >&6; }
> @@ -5292,16 +5293,17 @@ $as_echo "$ac_res" >&6; }
>    fi
>  
>      if test x$ac_success = xno; then
> -                for switch in -std=c++11 -std=c++0x +std=c++11 "-h std=c++11"; do
> -      cachevar=`$as_echo "ax_cv_cxx_compile_cxx11_$switch" | $as_tr_sh`
> -      { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether $CXX supports C++11 features with $switch" >&5
> +                for alternative in ${ax_cxx_compile_alternatives}; do
> +      for switch in -std=c++${alternative} +std=c++${alternative} "-h std=c++${alternative}"; do
> +        cachevar=`$as_echo "ax_cv_cxx_compile_cxx11_$switch" | $as_tr_sh`
> +        { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether $CXX supports C++11 features with $switch" >&5
>  $as_echo_n "checking whether $CXX supports C++11 features with $switch... " >&6; }
>  if { as_var=$cachevar; eval "test \"\${$as_var+set}\" = set"; }; then :
>    $as_echo_n "(cached) " >&6
>  else
>    ac_save_CXX="$CXX"
> -         CXX="$CXX $switch"
> -         cat confdefs.h - <<_ACEOF >conftest.$ac_ext
> +           CXX="$CXX $switch"
> +           cat confdefs.h - <<_ACEOF >conftest.$ac_ext
>  /* end confdefs.h.  */
>  
>  
> @@ -5596,14 +5598,18 @@ else
>    eval $cachevar=no
>  fi
>  rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
> -         CXX="$ac_save_CXX"
> +           CXX="$ac_save_CXX"
>  fi
>  eval ac_res=\$$cachevar
>  	       { $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_res" >&5
>  $as_echo "$ac_res" >&6; }
> -      if eval test x\$$cachevar = xyes; then
> -        CXX_DIALECT="$switch"
> -        ac_success=yes
> +        if eval test x\$$cachevar = xyes; then
> +          CXX_DIALECT="$switch"
> +          ac_success=yes
> +          break
> +        fi
> +      done
> +      if test x$ac_success = xyes; then
>          break
>        fi
>      done
The patch looks fine to me, but the above looks like unrelated changes.
Very much looks like gdbserver/configure was not regenerated
when gdb/warning.m4 was last touched for -Wno-error=deprecated-register,
and  gdb's configure hadn't picked up the changes to config/plugin.m4
yet either.

Could you please push the obvious preliminary patch that just
regenerates gdb/configure and gdbserver/configure,
then rebase your patch on top of that.

I've not looked at the ARM patch; Alan, do you know who can
lend a hand with that?

Thanks,
Pedro Alves

Re: [RFA 0/2] Add -Wduplicated-cond

[Sergio Durigan Junior]

On Saturday, April 21 2018, Tom Tromey wrote:

> Tested by the buildbot, though of course that doesn't have an ARM
> builder.  This seems like a good thing to set up, either on hardware
> or perhaps via qemu.

We have AArch32/64 builders, but no ARM indeed.  If anyone wants to
contribute, just let me know.

Cheers,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

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

[Omair Javaid]

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.

Re: [RFA 2/2] Add -Wduplicated-cond

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Could you please push the obvious preliminary patch that just
Pedro> regenerates gdb/configure and gdbserver/configure,
Pedro> then rebase your patch on top of that.

I did this the other day.

Tom

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

[Tom Tromey]

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

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

[Pedro Alves]

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