Re: [PATCH] On-demand loading of shlib's debuginfo

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

Hi Sergio,

it crashes for me on fedora-rawhide-x86_64:
$ echo 'main(){pause();}'|gcc -g -x c -;./gdb -nx ./a.out -ex 'set solib-add lazy' -ex start -ex 'list pause'
[...]
#3  in extract_typed_address (buf=0x10 <Address 0x10 out of bounds>, type=0x2000d60) at findvar.c:180
#4  in lm_dynamic_from_link_map (so=0x2009c00) at solib-svr4.c:169
[...]


On Tue, 19 Jul 2011 00:23:49 +0200, Sergio Durigan Junior wrote:
> With that in mind, we decided to tackle this problem progressively, and the
> first part of the solution is ready for submission.

That is currently it still touches the solib files on disk for the purpose of
solib_map_sections so that will be a different patch, looking forward.


> 1) Change the name of the `auto-solib-add' command to just `solib-add'
> (deprecatin `auto-solib-add').  Also, make the command be a tri-state,
> with `on', `off', and `lazy' options.  The default option is still `on'.

I do not understand why you haven't kept the original name, just extending it
to be tri-state.  Otherwise the auto_solib_add -> solib_add_opt rename could
be in a separate mechanical patch.


> 3) This patch only affects the `backtrace' command for now,

Do you have some benchmark of the benefits?  But without the
solib_map_sections part it may not yet been so significant I guess.


> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -4593,9 +4593,9 @@ bpstat_what (bpstat bs_head)
>        target_terminal_ours_for_output ();
>  
>  #ifdef SOLIB_ADD
> -      SOLIB_ADD (NULL, 0, &current_target, auto_solib_add);
> +      SOLIB_ADD (NULL, 0, &current_target, solib_add_opt, /*lazy_read=*/0);

nitpick: I find - except one such case - in GDB the style: , 0 /* lazy_read */);
+everywhere.


> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -14948,14 +14948,18 @@ will be loaded automatically when the inferior begins execution, you
>  attach to an independently started inferior, or when the dynamic linker
>  informs @value{GDBN} that a new library has been loaded.  If @var{mode}
>  is @code{off}, symbols must be loaded manually, using the
> -@code{sharedlibrary} command.  The default value is @code{on}.
> +@code{sharedlibrary} command.  If @var{mode} is @code{lazy}, then
> +@value{GDBN} will lazily load symbols from shared libraries, i.e., it
> +will load symbols on-demand.  The default value is @code{on}.

The crash reproducer at the top should have shown the "lazy" mode has limited
applicability now - which is also probably why it is not a default.

The lazy hook is now there only for backtrace so the documentation should
describe it works only for backtraces.  It could be applied to /usr/bin/gstack
but that is a Fedora only command/patch so far.


> +This command is now deprecated.  Use @code{set solib-add} instead.

This `auto-solib-add' gets deprecated but I do not see the new `solib-add'
setting described in the doc.


> --- a/gdb/solib-frv.c
> +++ b/gdb/solib-frv.c
> @@ -1302,7 +1302,7 @@ frv_fetch_objfile_link_map (struct objfile *objfile)
>  
>    /* Cause frv_current_sos() to be run if it hasn't been already.  */
>    if (main_lm_addr == 0)
> -    solib_add (0, 0, 0, 1);
> +    solib_add (0, 0, 0, 1, /*lazy_read=*/0);

1 should be SOLIB_ADD_ON instead.

 
> --- a/gdb/solib-svr4.c
> +++ b/gdb/solib-svr4.c
> @@ -2449,6 +2451,29 @@ elf_lookup_lib_symbol (const struct objfile *objfile,
>    return lookup_global_symbol_from_objfile (objfile, name, domain);
>  }
>  
> +/* Given PC, try to match a so_list which contains it.
> +   See match_pc_solist in solist.h.  */
> +
> +static struct so_list *
> +svr4_match_pc_solist (CORE_ADDR pc, struct so_list *so)
> +{
> +  struct so_list *iter, *res = NULL;
> +  CORE_ADDR cur = CORE_ADDR_MAX;
> +
> +  for (iter = so; iter; iter = iter->next)
> +    {
> +      CORE_ADDR addr = lm_dynamic_from_link_map (iter);
> +
> +      if (addr >= pc && addr < cur)

Maybe a note "cur" is in the DYNAMIC segment while PC is in the LOAD segment
and normally LOAD segment is under DYNAMIC segment; which is why the
conditional looks like reversed.


> +	{
> +	  cur = addr;
> +	  res = iter;
> +	}
> +    }
> +
> +  return res;
> +}
> +

> --- a/gdb/solib.c
> +++ b/gdb/solib.c
> @@ -888,6 +900,19 @@ libpthread_solib_p (struct so_list *so)
>    return libpthread_name_p (so->so_name);
>  }
>  
> +void
> +solib_on_demand_load (CORE_ADDR pc)

Missing function comment.


> +{
> +  struct so_list *solib;
> +
> +  /* On-demand loading of shared libraries' debuginfo.  */
> +  solib = solib_match_pc_solist (pc);
> +
> +  if (solib && !solib->symbols_loaded)
> +    solib_add (solib->so_name, 0, &current_target, 1,
> +	       /*lazy_read=*/1);

1 should be SOLIB_ADD_ON instead.


> +}
> +
>  /* GLOBAL FUNCTION
>  
>     solib_add -- read in symbol info for newly added shared libraries
> @@ -967,7 +1006,7 @@ solib_add (char *pattern, int from_tty,
>        printf_unfiltered
>  	("No loaded shared libraries match the pattern `%s'.\n", pattern);
>  
> -    if (loaded_any_symbols)
> +    if (loaded_any_symbols && !lazy_read)

I think it can break sunos_special_symbol_handling, that should still be
called there or to disable lazy_read functionality on sunos or so.


>        {
>  	struct target_so_ops *ops = solib_ops (target_gdbarch);
>  
> @@ -1265,6 +1304,28 @@ in_solib_dynsym_resolve_code (CORE_ADDR pc)
>    return ops->in_dynsym_resolve_code (pc);
>  }
>  
> +/* GLOBAL FUNCTION
> +
> +   solib_match_pc_solist -- check to see to which so_list PC belongs.
> +
> +   SYNOPSIS
> +
> +   struct so_list *solib_match_pc_solist (CORE_ADDR pc)
> +
> +   DESCRIPTION
> +
> +   Determine to which so_list the given PC belongs.  Returns the
> +   so_list if found, NULL otherwise.
> +*/
> +
> +struct so_list *
> +solib_match_pc_solist (CORE_ADDR pc)
> +{
> +  struct target_so_ops *ops = solib_ops (target_gdbarch);
> +
> +  return ops->match_pc_solist (pc, so_list_head);

On solib platforms where match_pc_solist is not defined it will crash.


> +}
> +
>  /*
>  
>     LOCAL FUNCTION
> @@ -1283,7 +1344,7 @@ static void
>  sharedlibrary_command (char *args, int from_tty)
>  {
>    dont_repeat ();
> -  solib_add (args, from_tty, (struct target_ops *) 0, 1);
> +  solib_add (args, from_tty, (struct target_ops *) 0, 1, /*lazy_read=*/0);

1 should be SOLIB_ADD_ON instead.



Thanks,
Jan