Fix thinko on common/offset-type.h (compare 'lhs' against 'rhs')

  While doing something else, I noticed that the OFFSET_TYPE's
"DEFINE_OFFSET_REL_OP" has a thinko: it is comparing 'lhs' against
itself, instead of against 'rhs'.  This patch fixes it.

I also found an interesting thing.  We have an unittest for
offset-type, and in theory it should have caught this problem, because
it has tests for relational operators.  However, the tests
successfully pass, and after some investigation I'm almost sure this
is because these operators are not being properly overloaded.  I tried
a few things to make them be used, without success.  If someone wants
to give this a try, I'd appreciate.

No regressions introduced.

gdb/ChangeLog:
2018-10-25  Sergio Durigan Junior  <sergiodj@redhat.com>

	* common/offset-type.h (DEFINE_OFFSET_REL_OP): Compare 'lhs'
	against 'rhs', instead of with 'lhs' again.
---
 gdb/ChangeLog            | 5 +++++
 gdb/common/offset-type.h | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)
  

On Thu, 25 Oct 2018 17:10:08 -0400
Sergio Durigan Junior <sergiodj@redhat.com> wrote:

> While doing something else, I noticed that the OFFSET_TYPE's
> "DEFINE_OFFSET_REL_OP" has a thinko: it is comparing 'lhs' against
> itself, instead of against 'rhs'.  This patch fixes it.
> 
> I also found an interesting thing.  We have an unittest for
> offset-type, and in theory it should have caught this problem, because
> it has tests for relational operators.  However, the tests
> successfully pass, and after some investigation I'm almost sure this
> is because these operators are not being properly overloaded.  I tried
> a few things to make them be used, without success.  If someone wants
> to give this a try, I'd appreciate.
> 
> No regressions introduced.
> 
> gdb/ChangeLog:
> 2018-10-25  Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	* common/offset-type.h (DEFINE_OFFSET_REL_OP): Compare 'lhs'
> 	against 'rhs', instead of with 'lhs' again.

LGTM.

(I'm surprised that it wasn't caught by the unit test or by someone
else noticing a bug elsewhere in GDB.)

Kevin
  

On 2018-10-25 5:10 p.m., Sergio Durigan Junior wrote:
> While doing something else, I noticed that the OFFSET_TYPE's

> "DEFINE_OFFSET_REL_OP" has a thinko: it is comparing 'lhs' against

> itself, instead of against 'rhs'.  This patch fixes it.

> 

> I also found an interesting thing.  We have an unittest for

> offset-type, and in theory it should have caught this problem, because

> it has tests for relational operators.  However, the tests

> successfully pass, and after some investigation I'm almost sure this

> is because these operators are not being properly overloaded.  I tried

> a few things to make them be used, without success.  If someone wants

> to give this a try, I'd appreciate.

> 

> No regressions introduced.

> 

> gdb/ChangeLog:

> 2018-10-25  Sergio Durigan Junior  <sergiodj@redhat.com>

> 

> 	* common/offset-type.h (DEFINE_OFFSET_REL_OP): Compare 'lhs'

> 	against 'rhs', instead of with 'lhs' again.

> ---

>  gdb/ChangeLog            | 5 +++++

>  gdb/common/offset-type.h | 2 +-

>  2 files changed, 6 insertions(+), 1 deletion(-)

> 

> diff --git a/gdb/ChangeLog b/gdb/ChangeLog

> index 61dc039d4f..d16c81b3a7 100644

> --- a/gdb/ChangeLog

> +++ b/gdb/ChangeLog

> @@ -1,3 +1,8 @@

> +2018-10-25  Sergio Durigan Junior  <sergiodj@redhat.com>

> +

> +	* common/offset-type.h (DEFINE_OFFSET_REL_OP): Compare 'lhs'

> +	against 'rhs', instead of with 'lhs' again.

> +

>  2018-10-25  Andrew Burgess  <andrew.burgess@embecosm.com>

>  

>  	* python/py-function.c (convert_values_to_python): Return

> diff --git a/gdb/common/offset-type.h b/gdb/common/offset-type.h

> index b480b14406..ed59227aa5 100644

> --- a/gdb/common/offset-type.h

> +++ b/gdb/common/offset-type.h

> @@ -81,7 +81,7 @@

>    {									\

>      using underlying = typename std::underlying_type<E>::type;		\

>      return (static_cast<underlying> (lhs)				\

> -	    OP static_cast<underlying> (lhs));				\

> +	    OP static_cast<underlying> (rhs));				\

>    }

>  

>  DEFINE_OFFSET_REL_OP(>)

> 


