Re: [PATCH] Fix thinko on common/offset-type.h (compare 'lhs' against 'rhs')

[Sergio Durigan Junior <sergiodj@redhat.com>]

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,

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