From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15609 invoked by alias); 8 Sep 2018 00:19:50 -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 15595 invoked by uid 89); 8 Sep 2018 00:19:50 -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=Probably X-HELO: userp2130.oracle.com Received: from userp2130.oracle.com (HELO userp2130.oracle.com) (156.151.31.86) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 08 Sep 2018 00:19:48 +0000 Received: from pps.filterd (userp2130.oracle.com [127.0.0.1]) by userp2130.oracle.com (8.16.0.22/8.16.0.22) with SMTP id w880J456177778; Sat, 8 Sep 2018 00:19:46 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=subject : to : cc : references : from : message-id : date : mime-version : in-reply-to : content-type : content-transfer-encoding; s=corp-2018-07-02; bh=Vw58iyb236mEict3hyCL8YsYizFp+93flGjaf0JnBmI=; b=wc+5ie6rn7vbQtbGUghz/0PzerPgYp1BxidaRWcINiDOUUySJU+mrL5E8dGHkDSWR4hw 8QL8T8QXjANL2q9jlgUC8023NVBunLAPqJzF923fcS9zhaJ4F56RHm6C2Bf/Tt35mNs4 veUQfySDbTpq/KoD2HnNg48zJJp5FL+MEeURezyai9xZ80ibxudUy1xbJUArH8EZtWom AuYP5iZ7lb8+R3fB6ZlKH+mObWz8m9D1WSoXVMJP/nk8qKkpgYF+RXVWLAd0Lh3UUkUU yYUwQVFHFM16Y0ac2KjTTMt36iYjTzXXSYAr3+OYzEaBLTMeDgGTdCuxDGfUQ3rzyW+6 Kw== Received: from userv0022.oracle.com (userv0022.oracle.com [156.151.31.74]) by userp2130.oracle.com with ESMTP id 2m7j6u58cb-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Sat, 08 Sep 2018 00:19:46 +0000 Received: from userv0121.oracle.com (userv0121.oracle.com [156.151.31.72]) by userv0022.oracle.com (8.14.4/8.14.4) with ESMTP id w880JjnE031338 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Sat, 8 Sep 2018 00:19:45 GMT Received: from abhmp0018.oracle.com (abhmp0018.oracle.com [141.146.116.24]) by userv0121.oracle.com (8.14.4/8.13.8) with ESMTP id w880JjFf016007; Sat, 8 Sep 2018 00:19:45 GMT Received: from [10.132.96.98] (/10.132.96.98) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Sat, 08 Sep 2018 00:19:45 +0000 Subject: Re: [PATCH v2 PR gdb/16841] virtual inheritance via typedef cannot find base To: Tom Tromey Cc: gdb-patches@sourceware.org References: <1535067835-60808-1-git-send-email-weimin.pan@oracle.com> <87d0tpvvao.fsf@tromey.com> From: Weimin Pan Message-ID: Date: Sat, 08 Sep 2018 00:19:00 -0000 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <87d0tpvvao.fsf@tromey.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2018-09/txt/msg00145.txt.bz2 Hi Tom, Thanks very much for the comments. On 9/7/2018 2:42 PM, Tom Tromey wrote: >>>>>> ">" == 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. OK, done. >>> 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. Done. > >>> +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. Done, Simone made the same comment. >>> +{ >>> + 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. OK, move check_typedef at the recursive call site and use types_equal for type comparison. > 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. Done. > >>> + { >>> + 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. Done, introduce local variable "this_v" to save its original value. > > Sorry about all the nits. This area is tricky. In any case the patch > is very close to ready. Currently working on failures from Simon's suggested test case run. Thanks again, Weimin > > Tom