From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7183 invoked by alias); 23 Jul 2013 22:50:42 -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 7164 invoked by uid 89); 23 Jul 2013 22:50:41 -0000 X-Spam-SWARE-Status: No, score=-5.1 required=5.0 tests=AWL,BAYES_50,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RDNS_NONE,SPF_HELO_PASS,SPF_PASS autolearn=no version=3.3.1 Received: from Unknown (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Tue, 23 Jul 2013 22:50:41 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r6NMoXBE001560 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 23 Jul 2013 18:50:34 -0400 Received: from valrhona.uglyboxes.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r6NMoWxb016754 (version=TLSv1/SSLv3 cipher=DHE-RSA-CAMELLIA256-SHA bits=256 verify=NO); Tue, 23 Jul 2013 18:50:33 -0400 Message-ID: <51EF08B8.2090301@redhat.com> Date: Tue, 23 Jul 2013 22:50:00 -0000 From: Keith Seitz User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: Tom Tromey CC: "gdb-patches@sourceware.org ml" Subject: Re: [RFA] Fix varobj/15166 References: <51DB3AA1.2060005@redhat.com> <87li4xxbjw.fsf@fleche.redhat.com> In-Reply-To: <87li4xxbjw.fsf@fleche.redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2013-07/txt/msg00548.txt.bz2 On 07/23/2013 01:32 PM, Tom Tromey wrote: > There were a few things I didn't understand here. > > First, which part of the test case actually triggered the crash? > It isn't obvious from the PR or the patch submission or the test. I apologize, it's buried in a bunch of other tests in there. [I found quite a few problems related to the actual PR, such as updating values outside varobj and noticing in CLI and vice-versa, so I fixed them all and added tests for them.] It's this specifically: (gdb) -var-update var1.child_of_pretty_printed_varobj where the index of child_of_pretty_printed_varobj > 2 (+ any baseclasses), e.g., if child_of_pretty_printed_varobj is the 3, 4, 5, ... item returned by the pp's children method. var1 is a C++ class/struct that has a pretty-printer installed, so its children are defined by the pretty-printer's children method. However varobj.c assumes/requires that the first (up to) three children (beyond baseclasses) are "public", "private", and "protected". No other children are permitted. ["Everything beyond the baseclasses can only be 'public', 'private', or 'protected'."] cplus_describe_child enforces this requirement. It calculates the number of children, and attempts to compute a "name" for this requested child (requests for value/expression are ignored): switch (index) { case 0: if (children[v_public] > 0) access = "public"; else if (children[v_private] > 0) access = "private"; else access = "protected"; break; case 1: if (children[v_public] > 0) { if (children[v_private] > 0) access = "private"; else access = "protected"; } else if (children[v_private] > 0) access = "protected"; break; case 2: /* Must be protected. */ access = "protected"; break; default: /* error! */ break; } gdb_assert (access); if (cname) *cname = xstrdup (access); For any requested child > 2, we end up in the default: case, which does nothing. ACCESS is NULL, and the assert triggers. Varobj simply cannot enforce the CPLUS_FAKE_CHILD thing for pretty-printed varobjs. It is only an issue for C++, where varobj.c requires/enforces these CPLUS_FAKE_CHILDren. > Keith> As a result, it strictly enforces that the immediate children of a > Keith> root varobj whose type is a class/struct must be one of the "fake" > Keith> children. When asking for an index greater than 2 (CPLUS_FAKE_CHILDren > Keith> are indices 0, 1, 2), the code asserts because of an unknown fake > Keith> child. > > It seems strange to me that the dynamic varobj case would ever end up in > this code. The path is pretty straightforward: #0 internal_error (file=0xfdbd14 "../../gdb/gdb/varobj.c", line=3887, string=0xfdbbc0 "%s: Assertion `%s' failed.") at ../../gdb/gdb/utils.c:842 #1 0x000000000080f1df in cplus_describe_child (parent=0x2284bb0, index=5, cname=0x0, cvalue=0x7fffffffd428, ctype=0x0, cfull_expression=0x0) at ../../gdb/gdb/varobj.c:3887 #2 0x000000000080f324 in cplus_value_of_child (parent=0x2284bb0, index=5) at ../../gdb/gdb/varobj.c:3928 #3 0x000000000080cdfb in value_of_child (parent=0x2284bb0, index=5) at ../../gdb/gdb/varobj.c:2839 #4 0x000000000080b837 in varobj_update (varp=0x7fffffffd598, explicit=1) at ../../gdb/gdb/varobj.c:2068 [...] > Perhaps dynamic varobjs should just dispatch to their own > "language_specific"-like object... or else it seems that all the > languages in the 'languages' array will need updates. It could dispatch, but the functionality for cplus_describe_child is capable of handling it (which is what this patch is attempting to add). Again, it is only an issue for C++ because CPLUS_FAKE_CHILD is only enforced for C++ varobjs. > Keith> +static PyObject * > Keith> +get_pretty_printed_element_index (struct varobj *var, int index) > [...] > Keith> + iter = PyObject_GetIter (children); > > It seems like calling this could implicitly cause a varobj update. > But probably I just don't understand. This does not modify the varobj, only the actual values (outside of varobj/in gdb) of the children. The user/UI can still call -varobj-update on the parent or any sibling and get notification of any changes. This could be exceptionally slow, but there is no way in the current API to rewind the iterator (if we have one saved) or access any child randomly. The alternatives are: 1) Update the root and report all the changes that occurred While this would be much more efficient, I don't think this is what MI clients will be expecting, and it is a significant departure from the current behavior. [Although that is how it originally worked: only root varobjs could be updated.] 2) Change the pretty-printer API to allow either random access to children or rewind the iterator. I also intentionally implemented it this way to avoid any such API changes. > Keith> + if (cvalue != NULL && value != NULL) > Keith> + *cvalue = v; > > I didn't understand why the condition uses 'value' here but the > assignment uses 'v'. Sometimes it's more obvious than you think: it's a typo. :-) Thank you for taking a look. Keith