Message ID | 20181025211008.12164-1-sergiodj@redhat.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 126806 invoked by alias); 25 Oct 2018 21:10:16 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 125782 invoked by uid 89); 25 Oct 2018 21:10:16 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=H*Ad:U*sergiodj, caught 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; Thu, 25 Oct 2018 21:10:15 +0000 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E7154C04A592 for <gdb-patches@sourceware.org>; Thu, 25 Oct 2018 21:10:13 +0000 (UTC) Received: from psique.yyz.redhat.com (unused-10-15-17-196.yyz.redhat.com [10.15.17.196]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9F6466051E; Thu, 25 Oct 2018 21:10:11 +0000 (UTC) From: Sergio Durigan Junior <sergiodj@redhat.com> To: GDB Patches <gdb-patches@sourceware.org> Cc: Sergio Durigan Junior <sergiodj@redhat.com> Subject: [PATCH] Fix thinko on common/offset-type.h (compare 'lhs' against 'rhs') Date: Thu, 25 Oct 2018 17:10:08 -0400 Message-Id: <20181025211008.12164-1-sergiodj@redhat.com> X-IsSubscribed: yes |
Commit Message
Sergio Durigan Junior
Oct. 25, 2018, 9:10 p.m. UTC
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(-)
Comments
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
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
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,
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,
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
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
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.
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(>)