From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31223 invoked by alias); 19 Jul 2012 14:18:12 -0000 Received: (qmail 31213 invoked by uid 22791); 19 Jul 2012 14:18:11 -0000 X-SWARE-Spam-Status: No, hits=-6.2 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,SPF_HELO_PASS,TW_BJ,T_RP_MATCHES_RCVD 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; Thu, 19 Jul 2012 14:17:58 +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.14.4/8.14.4) with ESMTP id q6JEHvCb006744 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 19 Jul 2012 10:17:57 -0400 Received: from host2.jankratochvil.net (ovpn-116-30.ams2.redhat.com [10.36.116.30]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q6JEHrWE002718 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Thu, 19 Jul 2012 10:17:56 -0400 Date: Thu, 19 Jul 2012 14:18:00 -0000 From: Jan Kratochvil To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 01/10] change gdb to refcount bfd everywhere Message-ID: <20120719141750.GB23801@host2.jankratochvil.net> References: <87vchk3lxs.fsf@fleche.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87vchk3lxs.fsf@fleche.redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) 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: 2012-07/txt/msg00336.txt.bz2 On Wed, 18 Jul 2012 21:31:59 +0200, Tom Tromey wrote: > --- /dev/null > +++ b/gdb/gdb_bfd.c > @@ -0,0 +1,90 @@ [...] > +/* Add reference to ABFD. Returns ABFD. */ > + > +struct bfd * > +gdb_bfd_ref (struct bfd *abfd) [...] > +/* Unreference and possibly close ABFD. */ > + > +void > +gdb_bfd_unref (struct bfd *abfd) [...] > --- /dev/null > +++ b/gdb/gdb_bfd.h > @@ -0,0 +1,35 @@ [...] > +/* Acquire a new reference to ABFD. Returns ABFD for convenience. > + It is fine for ABFD to be NULL; in this case the function does > + nothing and returns NULL. */ > + > +struct bfd *gdb_bfd_ref (struct bfd *abfd); > + > +/* Release a reference to ABFD. If this is the last reference, ABFD > + will be freed. If ABFD is NULL, this function does nothing. */ > + > +void gdb_bfd_unref (struct bfd *abfd); The comments are present in both and neither is a reference, they are already out of sync. > diff --git a/gdb/jit.c b/gdb/jit.c > index 568d17b..6a1425c 100644 > --- a/gdb/jit.c > +++ b/gdb/jit.c > @@ -861,6 +862,7 @@ jit_bfd_try_read_symtab (struct jit_code_entry *code_entry, > puts_unfiltered (_("Error opening JITed symbol file, ignoring it.\n")); > return; > } > + nbfd = gdb_bfd_ref (nbfd); I personally disagree about the returned value from gdb_bfd_ref being useful, it makes the code more magic IMO (plus it is not much compatible with the narrow GNU Coding Standard code formatting). But when it is already this way then this gdb_bfd_ref wrapped should have been around bfd_open_from_target_memory above as gdb_bfd_ref can handle NULL parameter. > @@ -193,9 +194,9 @@ allocate_objfile (bfd *abfd, int flags) > > /* Update the per-objfile information that comes from the bfd, ensuring > that any data that is reference is saved in the per-objfile data > - region. */ > + region. Note that we steal a reference to ABFD. */ > > - objfile->obfd = gdb_bfd_ref (abfd); > + objfile->obfd = abfd; Caller could gdb_bfd_unref its reference but YMMV. > @@ -794,10 +795,10 @@ add_vmap (LdInfo *ldi) > { > warning (_("\"%s\": not in executable format: %s."), > objname, bfd_errmsg (bfd_get_error ())); > - bfd_close (abfd); > + gdb_bfd_unref (abfd); > return NULL; > } > - obj = allocate_objfile (vp->bfd, 0); > + obj = allocate_objfile (gdb_bfd_ref (vp->bfd), 0); This code is a bit magic due to it. map_vmap is missing gdb_bfd_ref despite it creates new reference etc. This all pain would not exist with C++. > @@ -2519,14 +2512,10 @@ reread_symbols (void) > to close the descriptor but BFD lacks a way of closing the > BFD without closing the descriptor. */ > obfd_filename = bfd_get_filename (objfile->obfd); > - if (!bfd_close (objfile->obfd)) > - error (_("Can't close BFD for %s: %s"), objfile->name, > - bfd_errmsg (bfd_get_error ())); > + gdb_bfd_unref (objfile->obfd); > objfile->obfd = bfd_open_maybe_remote (obfd_filename); > if (objfile->obfd == NULL) > error (_("Can't open %s to read symbols."), objfile->name); > - else > - objfile->obfd = gdb_bfd_ref (objfile->obfd); Why isn't gdb_bfd_ref missing here? > /* bfd_openr sets cacheable to true, which is what we want. */ > if (!bfd_check_format (objfile->obfd, bfd_object)) > error (_("Can't read symbols from %s: %s."), objfile->name, Thanks, Jan