Woops.  I couldn't believe this had not caused any visible bugs, given that
the two offset types defined currently (cu_offset and sect_offset) are used
quite a lot.  I was also surprised that the unit tests in
unittests/offset-type-selftests.c passed, since we have checks for these:

  /* Test <, <=, >, >=.  */
  {
    constexpr off_A o1 = (off_A) 10;
    constexpr off_A o2 = (off_A) 20;

    static_assert (o1 < o2, "");
    static_assert (!(o2 < o1), "");

    static_assert (o2 > o1, "");
    static_assert (!(o1 > o2), "");

    static_assert (o1 <= o2, "");
    static_assert (!(o2 <= o1), "");

    static_assert (o2 >= o1, "");
    static_assert (!(o1 >= o2), "");

    static_assert (o1 <= o1, "");
    static_assert (o1 >= o1, "");
  }

I changed these to SELF_CHECK, stuck a gdb_assert(false) in the operator
definition (in the DEFINE_OFFSET_REL_OP macro), and the selftest still runs
without any error.

And if you just remove them (the DEFINE_OFFSET_REL_OP macro and its usages),
the compiler is perfectly happy.  So I'm starting to think this operator
definition is not used nor needed.  The important thing is that the compiler
rejects comparisons between different offset types, such as what is tested here:

  CHECK_VALID (false, void,   off_A {} < off_B {});

but if the compiler is able to generate a default comparison operator between
two operands of the same offset type, then I don't think we need to provide
one explicitly.

Therefore, I think we could just remove the relational operator definitions
entirely.

Simon
  

On Friday, October 26 2018, Simon Marchi wrote:

> On 2018-10-25 5:10 p.m., Sergio Durigan Junior wrote:
>> While doing something else, I noticed that the OFFSET_TYPE's
>> "DEFINE_OFFSET_REL_OP" has a thinko: it is comparing 'lhs' against
>> itself, instead of against 'rhs'.  This patch fixes it.
>> 
>> I also found an interesting thing.  We have an unittest for
>> offset-type, and in theory it should have caught this problem, because
>> it has tests for relational operators.  However, the tests
>> successfully pass, and after some investigation I'm almost sure this
>> is because these operators are not being properly overloaded.  I tried
>> a few things to make them be used, without success.  If someone wants
>> to give this a try, I'd appreciate.
>> 
>> No regressions introduced.
>> 
>> gdb/ChangeLog:
>> 2018-10-25  Sergio Durigan Junior  <sergiodj@redhat.com>
>> 
>> 	* common/offset-type.h (DEFINE_OFFSET_REL_OP): Compare 'lhs'
>> 	against 'rhs', instead of with 'lhs' again.
>> ---
>>  gdb/ChangeLog            | 5 +++++
>>  gdb/common/offset-type.h | 2 +-
>>  2 files changed, 6 insertions(+), 1 deletion(-)
>> 
>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>> index 61dc039d4f..d16c81b3a7 100644
>> --- a/gdb/ChangeLog
>> +++ b/gdb/ChangeLog
>> @@ -1,3 +1,8 @@
>> +2018-10-25  Sergio Durigan Junior  <sergiodj@redhat.com>
>> +
>> +	* common/offset-type.h (DEFINE_OFFSET_REL_OP): Compare 'lhs'
>> +	against 'rhs', instead of with 'lhs' again.
>> +
>>  2018-10-25  Andrew Burgess  <andrew.burgess@embecosm.com>
>>  
>>  	* python/py-function.c (convert_values_to_python): Return
>> diff --git a/gdb/common/offset-type.h b/gdb/common/offset-type.h
>> index b480b14406..ed59227aa5 100644
>> --- a/gdb/common/offset-type.h
>> +++ b/gdb/common/offset-type.h
>> @@ -81,7 +81,7 @@
>>    {									\
>>      using underlying = typename std::underlying_type<E>::type;		\
>>      return (static_cast<underlying> (lhs)				\
>> -	    OP static_cast<underlying> (lhs));				\
>> +	    OP static_cast<underlying> (rhs));				\
>>    }
>>  
>>  DEFINE_OFFSET_REL_OP(>)
>> 
>
> Woops.  I couldn't believe this had not caused any visible bugs, given that
> the two offset types defined currently (cu_offset and sect_offset) are used
> quite a lot.  I was also surprised that the unit tests in
> unittests/offset-type-selftests.c passed, since we have checks for these:
>
>   /* Test <, <=, >, >=.  */
>   {
>     constexpr off_A o1 = (off_A) 10;
>     constexpr off_A o2 = (off_A) 20;
>
>     static_assert (o1 < o2, "");
>     static_assert (!(o2 < o1), "");
>
>     static_assert (o2 > o1, "");
>     static_assert (!(o1 > o2), "");
>
>     static_assert (o1 <= o2, "");
>     static_assert (!(o2 <= o1), "");
>
>     static_assert (o2 >= o1, "");
>     static_assert (!(o1 >= o2), "");
>
>     static_assert (o1 <= o1, "");
>     static_assert (o1 >= o1, "");
>   }

