diff mbox

Introduce in_inclusive_range, fix -Wtautological-compare warnings

Message ID 1509373931-16340-1-git-send-email-simon.marchi@ericsson.com
State New, archived
Headers

Commit Message

Simon Marchi Oct. 30, 2017, 2:32 p.m. UTC 
  When compiling with clang or gcc 8, we see warnings like this:

/home/emaisin/src/binutils-gdb/gdb/arm-tdep.c:10013:13: error: comparison of 0 <= unsigned expression is always true [-Werror,-Wtautological-compare]
      if (0 <= insn_op1 && 3 >= insn_op1)
          ~ ^  ~~~~~~~~
/home/emaisin/src/binutils-gdb/gdb/arm-tdep.c:11722:20: error: comparison of unsigned expression >= 0 is always true [-Werror,-Wtautological-compare]
      else if (opB >= 0 && opB <= 2)
               ~~~ ^  ~

This is because an unsigned integer (opB in this case) will always be >=
0.  It is still useful to keep both bounds of the range in the
expression, even if one is at the edge of the data type range.  This
patch introduces a utility function in_inclusive_range that gets rid of
the warning while conveying that we are checking for a range.

Tested by rebuilding.

gdb/ChangeLog:

	* common/common-utils.h (in_inclusive_range): New function.
	* arm-tdep.c (arm_record_extension_space): Use
	in_inclusive_range.
	* cris-tdep.c (cris_spec_reg_applicable): Use
	in_inclusive_range.
---
 gdb/arm-tdep.c            | 4 ++--
 gdb/common/common-utils.h | 9 +++++++++
 gdb/cris-tdep.c           | 4 ++--
 3 files changed, 13 insertions(+), 4 deletions(-)

Comments

