From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15416 invoked by alias); 3 Feb 2010 04:36:42 -0000 Received: (qmail 15407 invoked by uid 22791); 3 Feb 2010 04:36:40 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 03 Feb 2010 04:36:35 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o134aYw3004619 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 2 Feb 2010 23:36:34 -0500 Received: from qcore.mollernet.net (vpn-8-240.rdu.redhat.com [10.11.8.240]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o134aXT9025974; Tue, 2 Feb 2010 23:36:33 -0500 Message-ID: <4B68FD51.9090209@redhat.com> Date: Wed, 03 Feb 2010 04:36:00 -0000 From: Chris Moller User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b3pre) Gecko/20090513 Fedora/3.0-2.3.beta2.fc11 Thunderbird/3.0b2 MIME-Version: 1.0 To: tromey@redhat.com CC: gdb-patches@sourceware.org Subject: Re: Patch for PR 10728 References: <4B689FD2.9070300@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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 X-SW-Source: 2010-02/txt/msg00057.txt.bz2 On 02/02/10 18:57, Tom Tromey wrote: > > There are other calls to val_print in this function. > It seems like they should all be wrapped. > I was basically applying the minimum-tinkering rule, but I'll wrap the other instances. > If cp_free_statmem_stack must be called, then you need a cleanup. > However, does it really need to be called? IMO it is ok to keep a > little data on this obstack in between calls as long as it is cleared at > the next call (so that we don't leak an unbounded amount over time). > It's not a matter of leaving a little data around, it's that the obstack has to be guaranteed to have been initialised (obstack_init()) and that it's empty. I guess the obstack_init could by moved to _initialize_cp_valprint() as part of and obstack_begin(), and, actually that would likely be a good idea. I'd mis-remembered that obstack_init doesn't allocate anything until the first grow--I just checked and it actually does allocate space which, so far as I can tell, can only be recovered with an obstack_finish. But there still has to be a means of guaranteeing the obstack is empty. Is there a cleanup for stuff that gets done in _initialize_cp_valprint()? > It isn't clear to me from this patch or the explanation that > cp_initialise_statmem_stack is called at every relevant entry point. > The problem is I don't know which eps are relevant. Does anything other than C/C++ have the statics problem? > In fact it would be better not to have to do that, because auditing > this call hierarchy is tricky. > > E.g., m2-valprint.c has a call to cp_print_value_fields. Does that > matter? > I honestly don't know--all this arm-waving is explicitly to fix statics in C, and possibly exclusively C++. I haven't much of a clue what the m2-* code is for (Modular 2?) and less of an idea if it needs to deal with statics. > I didn't read the old code in depth; is there a way to make this > self-initializing? > I tried to find a way to do that, but just a couple of steps downstream from where I have the initialisation (at cp_print_value_fields) the code starts recursing. The obstack stuff /has/ to be initialised and/or guaranteed clear, and that has to happen before the recursion starts. I could have stuck the initialisation in c_val_print, but that gets called from scm-valprint.c and jv-valprint.c, but I didn't want to tinker with otherwise unaffected code paths. > Chris> - first_dont_print > Chris> - = (CORE_ADDR *) obstack_base (&dont_print_statmem_obstack); > Chris> - i = (CORE_ADDR *) obstack_next_free (&dont_print_statmem_obstack) > Chris> - - first_dont_print; > Chris> + first_dont_print = > Chris> + (CORE_ADDR *)obstack_base (&dont_print_statmem_obstack); > Chris> + i = > Chris> + obstack_object_size (&dont_print_statmem_obstack) / sizeof(CORE_ADDR); > > The formatting is weird here; the first statement doesn't appear to have > changed, but has gratuitous formatting changes, I just thought it made the stmts more readable.