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

[Lancelot SIX <lsix@lancelotsix.com>]

Hi,

I have some comments I have added in the patch.  One minor minor style
comment and a comment on usage of var_enum.  Hopefully it makes sense
and is useful.

On Mon, Oct 18, 2021 at 07:01:32PM -0400, 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.
> 
> 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.
> ---
>  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,

This is a minor styling point, but I think nullptr is preferred over NULL
in new code.

> +};
> +
> +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);
> +}
> +
> +/* 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);
> +}
> +
> +/* 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
> +   whether to enable debuginfod.  */
> +
> +static bool
> +debuginfod_enabled ()
> +{
> +   if (debuginfod_urls.empty ()
> +       || strcmp (debuginfod_enable, debuginfod_off) == 0)

Given how var_enum works, the only valid values debuginfod_enable can
have are those listed in debuginfod_cmd_names.  Therefore I do not think
it is necessary to compare the content of the strings, comparing
debuginfod_enable to debuginfod_off (the values of the pointers) should
be enough.  I just had a quick look and it seems that this is how other
places use var_enums (at least, this is how it is used in gdb/infrun.c).

> +    return false;
> +
> +  if (strcmp (debuginfod_enable, debuginfod_ask) == 0)

Same remark here, probably just use 'if (debuginfod_enable == debuginfod_ask)'.

Best,
Lancelot.