From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3103 invoked by alias); 2 Feb 2004 04:06:43 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 3090 invoked from network); 2 Feb 2004 04:06:42 -0000 Received: from unknown (HELO nevyn.them.org) (66.93.172.17) by sources.redhat.com with SMTP; 2 Feb 2004 04:06:42 -0000 Received: from drow by nevyn.them.org with local (Exim 4.30 #1 (Debian)) id 1AnVM9-0002jj-N9; Sun, 01 Feb 2004 23:06:41 -0500 Date: Mon, 02 Feb 2004 04:06:00 -0000 From: Daniel Jacobowitz To: Roland McGrath Cc: gdb-patches@sources.redhat.com Subject: Re: [PATCH] add-symbol-file-from-memory command Message-ID: <20040202040641.GA9495@nevyn.them.org> Mail-Followup-To: Roland McGrath , gdb-patches@sources.redhat.com References: <200402020338.i123cQks022042@magilla.sf.frob.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <200402020338.i123cQks022042@magilla.sf.frob.com> User-Agent: Mutt/1.5.1i X-SW-Source: 2004-02/txt/msg00014.txt.bz2 On Sun, Feb 01, 2004 at 07:38:26PM -0800, Roland McGrath wrote: > This patch hasn't changed since the last time I posted it. But I'm hoping > that it will be approved this time. This support (the underlying function, > not the user command) is the last nontrivial piece required for backtraces > from system calls to work right with vanilla Linux 2.6 kernels. The only > objections previously were not apropos, and noone said anything about the > content of the code itself. If there is valid cause not to put this in > now, I hope I can see it stated clearly. > > The last time around, some said bfd_elf_bfd_from_remote_memory function was > "in flux" or "a work in progress". That function is fine as it is in BFD > and has sat there without change for a long time. If anyone intends to > change its calling convention, updating gdb to suit will be at most three > minutes work. Since the current BFD code was approved for commit as it is, > I can't see how speculations about possible future changes there warrant > holding up this gdb change now. > > I've made fresh diffs here, after testing this code with today's mainline > gdb. It still works just fine. I can't approve this patch, but I have some comments anyway. I can tell you one problem with this patch, based on my backport of it: there's an annoying/incorrect message when a program is re-run, saying: "" has disappeared; keeping its symbols This is merely an annoyance, the message is harmless but should be fixed. There also was a segfault when the objfile is released. To reproduce, I could just say "file\n" after loading one. GDB will try to xfree(""). I see that this is fixed in the patch below, so if the bits were unchanged from your last post then I must have come up with the wrong copy. > @@ -1802,6 +1810,106 @@ add_shared_symbol_files_command (char *a > #endif > } > > +/* Helper function passed to bfd_map_over_sections. */ > +static void > +build_addr_info (bfd *abfd, asection *sectp, void *info) > +{ > + struct section_addr_info *const sai = info; > + unsigned int i = 0; > + > + if ((bfd_get_section_flags (abfd, sectp) & (SEC_ALLOC|SEC_LOAD)) == 0) > + return; > + > + while (sai->other[i++].name != NULL) > + if (i == sai->num_sections) > + return; > + > + sai->other[i].addr = bfd_get_section_vma (abfd, sectp); > + sai->other[i].name = (char *) bfd_get_section_name (abfd, sectp); > + sai->other[i].sectindex = sectp->index; > +} Mind doing this in some way that isn't gratuitously quadratic? From the BFD manual aout bfd_map_over_sections: This is the preferred method for iterating over sections; an alternative would be to use a loop: | section *p; | for (p = abfd->sections; p != NULL; p = p->next) | func (abfd, p, ...) so the ->next chain is a documented interface. > + if (from_tty) > + { > + if (bfd_get_flavour (templ) != bfd_target_elf_flavour) > + error ("add-symbol-file-from-memory not supported for this target"); > + } > + else > + gdb_assert (bfd_get_flavour (templ) == bfd_target_elf_flavour); Please remove the check and the !from_tty branch. An error is fine in either case, and internal errors are not appropriate for user input. Further down you have different error behavior on !from_tty also. Is there a particular inspiration for this? > + reinit_frame_cache (); /* ??? */ Yes, this is necessary if the current cached backtrace would pass through the newly loaded object. -- Daniel Jacobowitz MontaVista Software Debian GNU/Linux Developer