Re: [PATCH] add-symbol-file-from-memory command

Daniel Jacobowitz
2004-02-02T04:06:43Z

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:
"<in-memory>" 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("<in-memory>"). 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
> }
> \f
> +/* 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