From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32187 invoked by alias); 28 Mar 2013 01:11:22 -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 32148 invoked by uid 89); 28 Mar 2013 01:11:14 -0000 X-Spam-SWARE-Status: No, score=-3.5 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,RCVD_IN_HOSTKARMA_NO,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,TW_CP autolearn=ham version=3.3.1 Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Thu, 28 Mar 2013 01:11:11 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id C49442E5D4; Wed, 27 Mar 2013 21:11:09 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id hHFwH-a5zX1p; Wed, 27 Mar 2013 21:11:09 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 90F4D2E59E; Wed, 27 Mar 2013 21:11:09 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 7A75FC0330; Wed, 27 Mar 2013 18:11:07 -0700 (PDT) Date: Thu, 28 Mar 2013 12:29:00 -0000 From: Joel Brobecker To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: RFC: solib.c:solib_map_sections so->so_name clobbering (was: [PATCH 02/10] clean up allocation of bfd filenames) Message-ID: <20130328011107.GA17105@adacore.com> References: <87r4s83lu5.fsf@fleche.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87r4s83lu5.fsf@fleche.redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2013-03/txt/msg01050.txt.bz2 Hi Tom, > BFD requires the user to allocate the file name for a BFD. > GDB does this inconsistently: sometimes the file name is malloc'd, > sometimes not. Sometimes it is freed, sometimes not. > > This patch adds a new function that reallocated the BFD's filename > using bfd_alloc. This ties the lifetime to the BFD and removes the > need to free the filename when closing the BFD. [...] > * solib-darwin.c (darwin_solib_get_all_image_info_addr_at_init): > Free found_pathname. This patch accidently triggered a regression in the "info sharedlibrary" command for darwin targets. With gdb-7.5: (gdb) info shared From To Syms Read Shared Object Library 0x00007fff8d8129eb 0x00007fff8d8131d8 Yes (*) /usr/lib/libSystem.B.dylib 0x00007fff8e5a4320 0x00007fff8e5a91c0 Yes (*) /usr/lib/system/libcache.dylib [etc] But with the current HEAD, the solib name becomes: (gdb) info shared From To Syms Read Shared Object Library 0x00007fff8d8129eb 0x00007fff8d8131d8 Yes (*) i386:x86-64 0x00007fff8e5a4320 0x00007fff8e5a91c0 Yes (*) i386:x86-64 That's because the solist's so_name get overwritten with the shared object's bfd's filename in solib_map_sections: /* copy full path name into so_name, so that later symbol_file_add can find it. */ [...] strcpy (so->so_name, bfd_get_filename (abfd)); This used to work more or less by accident, thanks to the following piece of code in solib-darwin.c:darwin_bfd_open: /* Make sure that the filename is malloc'ed. The current filename for fat-binaries BFDs is a name that was generated by BFD, usually a static string containing the name of the architecture. */ res->filename = xstrdup (pathname); But your patch removed (justifiably) this code, which led us to use the filename created by the BFD layer. For Mach-O fat binaries, bfd explicitly uses the architecture name of the extracted object. See bfd_mach_o_fat_member_init. I don't know why, but that's not the only architecture that produces something which is not a valid filename (refer to our discussion re AIX archives and their shared objects). To me, the part that looks the most intriguing of all is why solib.c would overwrite the so_name set by the solib backend. I tried researching that, but found that it has been this way since the beginning of CVS. The comment is now OBE, since symbol_file_add is no longer responsible for creating the BFD - the solib backend is. I also verified that so_name appears to be used for no other purposes but printing or so_name matching (Eg: deciding if loading of symbols for shared library should be done or not). With that in mind, I think the overwriting of the so_name might not be needed or make much sense anymore. Removing it should fix the regression on Darwin, and also help in the case of my work with the solib-aix patch [1]. What do you (or others!) think? I am happy to test and submit a proper patch. We wouldn't be able to use a patch like this on the 7.6 branch, so for Darwin, I'd restore the xstrdup below - it would be a memory leak, but better to have a leak than not having the shared library name, IMO. Thanks! > diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c > index bc2cd79..242f8cc 100644 > --- a/gdb/solib-darwin.c > +++ b/gdb/solib-darwin.c > @@ -510,17 +510,10 @@ darwin_bfd_open (char *pathname) > gdbarch_bfd_arch_info (target_gdbarch)); > if (!res) > { > - gdb_bfd_unref (abfd); > - make_cleanup (xfree, found_pathname); > + make_cleanup_bfd_close (abfd); > error (_("`%s': not a shared-library: %s"), > - found_pathname, bfd_errmsg (bfd_get_error ())); > + bfd_get_filename (abfd), bfd_errmsg (bfd_get_error ())); > } > - > - /* Make sure that the filename is malloc'ed. The current filename > - for fat-binaries BFDs is a name that was generated by BFD, usually > - a static string containing the name of the architecture. */ > - res->filename = xstrdup (pathname); -- Joel [1] http://www.sourceware.org/ml/gdb-patches/2013-03/msg00818.html