Re: [PATCH 2/7] gdb: add 'lazy' setting for command 'set debuginfod enabled'

[Andrew Burgess <aburgess@redhat.com>]

Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:

> 'set debuginfod enabled lazy' turns on debuginfod downloading like
> 'set debuginfod enabled on' but also enables ELF/DWARFs section
> downloading via debuginfod_section_query.

If I understand correctly, 'lazy' can still download the full files
(when needed), but might first download some of the sections (e.g. the
index), which might allow us to skip downloading the full files?

I'm not sure that adding the 'lazy' setting as another option is the
correct way to go.  I'll explain more below...

>
> If support for debuginfod section queries was not found at configure
> time, 'set debuginfod enabled lazy' will print an error message
> indicating the missing support and default to 'set debuginfod enabled on'.
>
> Also update the help text and gdb.texinfo section for 'set debuginfod enabled'
> with information on the lazy setting.
> ---
>  gdb/debuginfod-support.c | 20 +++++++++++++++++---
>  gdb/doc/gdb.texinfo      |  9 +++++++--
>  2 files changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
> index f909742362f..12025fcf0c0 100644
> --- a/gdb/debuginfod-support.c
> +++ b/gdb/debuginfod-support.c
> @@ -34,12 +34,14 @@ static cmd_list_element *show_debuginfod_prefix_list;
>  static const char debuginfod_on[] = "on";
>  static const char debuginfod_off[] = "off";
>  static const char debuginfod_ask[] = "ask";
> +static const char debuginfod_lazy[] = "lazy";
>  
>  static const char *debuginfod_enabled_enum[] =
>  {
>    debuginfod_on,
>    debuginfod_off,
>    debuginfod_ask,
> +  debuginfod_lazy,
>    nullptr
>  };
>  
> @@ -411,7 +413,7 @@ debuginfod_section_query (const unsigned char *build_id,
>    return scoped_fd (-ENOSYS);
>  #else
>  
> - if (!debuginfod_is_enabled ())
> + if (debuginfod_enabled != debuginfod_lazy || !debuginfod_is_enabled ())
>      return scoped_fd (-ENOSYS);

Because you check for debuginfod_lazy first, and due to the
short-circuit evaluation, then if debuginfod_enabled is set to 'on',
'off', or 'ask' we will always return -ENOSYS.

As the default is 'ask' this feature is off by default, which I think is
a shame.

Even if we could call debuginfod_is_enabled here, and it asked the user,
it would just set the value to 'on', which apparently still isn't good
enough.

I would suggest that this option (debuginfod_enabled) should remain as a
yes/no/ask choice.  If the user selects 'yes', and section downloading
is supported by the current version of debuginfod then we should just do
that.  I don't think this level of control is something that users will
normally care about -- they'll either be OK with downloading stuff (and
select 'on'), or they are against downloading stuff (and select 'off').
I doubt there are many users who say ... I'm OK downloading full debug
files, but not the individual sections.

You could possibly add a new setting to individually control downloading
the sections, though I'm so convinced that this isn't something a user
will care about that I would suggest this should be a maintenance
setting:

  maintenance set debuginfod download-sections on
  maintenance set debuginfod download-sections off
  maintenance show debuginfod download-sections

We can always move maintenance commands into the user space later if
needed,

>  
>    debuginfod_client *c = get_debuginfod_client ();
> @@ -451,6 +453,14 @@ static void
>  set_debuginfod_enabled (const char *value)
>  {
>  #if defined(HAVE_LIBDEBUGINFOD)
> +#if !defined(HAVE_DEBUGINFOD_FIND_SECTION)
> +  if (value == debuginfod_lazy)
> +    {
> +      debuginfod_enabled = debuginfod_on;
> +      error (_("Support for lazy downloading is not compiled into GDB. " \
> +"Defaulting to \"on\"."));
> +    }
> +#endif
>    debuginfod_enabled = value;
>  #else
>    /* Disabling debuginfod when gdb is not built with it is a no-op.  */
> @@ -550,8 +560,12 @@ _initialize_debuginfod ()
>  			_("Set whether to use debuginfod."),
>  			_("Show whether to use debuginfod."),
>  			_("\
> -When on, enable the use of debuginfod to download missing debug info and\n\
> -source files."),
> +When set to \"on\", enable the use of debuginfod to download missing\n\
> +debug info and source files. \"off\" disables the use of debuginfod.\n\
> +When set to \"ask\", a prompt may ask whether to enable or disable\n\
> +debuginfod.  When set to \"lazy\", debug info downloading will be\n\
> +deferred until it is required. GDB may also download components of\n\
> +debug info instead of entire files." ),
>  			set_debuginfod_enabled,
>  			get_debuginfod_enabled,
>  			show_debuginfod_enabled,
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index c1ca45521ea..ac0636e3115 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -48757,10 +48757,15 @@ debug info or source files.  By default, @code{debuginfod enabled} is set to
>  attempting to perform the next query.  By default, @code{debuginfod enabled}
>  is set to @code{ask} for interactive sessions.
>  
> +@item set debuginfod enabled lazy
> +@value{GDBN} will attempt to defer downloading entire debug info files until
> +necessary. @value{GDBN} may instead download individual components of the
> +debug info files such as @code{.gdb_index}.

Given how the code is currently written I would expect some changes to
'on' to make it clear that some functionality is missing in this mode.

But if you take my advice then the docs would end up different anyway..

Thanks,
Andrew

> +
>  @kindex show debuginfod enabled
>  @item show debuginfod enabled
> -Display whether @code{debuginfod enabled} is set to @code{on}, @code{off} or
> -@code{ask}.
> +Display whether @code{debuginfod enabled} is set to @code{on}, @code{off},
> +@code{ask} or @code{lazy}.
>  
>  @kindex set debuginfod urls
>  @cindex configure debuginfod URLs
> -- 
> 2.39.2