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

Jan Kratochvil
2011-07-20T20:53:35Z

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