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

[Simon Marchi <simon dot marchi at ericsson dot com>]

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