John Baldwin Oct. 30, 2017, 3:57 p.m. UTC | #1 
On 10/30/17 2:32 PM, Simon Marchi wrote:
> When compiling with clang or gcc 8, we see warnings like this:
> 
> /home/emaisin/src/binutils-gdb/gdb/arm-tdep.c:10013:13: error: comparison of 0 <= unsigned expression is always true [-Werror,-Wtautological-compare]
>       if (0 <= insn_op1 && 3 >= insn_op1)
>           ~ ^  ~~~~~~~~
> /home/emaisin/src/binutils-gdb/gdb/arm-tdep.c:11722:20: error: comparison of unsigned expression >= 0 is always true [-Werror,-Wtautological-compare]
>       else if (opB >= 0 && opB <= 2)
>                ~~~ ^  ~
> 
> This is because an unsigned integer (opB in this case) will always be >=
> 0.  It is still useful to keep both bounds of the range in the
> expression, even if one is at the edge of the data type range.  This
> patch introduces a utility function in_inclusive_range that gets rid of
> the warning while conveying that we are checking for a range.
> 
> Tested by rebuilding.
> 
> gdb/ChangeLog:
> 
> 	* common/common-utils.h (in_inclusive_range): New function.
> 	* arm-tdep.c (arm_record_extension_space): Use
> 	in_inclusive_range.
> 	* cris-tdep.c (cris_spec_reg_applicable): Use
> 	in_inclusive_range.
> ---
>  gdb/arm-tdep.c            | 4 ++--
>  gdb/common/common-utils.h | 9 +++++++++
>  gdb/cris-tdep.c           | 4 ++--
>  3 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/cris-tdep.c b/gdb/cris-tdep.c
> index d623eb6..209e29f 100644
> --- a/gdb/cris-tdep.c
> +++ b/gdb/cris-tdep.c
> @@ -1434,7 +1434,7 @@ cris_spec_reg_applicable (struct gdbarch *gdbarch,
>        /* Indeterminate/obsolete.  */
>        return 0;
>      case cris_ver_v0_3:
> -      return (version >= 0 && version <= 3);
> +      return in_inclusive_range (version, 0U, 3U);
>      case cris_ver_v3p:
>        return (version >= 3);
>      case cris_ver_v8:
> @@ -1442,7 +1442,7 @@ cris_spec_reg_applicable (struct gdbarch *gdbarch,
>      case cris_ver_v8p:
>        return (version >= 8);
>      case cris_ver_v0_10:
> -      return (version >= 0 && version <= 10);
> +      return in_inclusive_range (version, 0U, 10U);
>      case cris_ver_v3_10:
>        return (version >= 3 && version <= 10);
>      case cris_ver_v8_10:

I wonder if in this file it wouldn't be best to use the new function throughout
the various cases so that the style is more consistent?  LGTM regardless.
Simon Marchi Oct. 30, 2017, 4:01 p.m. UTC | #2 
On 2017-10-30 11:57, John Baldwin wrote:
> On 10/30/17 2:32 PM, Simon Marchi wrote:
>> When compiling with clang or gcc 8, we see warnings like this:
>> 
>> /home/emaisin/src/binutils-gdb/gdb/arm-tdep.c:10013:13: error: 
>> comparison of 0 <= unsigned expression is always true 
>> [-Werror,-Wtautological-compare]
>>       if (0 <= insn_op1 && 3 >= insn_op1)
>>           ~ ^  ~~~~~~~~
>> /home/emaisin/src/binutils-gdb/gdb/arm-tdep.c:11722:20: error: 
>> comparison of unsigned expression >= 0 is always true 
>> [-Werror,-Wtautological-compare]
>>       else if (opB >= 0 && opB <= 2)
>>                ~~~ ^  ~
>> 
>> This is because an unsigned integer (opB in this case) will always be 
>> >=
>> 0.  It is still useful to keep both bounds of the range in the
>> expression, even if one is at the edge of the data type range.  This
>> patch introduces a utility function in_inclusive_range that gets rid 
>> of
>> the warning while conveying that we are checking for a range.
>> 
>> Tested by rebuilding.
>> 
>> gdb/ChangeLog:
>> 
>> 	* common/common-utils.h (in_inclusive_range): New function.
>> 	* arm-tdep.c (arm_record_extension_space): Use
>> 	in_inclusive_range.
>> 	* cris-tdep.c (cris_spec_reg_applicable): Use
>> 	in_inclusive_range.
>> ---
>>  gdb/arm-tdep.c            | 4 ++--
>>  gdb/common/common-utils.h | 9 +++++++++
>>  gdb/cris-tdep.c           | 4 ++--
>>  3 files changed, 13 insertions(+), 4 deletions(-)
>> 
>> diff --git a/gdb/cris-tdep.c b/gdb/cris-tdep.c
>> index d623eb6..209e29f 100644
>> --- a/gdb/cris-tdep.c
>> +++ b/gdb/cris-tdep.c
>> @@ -1434,7 +1434,7 @@ cris_spec_reg_applicable (struct gdbarch 
>> *gdbarch,
>>        /* Indeterminate/obsolete.  */
>>        return 0;
>>      case cris_ver_v0_3:
>> -      return (version >= 0 && version <= 3);
>> +      return in_inclusive_range (version, 0U, 3U);
>>      case cris_ver_v3p:
>>        return (version >= 3);
>>      case cris_ver_v8:
>> @@ -1442,7 +1442,7 @@ cris_spec_reg_applicable (struct gdbarch 
>> *gdbarch,
>>      case cris_ver_v8p:
>>        return (version >= 8);
>>      case cris_ver_v0_10:
>> -      return (version >= 0 && version <= 10);
>> +      return in_inclusive_range (version, 0U, 10U);
>>      case cris_ver_v3_10:
>>        return (version >= 3 && version <= 10);
>>      case cris_ver_v8_10:
> 
> I wonder if in this file it wouldn't be best to use the new function 
> throughout
> the various cases so that the style is more consistent?  LGTM 
> regardless.


Good point, I'll make that change.  I'll see if it's possible in 
arm-tdep.c too or if it's a too daunting task.

Simon
John Baldwin Oct. 30, 2017, 4:05 p.m. UTC | #3 
On 10/30/17 4:01 PM, Simon Marchi wrote:
> On 2017-10-30 11:57, John Baldwin wrote:
>> On 10/30/17 2:32 PM, Simon Marchi wrote:
>>> When compiling with clang or gcc 8, we see warnings like this:
>>>
>>> /home/emaisin/src/binutils-gdb/gdb/arm-tdep.c:10013:13: error: 
>>> comparison of 0 <= unsigned expression is always true 
>>> [-Werror,-Wtautological-compare]
>>>       if (0 <= insn_op1 && 3 >= insn_op1)
>>>           ~ ^  ~~~~~~~~
>>> /home/emaisin/src/binutils-gdb/gdb/arm-tdep.c:11722:20: error: 
>>> comparison of unsigned expression >= 0 is always true 
>>> [-Werror,-Wtautological-compare]
>>>       else if (opB >= 0 && opB <= 2)
>>>                ~~~ ^  ~
>>>
>>> This is because an unsigned integer (opB in this case) will always be 
>>>> =
>>> 0.  It is still useful to keep both bounds of the range in the
>>> expression, even if one is at the edge of the data type range.  This
>>> patch introduces a utility function in_inclusive_range that gets rid 
>>> of
>>> the warning while conveying that we are checking for a range.
>>>
>>> Tested by rebuilding.
>>>
>>> gdb/ChangeLog:
>>>
>>> 	* common/common-utils.h (in_inclusive_range): New function.
>>> 	* arm-tdep.c (arm_record_extension_space): Use
>>> 	in_inclusive_range.
>>> 	* cris-tdep.c (cris_spec_reg_applicable): Use
>>> 	in_inclusive_range.
>>> ---
>>>  gdb/arm-tdep.c            | 4 ++--
>>>  gdb/common/common-utils.h | 9 +++++++++
>>>  gdb/cris-tdep.c           | 4 ++--
>>>  3 files changed, 13 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/gdb/cris-tdep.c b/gdb/cris-tdep.c
>>> index d623eb6..209e29f 100644
>>> --- a/gdb/cris-tdep.c
>>> +++ b/gdb/cris-tdep.c
>>> @@ -1434,7 +1434,7 @@ cris_spec_reg_applicable (struct gdbarch 
>>> *gdbarch,
>>>        /* Indeterminate/obsolete.  */
>>>        return 0;
>>>      case cris_ver_v0_3:
>>> -      return (version >= 0 && version <= 3);
>>> +      return in_inclusive_range (version, 0U, 3U);
>>>      case cris_ver_v3p:
>>>        return (version >= 3);
>>>      case cris_ver_v8:
>>> @@ -1442,7 +1442,7 @@ cris_spec_reg_applicable (struct gdbarch 
>>> *gdbarch,
>>>      case cris_ver_v8p:
>>>        return (version >= 8);
>>>      case cris_ver_v0_10:
>>> -      return (version >= 0 && version <= 10);
>>> +      return in_inclusive_range (version, 0U, 10U);
>>>      case cris_ver_v3_10:
>>>        return (version >= 3 && version <= 10);
>>>      case cris_ver_v8_10:
>>
>> I wonder if in this file it wouldn't be best to use the new function 
>> throughout
>> the various cases so that the style is more consistent?  LGTM 
>> regardless.
> 
> 
> Good point, I'll make that change.  I'll see if it's possible in 
> arm-tdep.c too or if it's a too daunting task.

It may be sufficient to just be consistent within a function?  In arm-tdep.c
it is two instances in separate functions whereas for cris-tdep.c it is
multiple instances in the same function which is what stuck out to me.
diff mbox

Patch

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index def3958..282efe0 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -10010,7 +10010,7 @@  arm_record_extension_space (insn_decode_record *arm_insn_r)
       && !INSN_RECORDED(arm_insn_r))
     {
       /* Handle MLA(S) and MUL(S).  */
-      if (0 <= insn_op1 && 3 >= insn_op1)
+      if (in_inclusive_range (insn_op1, 0U, 3U))
       {
         record_buf[0] = bits (arm_insn_r->arm_insn, 12, 15);
         record_buf[1] = ARM_PS_REGNUM;
@@ -11719,7 +11719,7 @@  thumb_record_ld_st_reg_offset (insn_decode_record *thumb_insn_r)
           record_buf[0] = reg_src1;
           thumb_insn_r->reg_rec_count = 1;
         }
-      else if (opB >= 0 && opB <= 2)
+      else if (in_inclusive_range (opB, 0U, 2U))
         {
           /* STR(2), STRB(2), STRH(2) .  */
           reg_src1 = bits (thumb_insn_r->arm_insn, 3, 5);
diff --git a/gdb/common/common-utils.h b/gdb/common/common-utils.h
index a32863c..4926a32 100644
--- a/gdb/common/common-utils.h
+++ b/gdb/common/common-utils.h
@@ -125,4 +125,13 @@  extern void free_vector_argv (std::vector<char *> &v);
    joining all the arguments with a whitespace separating them.  */
 extern std::string stringify_argv (const std::vector<char *> &argv);
 
+/* Return true if VALUE is in [LOW, HIGH].  */
+
+template <typename T>
+static bool
+in_inclusive_range (T value, T low, T high)
+{
+  return value >= low && value <= high;
+}
+
 #endif
diff --git a/gdb/cris-tdep.c b/gdb/cris-tdep.c
index d623eb6..209e29f 100644
--- a/gdb/cris-tdep.c
+++ b/gdb/cris-tdep.c
@@ -1434,7 +1434,7 @@  cris_spec_reg_applicable (struct gdbarch *gdbarch,
       /* Indeterminate/obsolete.  */
       return 0;
     case cris_ver_v0_3:
-      return (version >= 0 && version <= 3);
+      return in_inclusive_range (version, 0U, 3U);
     case cris_ver_v3p:
       return (version >= 3);
     case cris_ver_v8:
@@ -1442,7 +1442,7 @@  cris_spec_reg_applicable (struct gdbarch *gdbarch,
     case cris_ver_v8p:
       return (version >= 8);
     case cris_ver_v0_10:
-      return (version >= 0 && version <= 10);
+      return in_inclusive_range (version, 0U, 10U);
     case cris_ver_v3_10:
       return (version >= 3 && version <= 10);
     case cris_ver_v8_10: