From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22380 invoked by alias); 30 Aug 2011 11:47:58 -0000 Received: (qmail 22371 invoked by uid 22791); 30 Aug 2011 11:47:57 -0000 X-SWARE-Spam-Status: No, hits=-6.6 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_BJ 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; Tue, 30 Aug 2011 11:47:37 +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 p7UBlaKB032406 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 30 Aug 2011 07:47:36 -0400 Received: from localhost.localdomain (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 p7UBlYot006856; Tue, 30 Aug 2011 07:47:35 -0400 From: Phil Muldoon To: matt rice Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 2/7] [python] API for macros: memory management quirks. References: <1314198654-9008-1-git-send-email-ratmice@gmail.com> <1314198654-9008-3-git-send-email-ratmice@gmail.com> Reply-to: pmuldoon@redhat.com X-URL: http://www.redhat.com Date: Tue, 30 Aug 2011 11:47:00 -0000 In-Reply-To: <1314198654-9008-3-git-send-email-ratmice@gmail.com> (matt rice's message of "Wed, 24 Aug 2011 08:10:49 -0700") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-IsSubscribed: yes 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: 2011-08/txt/msg00592.txt.bz2 matt rice writes: > diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c > index f66c1d9..c56f431 100644 > --- a/gdb/dwarf2read.c > +++ b/gdb/dwarf2read.c > @@ -14351,8 +14351,7 @@ macro_start_file (int file, int line, > /* We don't create a macro table for this compilation unit > at all until we actually get a filename. */ > if (! pending_macros) > - pending_macros = new_macro_table (&objfile->objfile_obstack, > - objfile->macro_cache); > + pending_macros = new_macro_table (objfile, macro_table_type_from_objfile); The comments in macrotab.h need to be updated with the new parameter info. This and others. This is code I am not very sure of, having never dabbled in this area. So this review would benefit from a maintainer that is more familiar with this code. But what benefit do we accrue now that we purely store macros in the objfile's obstack over (the now defunct) bcache? Was there an underlying reason for dual this you discovered? It seems odd that in the pre-patched version that struct macro_table had two strategies for this. Contextually, why did you elect to use the obstack in the objfile, instead of the previous ancillary related structure? > diff --git a/gdb/macrotab.c b/gdb/macrotab.c > index efcf835..b2825d2 100644 > --- a/gdb/macrotab.c > +++ b/gdb/macrotab.c > @@ -35,13 +35,10 @@ > > struct macro_table > { > - /* The obstack this table's data should be allocated in, or zero if > - we should use xmalloc. */ > - struct obstack *obstack; > - > - /* The bcache we should use to hold macro names, argument names, and > - definitions, or zero if we should use xmalloc. */ > - struct bcache *bcache; > + /* The objfile who's obstack this table's data should be allocated in, > + and bcache we should use to hold macro names, argument > + names, and definitions, or zero if we should use xmalloc. */ > + struct objfile *objfile; Comment seems wrong, as bcache data structure no longer exists. Additionally, (not in the code, but in your email), a narrative why we are removing the bcache is needed. > static void > macro_bcache_free (struct macro_table *t, void *obj) > { > - if (t->bcache) > + if (t->objfile) > /* There are cases where we need to remove entries from a macro > table, even when reading debugging information. This should be > rare, and there's no easy way to free data from a bcache, so we > @@ -147,6 +146,12 @@ macro_bcache_free (struct macro_table *t, void *obj) > xfree (obj); > } Updated comments needed. Referral to bcache. > +/* Create a new, empty macro table. Allocate it in OBJFILE's obstack, > + or use xmalloc if OBJFILE is zero. Use OBJFILE's bcache to store > + all macro names, arguments, definitions, and anything else that > + might be the same amongst compilation units in an executable file; > + if OBJFILE is zero, don't cache these things. > + > + Note that, if OBJFILE is non-zero, then removing information from the > + table may leak memory. Neither obstacks nor bcaches really allow > + you to remove information, so although we can update the data > + structure to record the change, we can't free the old data. > + At the moment, since we only provide obstacks and bcaches for macro > + tables for symtabs, this isn't a problem; only odd debugging > + information makes a definition and then deletes it at the same > + source location (although 'gcc -DFOO -UFOO -DFOO=2' does do that > + in GCC 4.1.2.). */ > +struct macro_table *new_macro_table (struct objfile *objfile, > + enum macro_table_type table_type); Comments need to be updated. Cheers, Phil