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

[Aaron Merey <amerey@redhat.com>]

Hi Lancelot,

On Thu, Oct 21, 2021 at 6:02 PM Lancelot SIX <lsix@lancelotsix.com> wrote:
> This is a minor styling point, but I think nullptr is preferred over NULL
> in new code.
> [...]
> 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).

Fixed.  I also replaced strerror with safe_strerror as Keith recommended.

On Thu, Oct 21, 2021 at 6:26 PM Lancelot SIX <lsix@lancelotsix.com> wrote:
> > Just a question for others: Is this the normal style used in GDB? E.g., the 
> > python command is always available:
> > (gdb) python print(1)
> > Python scripting is not supported in this copy of GDB.
> >
> > Like command/command-line options, I'd like us to be vigilant of consistency.
> > [Maybe that's just my personal thing, though.]
> >
> > It's just a question -- I am not asking for any changes.
>
> From what I can tell, the usage is usually to have the commands always
> present, but act as a noop (while maybe still printing a warning to the 
> user) when the feature is not built into GDB.  I think some commands
> would also force the value to become off if they detect the support is
> missing (see gdb_internal_backtrace_set_cmd for an example).
>
> One argument for this is that it allows the commands to be placed in
> scripts (such as .gdbinit for example) and not cause an error when
> executing script if the command is missing.  This would result in
> subsequent commands in the script to be ignored which could be
> unfortunate.  If the command is turned into a noop it is easier to have
> just one portable gdbinit file shared across deployment.
>
> That being said, this is just my personal view on the subject, I’ll let 
> maintainers tell if there is a guideline to follow.

I think you and Keith are right so I've added a separate set of set/show
functions to be used when GDB isn't built with debuginfod. The revised
patch is below. Thanks for the comments.

Aaron

From b68c40dbb6d9018e872e3d5db366261a39ae3422 Mon Sep 17 00:00:00 2001
From: Aaron Merey <amerey@redhat.com>
Date: Mon, 18 Oct 2021 18:30:30 -0400
Subject: [PATCH] gdb: add set/show commands for managing debuginfod

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.

If GDB is not built with debuginfod then these commands will display

  Support for debuginfod is not compiled into GDB
---
 gdb/debuginfod-support.c                      | 218 +++++++++++++++++-
 .../gdb.debuginfod/fetch_src_and_symbols.exp  |  25 +-
 2 files changed, 232 insertions(+), 11 deletions(-)

diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index 2d626e335a0..e0556bd7cea 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -18,10 +18,26 @@
 
 #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"
+#include "cli/cli-cmds.h"
+#include "cli/cli-style.h"
+
+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,
+  nullptr,
+};
+
+static const char *debuginfod_enable = debuginfod_ask;
+static std::string debuginfod_urls;
+static bool debuginfod_debug = true;
 
 #ifndef HAVE_LIBDEBUGINFOD
 scoped_fd
@@ -41,6 +57,53 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
 {
   return scoped_fd (-ENOSYS);
 }
+
+/* Stub set/show functions for debuginfod commands.  */
+
+static void
+set_debuginfod_command (const char *args, int from_tty,
+			struct cmd_list_element *c)
+{
+  error (_("Support for debuginfod is not compiled into GDB"));
+  debuginfod_enable = debuginfod_off;
+}
+
+static void
+show_debuginfod_command (struct ui_file *file, int from_tty,
+			 struct cmd_list_element *cmd, const char *value)
+{
+  error (_("Support for debuginfod is not compiled into GDB"));
+}
+
+static void
+set_debuginfod_urls_command (const char *args, int from_tty,
+			     struct cmd_list_element *c)
+{
+  error (_("Support for debuginfod is not compiled into GDB"));
+  debuginfod_urls.clear ();
+}
+
+static void
+show_debuginfod_urls_command (struct ui_file *file, int from_tty,
+			      struct cmd_list_element *cmd, const char *value)
+{
+  error (_("Support for debuginfod is not compiled into GDB"));
+}
+
+static void
+set_debuginfod_debug_command (const char *args, int from_tty,
+			      struct cmd_list_element *c)
+{
+  error (_("Support for debuginfod is not compiled into GDB"));
+  debuginfod_debug = false;
+}
+
+static void
+show_debuginfod_debug_command (struct ui_file *file, int from_tty,
+			       struct cmd_list_element *cmd, const char *value)
+{
+  error (_("Support for debuginfod is not compiled into GDB"));
+}
 #else
 #include <elfutils/debuginfod.h>
 
