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

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

[Sergio Durigan Junior]

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(>)
-- 
2.17.1

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

[Kevin Buettner]

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

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

[Simon Marchi]

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

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

[Sergio Durigan Junior]

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/

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

[Sergio Durigan Junior]

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,

-- 
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: [PATCH] Fix thinko on common/offset-type.h (compare 'lhs' against 'rhs')

[Simon Marchi]

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

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

[Pedro Alves]

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

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

[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.


-- 
Thanks,
Pedro Alves

[PATCH] Remove relational operators from common/offset-type.h

[Sergio Durigan Junior]

This patch is a follow-up of:

  https://sourceware.org/ml/gdb-patches/2018-10/msg00601.html

It removes the declaration of the relational operators for
common/offset-type.h.  As it turns out, these overloads are not being
used when a new offset type is declared, because, according to Pedro
Alves:

  I think the functions aren't called because they are templates, and
  thus the built-in (non-template) versions take precedence.  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.

The patch also adjusts the comments on the code.

No regressions introduced.

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

	* common/offset-type.h (DEFINE_OFFSET_REL_OP): Delete.
	Adjust comments.
---
 gdb/ChangeLog            |  5 +++++
 gdb/common/offset-type.h | 18 +-----------------
 2 files changed, 6 insertions(+), 17 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 1cba619fd9..6fb02d26ea 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2018-10-29  Sergio Durigan Junior  <sergiodj@redhat.com>
+
+	* common/offset-type.h (DEFINE_OFFSET_REL_OP): Delete.
+	Adjust comments.
+
 2018-10-29  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>
 
 	* procfs.c: Include common/pathstuff.h.
diff --git a/gdb/common/offset-type.h b/gdb/common/offset-type.h
index b480b14406..174ad1e456 100644
--- a/gdb/common/offset-type.h
+++ b/gdb/common/offset-type.h
@@ -57,7 +57,7 @@
 /* The macro macro is all you need to know use offset types.  The rest
    below is all implementation detail.  */
 
-/* For each enum class type that you want to support relational
+/* For each enum class type that you want to support arithmetic
    operators, declare an "is_offset_type" overload that has exactly
    one parameter, of type that enum class.  E.g.,:
 
@@ -73,22 +73,6 @@
    function via ADL.
 */
 
-#define DEFINE_OFFSET_REL_OP(OP)					\
-  template<typename E,							\
-	   typename = decltype (is_offset_type (std::declval<E> ()))>	\
-  constexpr bool							\
-  operator OP (E lhs, E rhs)						\
-  {									\
-    using underlying = typename std::underlying_type<E>::type;		\
-    return (static_cast<underlying> (lhs)				\
-	    OP static_cast<underlying> (lhs));				\
-  }
-
-DEFINE_OFFSET_REL_OP(>)
-DEFINE_OFFSET_REL_OP(>=)
-DEFINE_OFFSET_REL_OP(<)
-DEFINE_OFFSET_REL_OP(<=)
-
 /* Adding or subtracting an integer to an offset type shifts the
    offset.  This is like "PTR = PTR + INT" and "PTR += INT".  */
 
-- 
2.17.1

Re: [PATCH] Remove relational operators from common/offset-type.h

[Simon Marchi]

On 2018-10-29 17:14, Sergio Durigan Junior wrote:
> This patch is a follow-up of:
> 
>   https://sourceware.org/ml/gdb-patches/2018-10/msg00601.html
> 
> It removes the declaration of the relational operators for
> common/offset-type.h.  As it turns out, these overloads are not being
> used when a new offset type is declared, because, according to Pedro
> Alves:
> 
>   I think the functions aren't called because they are templates, and
>   thus the built-in (non-template) versions take precedence.  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.
> 
> The patch also adjusts the comments on the code.
> 
> No regressions introduced.
> 
> gdb/ChangeLog:
> 2018-10-29  Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	* common/offset-type.h (DEFINE_OFFSET_REL_OP): Delete.
> 	Adjust comments.

Thanks, LGTM.

Simon

Re: [PATCH] Remove relational operators from common/offset-type.h

[Sergio Durigan Junior]

On Monday, October 29 2018, Simon Marchi wrote:

> On 2018-10-29 17:14, Sergio Durigan Junior wrote:
>> This patch is a follow-up of:
>>
>>   https://sourceware.org/ml/gdb-patches/2018-10/msg00601.html
>>
>> It removes the declaration of the relational operators for
>> common/offset-type.h.  As it turns out, these overloads are not being
>> used when a new offset type is declared, because, according to Pedro
>> Alves:
>>
>>   I think the functions aren't called because they are templates, and
>>   thus the built-in (non-template) versions take precedence.  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.
>>
>> The patch also adjusts the comments on the code.
>>
>> No regressions introduced.
>>
>> gdb/ChangeLog:
>> 2018-10-29  Sergio Durigan Junior  <sergiodj@redhat.com>
>>
>> 	* common/offset-type.h (DEFINE_OFFSET_REL_OP): Delete.
>> 	Adjust comments.
>
> Thanks, LGTM.

Thanks, pushed.

fd332753fa7050bb9d7c89147e32d285099fe402

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