This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Fix thinko on common/offset-type.h (compare 'lhs' against 'rhs')
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/