@@ -68,6 +131,66 @@ struct debuginfod_client_deleter
 using debuginfod_client_up
   = std::unique_ptr<debuginfod_client, debuginfod_client_deleter>;
 
+/* No-op setter used for compatibility when gdb is built without debuginfod.  */
+
+static void
+set_debuginfod_command (const char *args, int from_tty,
+			struct cmd_list_element *c)
+{
+  return;
+}
+
+/* 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"), safe_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);
+}
+
+/* No-op setter used for compatibility when gdb is built without debuginfod.  */
+
+static void
+set_debuginfod_debug_command (const char *args, int from_tty,
+			      struct cmd_list_element *c)
+{
+  return;
+}
+
+/* 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 +243,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 ()
+       || debuginfod_enable == debuginfod_off)
+    return false;
+
+  if (debuginfod_enable == debuginfod_ask)
+    {
+      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;
+}
+
 /* See debuginfod-support.h  */
 
 scoped_fd
@@ -128,8 +284,7 @@ debuginfod_source_query (const unsigned char *build_id,
 			 const char *srcpath,
 			 gdb::unique_xmalloc_ptr<char> *destname)
 {
-  const char *urls_env_var = getenv (DEBUGINFOD_URLS_ENV_VAR);
-  if (urls_env_var == NULL || urls_env_var[0] == '\0')
+  if (!debuginfod_enabled ())
     return scoped_fd (-ENOSYS);
 
   debuginfod_client *c = get_debuginfod_client ();
@@ -147,8 +302,7 @@ debuginfod_source_query (const unsigned char *build_id,
 					nullptr));
   debuginfod_set_user_data (c, nullptr);
 
-  /* TODO: Add 'set debug debuginfod' command to control when error messages are shown.  */
-  if (fd.get () < 0 && fd.get () != -ENOENT)
+  if (debuginfod_debug && fd.get () < 0 && fd.get () != -ENOENT)
     printf_filtered (_("Download failed: %s.  Continuing without source file %ps.\n"),
 		     safe_strerror (-fd.get ()),
 		     styled_string (file_name_style.style (),  srcpath));
@@ -167,8 +321,7 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
 			    const char *filename,
 			    gdb::unique_xmalloc_ptr<char> *destname)
 {
-  const char *urls_env_var = getenv (DEBUGINFOD_URLS_ENV_VAR);
-  if (urls_env_var == NULL || urls_env_var[0] == '\0')
+  if (!debuginfod_enabled ())
     return scoped_fd (-ENOSYS);
 
   debuginfod_client *c = get_debuginfod_client ();
@@ -184,7 +337,7 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
 					   &dname));
   debuginfod_set_user_data (c, nullptr);
 
-  if (fd.get () < 0 && fd.get () != -ENOENT)
+  if (debuginfod_debug && fd.get () < 0 && fd.get () != -ENOENT)
     printf_filtered (_("Download failed: %s.  Continuing without debug info for %ps.\n"),
 		     safe_strerror (-fd.get ()),
 		     styled_string (file_name_style.style (),  filename));
@@ -195,3 +348,52 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
   return fd;
 }
 #endif
