From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 100244 invoked by alias); 26 Oct 2018 16:29:24 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 100224 invoked by uid 89); 26 Oct 2018 16:29:23 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 26 Oct 2018 16:29:22 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D0DEC3082B61; Fri, 26 Oct 2018 16:29:20 +0000 (UTC) Received: from localhost (unused-10-15-17-196.yyz.redhat.com [10.15.17.196]) by smtp.corp.redhat.com (Postfix) with ESMTP id 38EBD67007; Fri, 26 Oct 2018 16:29:20 +0000 (UTC) From: Sergio Durigan Junior To: Simon Marchi Cc: GDB Patches , Pedro Alves Subject: Re: [PATCH] Fix thinko on common/offset-type.h (compare 'lhs' against 'rhs') References: <20181025211008.12164-1-sergiodj@redhat.com> Date: Fri, 26 Oct 2018 16:29:00 -0000 In-Reply-To: (Simon Marchi's message of "Fri, 26 Oct 2018 16:08:23 +0000") Message-ID: <87o9bgeko0.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2018-10/txt/msg00625.txt.bz2 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 >> >> * 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 >> + >> + * common/offset-type.h (DEFINE_OFFSET_REL_OP): Compare 'lhs' >> + against 'rhs', instead of with 'lhs' again. >> + >> 2018-10-25 Andrew Burgess >> >> * 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::type; \ >> return (static_cast (lhs) \ >> - OP static_cast (lhs)); \ >> + OP static_cast (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/