Re: [PATCH 1/2] gdb: add set/show commands for managing debuginfod

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2021-10-18 19:01, Aaron Merey via Gdb-patches wrote:
> Add 'set/show debuginfod' command.  Accepts "on", "off" or "ask" as
> an argument.  "on" enables debuginfod for the current session.  "off"
> disables debuginfod for the current session.  "ask" will prompt
> the user to either enable or disable debuginfod when the next query
> is about to be performed:
> 
>   This GDB supports auto-downloading debuginfo from the following URLs:
>   <URL1> <URL2> ...
>   Enable debuginfod? (y or [n]) y
>   Debuginfod has been enabled.
>   To make this setting permanent, add 'set debuginfod on' to .gdbinit.
> 
> For interactive sessions, "ask" is the default. For non-interactive
> sessions, "no" is the default.
> 
> Also add 'set/show debuginfod-urls' command. Accepts a string of
> space-separated debuginfod server URLs to be queried. The default
> value is copied from $DEBUGINFOD_URLS.

I would suggest making "set debuginfod" a prefix command and have "set
debuginfod urls".  This is how we "namespace" commands.  This allows
doing "help set debuginfod" so see help of everything
debuginfod-related.  So you would have:

 - set debuginfod on/off/ask
 - set debuginfod urls <URLS>

This is how it's done for "set index-cache", for example, I think that
works well / is intuitive.

> Finally add 'set debug debuginfod" command to control whether
> debuginfod-related debugging output is displayed. This debugging
> output is displayed by default.
> 
>   (gdb) run
>   Starting program: /bin/sleep 5
>   Download failed: No route to host.  Continuing without debug info for /lib64/libc.so.6.

To stay consistent, any "set debug ..." output should be disabled by
default.  It's really about debug output for GDB developers.  I think it
would be useful to have a "set debug debuginfo" command, but it could
for example print low level information about each call we do to the
debuginfo lib, allowing you to diagnose problems.

What you want here is informational user messages, which could be
controlled by "set debuginfod verbose" or something like that (I don't
know if you want it to be a boolean or be able to have multiple levels
of verbosity).  Note that we do have the "set verbose" command, but I
find it rather... coarse-grained.

Edit: I now see Keith mentioned that since these messages are always
errors, they should always be printed.  I think I agree with him.

> ---
>  gdb/debuginfod-support.c                      | 151 +++++++++++++++++-
>  .../gdb.debuginfod/fetch_src_and_symbols.exp  |  25 ++-
>  2 files changed, 165 insertions(+), 11 deletions(-)
> 
> diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
> index 2d626e335a0..1a4a6e1dde0 100644
> --- a/gdb/debuginfod-support.c
> +++ b/gdb/debuginfod-support.c
> @@ -18,7 +18,6 @@
>  
>  #include "defs.h"
>  #include <errno.h>
> -#include "cli/cli-style.h"
>  #include "gdbsupport/scoped_fd.h"
>  #include "debuginfod-support.h"
>  #include "gdbsupport/gdb_optional.h"
> @@ -42,6 +41,8 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
>    return scoped_fd (-ENOSYS);
>  }
>  #else
> +#include "cli/cli-cmds.h"
> +#include "cli/cli-style.h"
>  #include <elfutils/debuginfod.h>
>  
>  struct user_data
> @@ -68,6 +69,63 @@ struct debuginfod_client_deleter
>  using debuginfod_client_up
>    = std::unique_ptr<debuginfod_client, debuginfod_client_deleter>;
>  
> +static const char debuginfod_on[] = "on";
> +static const char debuginfod_off[] = "off";
> +static const char debuginfod_ask[] = "ask";
> +static const char *const debuginfod_cmd_names[] =
> +{
> +  debuginfod_on,
> +  debuginfod_off,
> +  debuginfod_ask,
> +  NULL,

NULL -> nullptr

> +};
> +
> +static const char *debuginfod_enable = debuginfod_ask;
> +static std::string debuginfod_urls;
> +static bool debuginfod_debug = true;
> +
> +/* Show whether debuginfod is enabled.  */
> +
> +static void
> +show_debuginfod_command (struct ui_file *file, int from_tty,
> +			 struct cmd_list_element *cmd, const char *value)
> +{
> +   fprintf_unfiltered (file, _("Debuginfod functionality is currently set " \
> +		       "to \"%s\".\n"), debuginfod_enable);

There is one extra space in front of fprintf_unfiltered.

> +}
> +
> +/* Set the URLs that debuginfod will query.  */
> +
> +static void
> +set_debuginfod_urls_command (const char *args, int from_tty,
> +			     struct cmd_list_element *c)
> +{
> +  if (setenv ("DEBUGINFOD_URLS", debuginfod_urls.c_str (), 1) != 0)
> +    warning (_("Unable to set debuginfod URLs: %s"), strerror (errno));
> +}
> +
> +/* Show the URLs that debuginfod will query.  */
> +
> +static void
> +show_debuginfod_urls_command (struct ui_file *file, int from_tty,
> +			      struct cmd_list_element *cmd, const char *value)
> +{
> +  if (debuginfod_urls.empty ())
> +    fprintf_unfiltered (file, _("You have not set debuginfod URLs.\n"));
> +  else
> +    fprintf_filtered (file, _("Debuginfod URLs are currently set to:\n%s\n"),
> +		      value);
> +}