+
+/* Register debuginfod commands.  */
+
+void _initialize_debuginfod ();
+void
+_initialize_debuginfod ()
+{
+#ifdef HAVE_LIBDEBUGINFOD
+  char *urls = getenv (DEBUGINFOD_URLS_ENV_VAR);
+
+  if (urls != nullptr)
+    debuginfod_urls = urls;
+#endif
+
+  add_setshow_enum_cmd ("debuginfod", class_run, debuginfod_cmd_names,
+			&debuginfod_enable, _("\
+Set whether debuginfod is enabled."), _("\
+Show whether debuginfod is enabled."), _("\
+Automatically query debuginfod servers for missing debuginfo, executables \
+or source files. This command accepts the following arguments:\n\
+  on  - debuginfod will be enabled\n\
+  off - debuginfod will be disabled\n\
+  ask - an interactive prompt will ask whether to enable debuginfod\n\
+For interactive sessions, \"ask\" is the default. For non-interactive \
+sessions, \"off\" is the default."),
+			set_debuginfod_command,
+		        show_debuginfod_command, &setlist, &showlist);
+
+  add_setshow_string_noescape_cmd ("debuginfod-urls", class_run,
+				   &debuginfod_urls, _("\
+Set the list of debuginfod server URLs."), _("\
+Show the list of debuginfod server URLs."), _("\
+Manage the space-separated list of debuginfod server URLs that GDB will query \
+when missing debuginfo, executables or source files.\nThe default value is \
+copied from $DEBUGINFOD_URLS."),
+				   set_debuginfod_urls_command,
+				   show_debuginfod_urls_command,
+				   &setlist, &showlist);
+
+  add_setshow_boolean_cmd ("debuginfod", class_maintenance,
+			   &debuginfod_debug, _("\
+Set debuginfod debugging."), _("\
+Show debuginfod debugging."), _("\
+When set, display debugging output for debuginfod. \
+Displayed by default"),
+			   set_debuginfod_debug_command,
+			   show_debuginfod_debug_command,
+			   &setdebuglist, &showdebuglist);
+}
diff --git a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
index 93490fce41e..cbf4cce1b00 100644
--- a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
+++ b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
@@ -217,7 +217,8 @@ proc local_url { } {
     setenv DEBUGINFOD_URLS http://127.0.0.1:$port
 
     # gdb should now find the symbol and source files
-    clean_restart $binfile
+    clean_restart
+    gdb_test "file $binfile" "" "file [file tail $binfile]" "Enable debuginfod?.*" "y"
     gdb_test_no_output "set substitute-path $outputdir /dev/null" \
 	"set substitute-path"
     gdb_test "br main" "Breakpoint 1 at.*file.*"
@@ -226,8 +227,26 @@ proc local_url { } {
     # gdb should now find the debugaltlink file
     clean_restart
     gdb_test "file ${binfile}_alt.o" \
-	".*Reading symbols from ${binfile}_alt.o\.\.\.*" \
-	"file [file tail ${binfile}_alt.o]"
+	".*Downloading.*separate debug info.*" \
+	"file [file tail ${binfile}_alt.o]" \
+	".*Enable debuginfod?.*" "y"
+
+    # Configure debuginfod with commands
+    unsetenv DEBUGINFOD_URLS
+    clean_restart
+    gdb_test "file $binfile" ".*No debugging symbols.*" \
+	"file [file tail $binfile] cmd"
+    gdb_test_no_output "set debuginfod off"
+    gdb_test_no_output "set debuginfod-urls http://127.0.0.1:$port"
+
+    # gdb shouldn't find the debuginfo since debuginfod has been disabled
+    gdb_test "file $binfile" ".*No debugging symbols.*" \
+	"file [file tail $binfile] cmd off"
+
+    # Enable debuginfod and fetch the debuginfo
+    gdb_test_no_output "set debuginfod on"
+    gdb_test "file $binfile" ".*Reading symbols from.*debuginfo.*" \
+	"file [file tail $binfile] cmd on"
 }
 
 set envlist \
-- 
2.31.1