From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18858 invoked by alias); 7 Sep 2018 21:42:44 -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 18845 invoked by uid 89); 7 Sep 2018 21:42:43 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.4 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-HELO: gateway31.websitewelcome.com Received: from gateway31.websitewelcome.com (HELO gateway31.websitewelcome.com) (192.185.143.40) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 07 Sep 2018 21:42:41 +0000 Received: from cm10.websitewelcome.com (cm10.websitewelcome.com [100.42.49.4]) by gateway31.websitewelcome.com (Postfix) with ESMTP id 837ABE4E32 for ; Fri, 7 Sep 2018 16:42:40 -0500 (CDT) Received: from box5379.bluehost.com ([162.241.216.53]) by cmsmtp with SMTP id yOWWfo8UBBcCXyOWWfFu72; Fri, 07 Sep 2018 16:42:40 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=tromey.com; s=default; h=Content-Type:MIME-Version:Message-ID:In-Reply-To:Date: References:Subject:Cc:To:From:Sender:Reply-To:Content-Transfer-Encoding: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=seDBn+BAHrgOfp0j7U6x/6Oy9PRSRwllAzDHKvb+r/g=; b=mJJeUnVjFsK83z9qQqIpK8oCBG qYpcft02F3l0i3alADRyQdDGBgai/sqW4EfgK7S4Fn5aLBkkXyzvZEiHKrU1nRknHJ5c/CGBgM1FM LOL6pzxG/qjsYnjHOwQTG13o+; Received: from 75-166-85-72.hlrn.qwest.net ([75.166.85.72]:51288 helo=pokyo) by box5379.bluehost.com with esmtpsa (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.91) (envelope-from ) id 1fyOWW-001p8S-9C; Fri, 07 Sep 2018 16:42:40 -0500 From: Tom Tromey To: Weimin Pan Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v2 PR gdb/16841] virtual inheritance via typedef cannot find base References: <1535067835-60808-1-git-send-email-weimin.pan@oracle.com> Date: Fri, 07 Sep 2018 21:42:00 -0000 In-Reply-To: <1535067835-60808-1-git-send-email-weimin.pan@oracle.com> (Weimin Pan's message of "Thu, 23 Aug 2018 18:43:55 -0500") Message-ID: <87d0tpvvao.fsf@tromey.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1.50 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-SW-Source: 2018-09/txt/msg00129.txt.bz2 >>>>> ">" == Weimin Pan writes: >> Finding data member in virtual base class >> This patch fixes the original problem - printing member in a virtual base, >> using various expressions, do not yield the same value. A simple test case >> below demonstrates the problem: Thank you for the patch. >> diff --git a/gdb/testsuite/gdb.cp/virtbase2.cc b/gdb/testsuite/gdb.cp/virtbase2.cc >> new file mode 100644 >> index 0000000..4f7631e >> --- /dev/null >> +++ b/gdb/testsuite/gdb.cp/virtbase2.cc >> @@ -0,0 +1,21 @@ >> +struct base { New test cases should come with the GPL header. You can copy one from some other .cc file in gdb.cp, just make sure to fix the copyright line. >> index 9bdbf22..754e7d0 100644 >> --- a/gdb/valops.c >> +++ b/gdb/valops.c >> @@ -3329,6 +3329,35 @@ compare_parameters (struct type *t1, struct type *t2, int skip_artificial) >> return 0; >> } >> +/* C++: Given an aggregate type VT, and a class type CLS, >> + search recursively for CLS and return its offset, >> + relative to VT, if it is a virtual base member. */ The comment says this "return"s the offset, but really it stores the offset in BOFFS and returns a boolean. The comment should explain this. >> +static int >> +add_virtual_base_offset (struct type *vt, struct type *cls, >> + struct value *v, int &boffs) I think the function is misnamed, because it doesn't actually add anything. Also, gdb style is not to use non-const references for out parameters. Instead make "boffs" a pointer. >> +{ >> + for (int i = 0; i < TYPE_N_BASECLASSES (vt); i++) >> + { >> + struct type *ftype = TYPE_FIELD_TYPE (vt, i); >> + if (check_typedef (ftype) == cls) You probably want types_equal rather than == here. The difference is that two types will not be == if they came from different shared libraries. However, they may in fact be "the same" type from the user point of view. You don't have to call check_typedef for types_equal. However I do think you have to call check_typedef at the recursion spot. I recall there being bugs in the past where gdb didn't check_typedef a base type and ran into problems. >> @@ -3393,6 +3422,15 @@ value_struct_elt_for_reference (struct type *domain, int offset, >> tmp = lookup_pointer_type (TYPE_SELF_TYPE (type)); >> v = value_cast_pointers (tmp, v, 1); >> mem_offset = value_as_long (ptr); >> + if (domain != curtype) Probably types_equal here too. >> + { >> + struct value *v2 = value_of_this_silent (current_language); Hah, I was going to write that this is incorrect, but I see it is in the code already. I suppose this makes sense, because this whole code block is about the implicit this case. Which seems like the wrong place to put it, but none of this is your problem. However, rather than calling value_of_this_silent for a second time, just save the original value and re-use it. Sorry about all the nits. This area is tricky. In any case the patch is very close to ready. Tom