Since the source of truth for this setting is the env var, this set/show
pair would be a good use case for the new setter/getter-based commands,
where you don't have the "debuginfod_urls" global variable:

https://gitlab.com/gnutools/binutils-gdb/-/blob/a4b0231e179607e47b1cdf1fe15c5dc25e482fad/gdb/command.h#L700-705

Let's say that "set_debuginfod_urls_command" fails to set the env var
for some reason, the show_debuginfod_urls_command will still show the
contents of the debuginfod_urls global variable, which does not reflect
the reality.

The setter would use setenv, and the getter would use getenv.

> +
> +/* Show whether to display debuginfod debugging output.  */
> +
> +static void
> +show_debuginfod_debug_command (struct ui_file *file, int from_tty,
> +			       struct cmd_list_element *cmd, const char *value)
> +{
> +  fprintf_filtered (file, _("Debuginfod debugging output is %s.\n"), value);
> +}
> +
>  static int
>  progressfn (debuginfod_client *c, long cur, long total)
>  {
> @@ -120,6 +178,39 @@ get_debuginfod_client ()
>    return global_client.get ();
>  }
>  
> +/* Check if debuginfod is enabled. If configured to do so, ask the user

Two spaces after period.

> +   whether to enable debuginfod.  */
> +
> +static bool
> +debuginfod_enabled ()
> +{
> +   if (debuginfod_urls.empty ()
> +       || strcmp (debuginfod_enable, debuginfod_off) == 0)

Since it's an enum, you can do "debuginfod_enable == debuginfod_off" (oh
I now see Lancelot mentioned it as well).

> +    return false;
> +
> +  if (strcmp (debuginfod_enable, debuginfod_ask) == 0)
> +    {
> +      int resp = nquery (_("\nThis GDB supports auto-downloading debuginfo " \
> +			   "from the following URLs:\n%s\nEnable debuginfod? "),
> +			 debuginfod_urls.c_str ());
> +      if (!resp)
> +	{
> +	  printf_filtered (_("Debuginfod has been disabled.\nTo make this " \
> +			     "setting permanent, add \'set debuginfod off\' " \
> +			     "to .gdbinit.\n"));
> +	  debuginfod_enable = debuginfod_off;
> +	  return false;
> +	}
> +
> +      printf_filtered (_("Debuginfod has been enabled.\nTo make this " \
> +			 "setting permanent, add \'set debuginfod on\' " \
> +			 "to .gdbinit.\n"));
> +      debuginfod_enable = debuginfod_on;
> +    }
> +
> +  return true;
> +}

The behavior above seems good to me.

Simon