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

[Daniel Jacobowitz <drow@mvista.com>]

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