Thanks for the review.

Yeah, I was surprised too, as did basically the same things you did to
investigate this (and came up with the conclusion).

> I changed these to SELF_CHECK, stuck a gdb_assert(false) in the operator
> definition (in the DEFINE_OFFSET_REL_OP macro), and the selftest still runs
> without any error.
>
> And if you just remove them (the DEFINE_OFFSET_REL_OP macro and its usages),
> the compiler is perfectly happy.  So I'm starting to think this operator
> definition is not used nor needed.  The important thing is that the compiler
> rejects comparisons between different offset types, such as what is tested here:
>
>   CHECK_VALID (false, void,   off_A {} < off_B {});
>
> but if the compiler is able to generate a default comparison operator between
> two operands of the same offset type, then I don't think we need to provide
> one explicitly.

Yeah, that's exactly what I thought.  I was actually going to propose
the removal of the comparison operator in the patch, but I wasn't 100%
sure that it is *really* not needed in all cases.  I mean, it's clearly
not needed in our current cases.

> Therefore, I think we could just remove the relational operator definitions
> entirely.

OK, I'll go with that, then.  I'll submit a patch for that soon (have
some errands to run right now).

Thanks,
  

On Friday, October 26 2018, Kevin Buettner wrote:

> On Thu, 25 Oct 2018 17:10:08 -0400
> Sergio Durigan Junior <sergiodj@redhat.com> wrote:
>
>> While doing something else, I noticed that the OFFSET_TYPE's
>> "DEFINE_OFFSET_REL_OP" has a thinko: it is comparing 'lhs' against
>> itself, instead of against 'rhs'.  This patch fixes it.
>> 
>> I also found an interesting thing.  We have an unittest for
>> offset-type, and in theory it should have caught this problem, because
>> it has tests for relational operators.  However, the tests
>> successfully pass, and after some investigation I'm almost sure this
>> is because these operators are not being properly overloaded.  I tried
>> a few things to make them be used, without success.  If someone wants
>> to give this a try, I'd appreciate.
>> 
>> No regressions introduced.
>> 
>> gdb/ChangeLog:
>> 2018-10-25  Sergio Durigan Junior  <sergiodj@redhat.com>
>> 
>> 	* common/offset-type.h (DEFINE_OFFSET_REL_OP): Compare 'lhs'
>> 	against 'rhs', instead of with 'lhs' again.
>
> LGTM.
>
> (I'm surprised that it wasn't caught by the unit test or by someone
> else noticing a bug elsewhere in GDB.)

Thanks for the review, Kevin.

According to my discussion with Simon, the proposed approach (for which
I'll submit a new patch soon) is to entirely remove the overloads for
relational operators, since they're clearly not being used.

Thanks,
  

On 2018-10-26 12:29, Sergio Durigan Junior wrote:
>> Therefore, I think we could just remove the relational operator 
>> definitions
>> entirely.
> 
> OK, I'll go with that, then.  I'll submit a patch for that soon (have
> some errands to run right now).

We just need confirmation from Pedro that this is ok and we're not 
missing anything important here.

Simon
  

On 10/26/2018 07:23 PM, Simon Marchi wrote:
> On 2018-10-26 12:29, Sergio Durigan Junior wrote:
>>> Therefore, I think we could just remove the relational operator definitions
>>> entirely.
>>
>> OK, I'll go with that, then.  I'll submit a patch for that soon (have
>> some errands to run right now).
> 
> We just need confirmation from Pedro that this is ok and we're not missing anything important here.
I don't recall why I added that.  Probably just assumed blindly
that it was needed.

I think the functions aren't called because they are templates, and
thus the built-in (non-template) versions take preference.  If you
make them non-templates, then they should be called.  But, the
built-ins are fine, so yeah, we can just remove the
custom definitions.

Thanks,
Pedro Alves
  

On 10/29/2018 08:10 PM, Pedro Alves wrote:
> On 10/26/2018 07:23 PM, Simon Marchi wrote:
>> On 2018-10-26 12:29, Sergio Durigan Junior wrote:
>>>> Therefore, I think we could just remove the relational operator definitions
>>>> entirely.
>>> OK, I'll go with that, then.  I'll submit a patch for that soon (have
>>> some errands to run right now).
>> We just need confirmation from Pedro that this is ok and we're not missing anything important here.
> I don't recall why I added that.  Probably just assumed blindly
> that it was needed.
> 
> I think the functions aren't called because they are templates, and
> thus the built-in (non-template) versions take preference.  If you

precedence

> make them non-templates, then they should be called.  But, the
> built-ins are fine, so yeah, we can just remove the
> custom definitions.