[PATCH 0/2 v3] gdb: Add debuginfod first-use notification

[PATCH 0/2 v3] gdb: Add debuginfod first-use notification

[Aaron Merey]

Hi,

Patch 1/2 implements an interactive debuginfod first-use notice
that was previously discussed[1].

Patch 2/2 is based on a patch that Eli reviewed[2] a while ago.
I held off on merging it since it referenced debuginfod core file
support for gdb that is still forthcoming.  Expanded debuginfod
documentation is needed now so I've revised the old patch with
information regarding the new commands added in patch [1/2].
Mentions of debuginfo core file support have been removed for now.

Thanks,
Aaron

[1] https://sourceware.org/pipermail/gdb-patches/2021-September/182288.html
[2] https://sourceware.org/pipermail/gdb-patches/2021-May/178571.html

Aaron Merey (2):
  gdb: add set/show commands for managing debuginfod
  gdb.texinfo: Expand documentation for debuginfod

 gdb/debuginfod-support.c                      | 151 +++++++++++++++++-
 gdb/doc/gdb.texinfo                           |  94 ++++++++++-
 .../gdb.debuginfod/fetch_src_and_symbols.exp  |  25 ++-
 3 files changed, 252 insertions(+), 18 deletions(-)

-- 
2.31.1

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

[Aaron Merey]

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,
+};
+
+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)
+    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;
+}
+
 /* See debuginfod-support.h  */
 
 scoped_fd
@@ -128,8 +219,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 +237,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 +256,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 +272,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 +283,50 @@ 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;
+
+  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."),
+			NULL, 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"),
+			   NULL, show_debuginfod_debug_command,
+			   &setdebuglist, &showdebuglist);
+#endif
+}
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

[PATCH 2/2] gdb.texinfo: Expand documentation for debuginfod

[Aaron Merey]

Add section describing GDB's usage of debuginfod and new
debuginfod commands.

Refer to this new section in the description of the '--with-debuginfod'
configure option.

Mention debuginfod in the 'Separate Debug Files' section.
---
 gdb/doc/gdb.texinfo | 94 +++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 87 insertions(+), 7 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 631a7c03b31..90c73a6a2b6 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -184,6 +184,7 @@ software in general.  We will miss him.
                                  the operating system
 * Trace File Format::		GDB trace file format
 * Index Section Format::        .gdb_index section format
+* Debuginfod::                  Download debugging resources with @code{debuginfod}
 * Man Pages::			Manual pages
 * Copying::			GNU General Public License says
                                 how you can copy and share GDB
@@ -21398,7 +21399,9 @@ For the ``build ID'' method, @value{GDBN} looks in the
 a file named @file{@var{nn}/@var{nnnnnnnn}.debug}, where @var{nn} are the
 first 2 hex characters of the build ID bit string, and @var{nnnnnnnn}
 are the rest of the bit string.  (Real build ID strings are 32 or more
-hex characters, not 10.)
+hex characters, not 10.)  @value{GDBN} can automatically query
+@code{debuginfod} servers using build IDs in order to download separate debug
+files that cannot be found locally.  For more information see @ref{Debuginfod}.
 @end itemize
 
 So, for example, suppose you ask @value{GDBN} to debug
@@ -21419,6 +21422,10 @@ debug information files, in the indicated order:
 @file{/usr/lib/debug/usr/bin/ls.debug}.
 @end itemize
 
+If the debug file still has not been found and @code{debuginfod}
+(@pxref{Debuginfod}) is enabled, @value{GDBN} will attempt to download the
+file from @code{debuginfod} servers.
+
 @anchor{debug-file-directory}
 Global debugging info directories default to what is set by @value{GDBN}
 configure option @option{--with-separate-debug-dir}.  During @value{GDBN} run
@@ -38750,12 +38757,12 @@ Use the curses library instead of the termcap library, for text-mode
 terminal operations.
 
 @item --with-debuginfod
-Build @value{GDBN} with libdebuginfod, the debuginfod client library.
-Used to automatically fetch source files and separate debug files from
-debuginfod servers using the associated executable's build ID. Enabled
-by default if libdebuginfod is installed and found at configure time.
-debuginfod is packaged with elfutils, starting with version 0.178. You
-can get the latest version from `https://sourceware.org/elfutils/'.
+Build @value{GDBN} with @file{libdebuginfod}, the @code{debuginfod} client
+library.  Used to automatically fetch ELF, DWARF and source files from
+@code{debuginfod} servers using build IDs associated with any missing
+files.  Enabled by default if @file{libdebuginfod} is installed and found
+at configure time.  For more information regarding @code{debuginfod} see
+@ref{Debuginfod}.
 
 @item --with-libunwind-ia64
 Use the libunwind library for unwinding function call stack on ia64
@@ -47069,6 +47076,79 @@ switch (die->tag)
   @}
 @end smallexample
 
+@node Debuginfod
+@appendix Download debugging resources with Debuginfod
+@cindex debuginfod
+
+@code{debuginfod} is an HTTP server for distributing ELF, DWARF and source code.
+
+With the @code{debuginfod} client library, @file{libdebuginfod}, @value{GDBN}
+can query servers using the build IDs of missing resources in order to download
+them on demand.
+
+@value{GDBN} is able to automatically download missing debug info and source
+files using @code{debuginfod}.
+
+For instructions on building @value{GDBN} with @file{libdebuginfod},
+@xref{Configure Options,,--with-debuginfod}.  @code{debuginfod} is packaged
+with @code{elfutils}, starting with version 0.178.  See
+@uref{https://sourceware.org/elfutils/Debuginfod.html} for more information
+regarding @code{debuginfod}.
+
+@menu
+* Debuginfod Settings::		Configuring debuginfod with @value{GDBN}
+@end menu
+
+@node Debuginfod Settings
+@section Debuginfod Settings
+
+@value{GDBN} provides the following commands for configuring @code{debuginfod}.
+
+@table @code
+@kindex set debuginfod
+@anchor{set debuginfod}
+@item set debuginfod
+@itemx set debuginfod on
+@cindex enable debuginfod
+@value{GDBN} will attempt to query @code{debuginfod} servers when missing debug
+info or source files.
+
+@item set debuginfod off
+@value{GDBN} will not attempt to query @code{debuginfod} servers when missing
+debug info or source files.
+
+@item set debuginfod ask
+@value{GDBN} will prompt the user to enable or disable @code{debuginfod} before
+attempting to perform the next query.
+
+@kindex show debuginfod
+@item show debuginfod
+Show whether @code{debuginfod} is set to @code{on}, @code{off} or @code{ask}.
+
+@kindex set debuginfod-urls
+@cindex configure debuginfod URLs
+@item set debuginfod-urls
+@itemx set debuginfod-urls @var{urls}
+Set the space-separated list of URLs that @code{debuginfod} will attempt to
+query.
+
+@item show debuginfod-urls
+Display the list of URLs that @code{debuginfod} will attempt to query.
+
+@kindex set debug debuginfod
+@kindex show debug debuginfod
+@cindex debugging debuginfod
+@item set debug debuginfod
+@itemx show debug debuginfod
+Enable or disable @code{debuginfod}-related debugging output.
+Use @code{1} to enable and @code{0} to disable.
+@end table
+
+By default, @code{debuginfod} is set to @code{ask} for interactive sessions and
+@code{off} for non-interactive sessions. The default value of
+@code{debuginfod-urls} is copied from @var{$DEBUGINFOD_URLS}. Debuginfod
+debugging output is shown by default.
+
 @node Man Pages
 @appendix Manual pages
 @cindex Man pages
-- 
2.31.1

Re: [PATCH 2/2] gdb.texinfo: Expand documentation for debuginfod

[Eli Zaretskii]

> Date: Mon, 18 Oct 2021 19:01:33 -0400
> From: Aaron Merey via Gdb-patches <gdb-patches@sourceware.org>
> Cc: simark@simark.ca
> 
> Add section describing GDB's usage of debuginfod and new
> debuginfod commands.

Thanks.

> +With the @code{debuginfod} client library, @file{libdebuginfod}, @value{GDBN}
> +can query servers using the build IDs of missing resources in order to download
> +them on demand.
> +
> +@value{GDBN} is able to automatically download missing debug info and source
> +files using @code{debuginfod}.

It sounds like these two paragraphs say the same, so only one of them
is needed.

> +For instructions on building @value{GDBN} with @file{libdebuginfod},
> +@xref{Configure Options,,--with-debuginfod}.  @code{debuginfod} is packaged

This should be @pxref.  @xref produces a capitalized "See", so it is
only appropriate at the beginning of a sentence.

> +@item set debuginfod
> +@itemx set debuginfod on
> +@cindex enable debuginfod
> +@value{GDBN} will attempt to query @code{debuginfod} servers when missing debug
> +info or source files.
> +
> +@item set debuginfod off
> +@value{GDBN} will not attempt to query @code{debuginfod} servers when missing
> +debug info or source files.
> +
> +@item set debuginfod ask
> +@value{GDBN} will prompt the user to enable or disable @code{debuginfod} before
> +attempting to perform the next query.

Please state which one of these is the default.  This text:

> +By default, @code{debuginfod} is set to @code{ask} for interactive sessions and
> +@code{off} for non-interactive sessions.

should be here, not further down.

> +@kindex set debuginfod-urls
> +@cindex configure debuginfod URLs
> +@item set debuginfod-urls
> +@itemx set debuginfod-urls @var{urls}
> +Set the space-separated list of URLs that @code{debuginfod} will attempt to
> +query.

Should we tell what protocol(s) are supported for these URLs?

Also, please describe the default here, not further down.

> +@code{off} for non-interactive sessions. The default value of
> +@code{debuginfod-urls} is copied from @var{$DEBUGINFOD_URLS}. Debuginfod
> +debugging output is shown by default.

Don't forget to leave 2 spaces between sentences.

Re: [PATCH 2/2] gdb.texinfo: Expand documentation for debuginfod

[Aaron Merey]

Thanks Eli. The updated patch is below.

Aaron

From 94df86b8775e0500a6ce6bbb8a65f90a8ec838da Mon Sep 17 00:00:00 2001
From: Aaron Merey <amerey@redhat.com>
Date: Tue, 19 Oct 2021 18:20:40 -0400
Subject: [PATCH] gdb.texinfo: Expand documentation for debuginfod

Add section describing GDB's usage of debuginfod and new
debuginfod commands.

Refer to this new section in the description of the '--with-debuginfod'
configure option.

Mention debuginfod in the 'Separate Debug Files' section.
---
 gdb/doc/gdb.texinfo | 91 +++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 84 insertions(+), 7 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 631a7c03b31..a7400c5bdf7 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -184,6 +184,7 @@ software in general.  We will miss him.
                                  the operating system
 * Trace File Format::		GDB trace file format
 * Index Section Format::        .gdb_index section format
+* Debuginfod::                  Download debugging resources with @code{debuginfod}
 * Man Pages::			Manual pages
 * Copying::			GNU General Public License says
                                 how you can copy and share GDB
@@ -21398,7 +21399,9 @@ For the ``build ID'' method, @value{GDBN} looks in the
 a file named @file{@var{nn}/@var{nnnnnnnn}.debug}, where @var{nn} are the
 first 2 hex characters of the build ID bit string, and @var{nnnnnnnn}
 are the rest of the bit string.  (Real build ID strings are 32 or more
-hex characters, not 10.)
+hex characters, not 10.)  @value{GDBN} can automatically query
+@code{debuginfod} servers using build IDs in order to download separate debug
+files that cannot be found locally.  For more information see @ref{Debuginfod}.
 @end itemize
 
 So, for example, suppose you ask @value{GDBN} to debug
@@ -21419,6 +21422,10 @@ debug information files, in the indicated order:
 @file{/usr/lib/debug/usr/bin/ls.debug}.
 @end itemize
 
+If the debug file still has not been found and @code{debuginfod}
+(@pxref{Debuginfod}) is enabled, @value{GDBN} will attempt to download the
+file from @code{debuginfod} servers.
+
 @anchor{debug-file-directory}
 Global debugging info directories default to what is set by @value{GDBN}
 configure option @option{--with-separate-debug-dir}.  During @value{GDBN} run
@@ -38750,12 +38757,12 @@ Use the curses library instead of the termcap library, for text-mode
 terminal operations.
 
 @item --with-debuginfod
-Build @value{GDBN} with libdebuginfod, the debuginfod client library.
-Used to automatically fetch source files and separate debug files from
-debuginfod servers using the associated executable's build ID. Enabled
-by default if libdebuginfod is installed and found at configure time.
-debuginfod is packaged with elfutils, starting with version 0.178. You
-can get the latest version from `https://sourceware.org/elfutils/'.
+Build @value{GDBN} with @file{libdebuginfod}, the @code{debuginfod} client
+library.  Used to automatically fetch ELF, DWARF and source files from
+@code{debuginfod} servers using build IDs associated with any missing
+files.  Enabled by default if @file{libdebuginfod} is installed and found
+at configure time.  For more information regarding @code{debuginfod} see
+@ref{Debuginfod}.
 
 @item --with-libunwind-ia64
 Use the libunwind library for unwinding function call stack on ia64
@@ -47069,6 +47076,76 @@ switch (die->tag)
   @}
 @end smallexample
 
+@node Debuginfod
+@appendix Download debugging resources with Debuginfod
+@cindex debuginfod
+
+@code{debuginfod} is an HTTP server for distributing ELF, DWARF and source code.
+
+With the @code{debuginfod} client library, @file{libdebuginfod}, @value{GDBN}
+can query servers using the build IDs associated with missing debug info,
+executables and source files in order to download them on demand.
+
+For instructions on building @value{GDBN} with @file{libdebuginfod},
+@pxref{Configure Options,,--with-debuginfod}.  @code{debuginfod} is packaged
+with @code{elfutils}, starting with version 0.178.  See
+@uref{https://sourceware.org/elfutils/Debuginfod.html} for more information
+regarding @code{debuginfod}.
+
+@menu
+* Debuginfod Settings::		Configuring debuginfod with @value{GDBN}
+@end menu
+
+@node Debuginfod Settings
+@section Debuginfod Settings
+
+@value{GDBN} provides the following commands for configuring @code{debuginfod}.
+
+@table @code
+@kindex set debuginfod
+@anchor{set debuginfod}
+@item set debuginfod
+@itemx set debuginfod on
+@cindex enable debuginfod
+@value{GDBN} will attempt to query @code{debuginfod} servers when missing debug
+info or source files.
+
+@item set debuginfod off
+@value{GDBN} will not attempt to query @code{debuginfod} servers when missing
+debug info or source files.  By default, @code{debuginfod} is set to @code{off}
+for non-interactive sessions.
+
+@item set debuginfod ask
+@value{GDBN} will prompt the user to enable or disable @code{debuginfod} before
+attempting to perform the next query.  By default, @code{debuginfod} is set to
+@code{ask} for interactive sessions.
+
+@kindex show debuginfod
+@item show debuginfod
+Show whether @code{debuginfod} is set to @code{on}, @code{off} or @code{ask}.
+
+@kindex set debuginfod-urls
+@cindex configure debuginfod URLs
+@item set debuginfod-urls
+@itemx set debuginfod-urls @var{urls}
+Set the space-separated list of URLs that @code{debuginfod} will attempt to
+query.  Only @code{http://}, @code{https://} and @code{file://} protocols
+should be used.  The default value of @code{debuginfod-urls} is copied from
+@var{$DEBUGINFOD_URLS}.
+
+@item show debuginfod-urls
+Display the list of URLs that @code{debuginfod} will attempt to query.
+
+@kindex set debug debuginfod
+@kindex show debug debuginfod
+@cindex debugging debuginfod
+@item set debug debuginfod
+@itemx show debug debuginfod
+Enable or disable @code{debuginfod}-related debugging output.  Use @code{1}
+to enable and @code{0} to disable.  Debuginfod debugging output is shown
+by default.
+@end table
+
 @node Man Pages
 @appendix Manual pages
 @cindex Man pages
-- 
2.31.1

Re: [PATCH 2/2] gdb.texinfo: Expand documentation for debuginfod

[Eli Zaretskii]

> From: Aaron Merey <amerey@redhat.com>
> Cc: gdb-patches@sourceware.org,
> 	simark@simark.ca
> Date: Tue, 19 Oct 2021 18:35:09 -0400
> 
> Thanks Eli. The updated patch is below.

Thanks, this is OK.

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

[Keith Seitz]

Hi,

Thank you for your perseverance with this patch. This is really
shaping up as a nice, user-friendly addition. It's also very
"gdb-ish."

I have just a few things. Nothing major.

On 10/18/21 16:01, Aaron Merey wrote:

>    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.

Very nice!

> 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.

At first I thought this space-separated list as a bit odd, but I see that
this is exactly the same format as the environment variable DEBUGINFOD_URLS.
I certainly appreciate the consistency.

> 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.

Here I have a question. From the patch, it appears that the debugging output
consists entirely of error messages? Am I alone thinking we should always print these
for the user?

> 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
> @@ -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,
> +};
> +
> +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));
>
I believe we prefer to use safe_strerror (as used elsewhere in this file).

> +
> +/* 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)
>   {
> @@ -195,3 +283,50 @@ 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;
> +
> +  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."),
> +			NULL, 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"),
> +			   NULL, show_debuginfod_debug_command,
> +			   &setdebuglist, &showdebuglist);
> +#endif
> +}

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.

With the safe_strerror change, though, I recommend a maintainer review
this patch for final approval(s).

Keith

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

[Lancelot SIX]

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.

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

[Lancelot SIX]

> 
> 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.

Hi,

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.

Best,
Lancelot.
> 
> With the safe_strerror change, though, I recommend a maintainer review
> this patch for final approval(s).
> 
> Keith
> 

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

[Aaron Merey]

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

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

[Simon Marchi]

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

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

[Aaron Merey]

Hi Simon,

Thanks for the comments. The revised patch is below. It includes
revisions based on Lancelot's feedback too.

Aaron

From da52be8a278973bd401a6d4be7fda3ac25568a01 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' commands.  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' commands. Accepts a string of
space-separated debuginfod server URLs to be queried.  The default
value is copied from $DEBUGINFOD_URLS.

Finally add 'set/show debuginfod verbose" commands to control whether
debuginfod-related output is displayed.  Verbose output is enabled
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 just display

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

diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index 2d626e335a0..38b527f7827 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -18,10 +18,22 @@
 
 #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"
+
+/* Set/show debuginfod commands.  */
+static cmd_list_element *set_debuginfod_prefix_list;
+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_enable = debuginfod_ask;
+static unsigned debuginfod_verbose = 1;
 
 #ifndef HAVE_LIBDEBUGINFOD
 scoped_fd
@@ -41,6 +53,73 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
 {
   return scoped_fd (-ENOSYS);
 }
+
+#define NO_IMPL _("Support for debuginfod is not compiled into GDB.")
+
+/* Stub set/show commands that indicate debuginfod is not supported.  */
+
+static void
+set_debuginfod_on_command (const char *args, int from_tty)
+{
+  error (NO_IMPL);
+  debuginfod_enable = debuginfod_off;
+}
+
+static void
+set_debuginfod_off_command (const char *args, int from_tty)
+{
+  error (NO_IMPL);
+  debuginfod_enable = debuginfod_off;
+}
+
+static void
+set_debuginfod_ask_command (const char *args, int from_tty)
+{
+  error (NO_IMPL);
+  debuginfod_enable = debuginfod_off;
+}
+
+static void
+show_debuginfod_command (const char *args, int from_tty)
+{
+  error (NO_IMPL);
+}
+
+static void
+set_debuginfod_urls_command (const std::string& urls)
+{
+  error (NO_IMPL);
+}
+
+static const std::string&
+get_debuginfod_urls_command ()
+{
+  static std::string empty;
+  return empty;
+}
+
+static void
+show_debuginfod_urls_command (struct ui_file *file, int from_tty,
+			      struct cmd_list_element *cmd, const char *value)
+{
+  error (NO_IMPL);
+}
+
+static void
+set_debuginfod_verbose_command (const char *args, int from_tty,
+				struct cmd_list_element *c)
+{
+  error (NO_IMPL);
+  debuginfod_verbose = 0;
+}
+
+static void
+show_debuginfod_verbose_command (struct ui_file *file, int from_tty,
+				 struct cmd_list_element *cmd,
+				 const char *value)
+{
+  error (NO_IMPL);
+}
 #else
 #include <elfutils/debuginfod.h>
 
@@ -68,6 +147,96 @@ struct debuginfod_client_deleter
 using debuginfod_client_up
   = std::unique_ptr<debuginfod_client, debuginfod_client_deleter>;
 
+/* Enable debuginfod.  */
+
+static void
+set_debuginfod_on_command (const char *args, int from_tty)
+{
+  debuginfod_enable = debuginfod_on;
+}
+
+/* Disable debuginfod.  */
+
+static void
+set_debuginfod_off_command (const char *args, int from_tty)
+{
+  debuginfod_enable = debuginfod_off;
+}
+
+/* Before next query, ask user whether to enable debuginfod.  */
+
+static void
+set_debuginfod_ask_command (const char *args, int from_tty)
+{
+  debuginfod_enable = debuginfod_ask;
+}
+
+/* Show whether debuginfod is enabled.  */
+
+static void
+show_debuginfod_command (const char *args, int from_tty)
+{
+  printf_unfiltered (_("Debuginfod functionality is currently set to " \
+		     "\"%s\".\n"), debuginfod_enable);
+}
+
+/* Set the URLs that debuginfod will query.  */
+
+static void
+set_debuginfod_urls_command (const std::string& urls)
+{
+  if (setenv ("DEBUGINFOD_URLS", urls.c_str (), 1) != 0)
+    warning (_("Unable to set debuginfod URLs: %s"), safe_strerror (errno));
+}
+
+/* Get current debuginfod URLs.  */
+
+static const std::string&
+get_debuginfod_urls_command ()
+{
+  static std::string urls;
+  const char *envvar = getenv (DEBUGINFOD_URLS_ENV_VAR);
+
+  if (envvar != nullptr)
+    urls = envvar;
+  else
+    urls.clear ();
+
+  return urls;
+}
+
+/* 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 (value == nullptr || value[0] == '\0')
+    fprintf_unfiltered (file, _("Debuginfod URLs have not been set.\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_verbose_command (const char *args, int from_tty,
+			      struct cmd_list_element *c)
+{
+  return;
+}
+
+/* Show verbosity.  */
+
+static void
+show_debuginfod_verbose_command (struct ui_file *file, int from_tty,
+			       struct cmd_list_element *cmd, const char *value)
+{
+  fprintf_filtered (file, _("Debuginfod verbose output is set to %s.\n"),
+		    value);
+}
+
 static int
 progressfn (debuginfod_client *c, long cur, long total)
 {
@@ -120,6 +289,41 @@ 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 ()
+{
+  const char *urls = getenv (DEBUGINFOD_URLS_ENV_VAR);
+
+  if (urls == nullptr || urls[0] == '\0'
+      || 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? "),
+			 urls);
+      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 +332,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 +350,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_verbose > 0 && 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 +369,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 +385,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_verbose > 0 && 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 +396,61 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
   return fd;
 }
 #endif
+
+/* Register debuginfod commands.  */
+
+void _initialize_debuginfod ();
+void
+_initialize_debuginfod ()
+{
+  /* set debuginfod */
+  add_basic_prefix_cmd ("debuginfod", class_run,
+			_("Set debuginfod options"),
+			&set_debuginfod_prefix_list,
+			false, &setlist);
+
+  /* show debuginfod */
+  add_prefix_cmd ("debuginfod", class_run,
+		  show_debuginfod_command,
+		  _("Show debuginfod options"),
+		  &show_debuginfod_prefix_list,
+		  false, &showlist);
+
+  /* set debuginfod on */
+  add_cmd ("on", class_run, set_debuginfod_on_command,
+	   _("Enable debuginfod."), &set_debuginfod_prefix_list);
+
+  /* set debuginfod off */
+  add_cmd ("off", class_run, set_debuginfod_off_command,
+	   _("Disable debuginfod."), &set_debuginfod_prefix_list);
+
+  /* set debuginfod ask */
+  add_cmd ("ask", class_run, set_debuginfod_ask_command, _("\
+Ask the user whether to enable debuginfod before performing the next query."),
+	   &set_debuginfod_prefix_list);
+
+  /* set/show debuginfod urls */
+  add_setshow_string_noescape_cmd ("urls", class_run, _("\
+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,
+				   get_debuginfod_urls_command,
+				   show_debuginfod_urls_command,
+				   &set_debuginfod_prefix_list,
+				   &show_debuginfod_prefix_list);
+
+  /* set/show debuginfod verbose */
+  add_setshow_zuinteger_cmd ("verbose", class_support,
+			     &debuginfod_verbose, _("\
+Set verbosity of debuginfod output."), _("\
+Show debuginfod debugging."), _("\
+When set to a non-zero value, display verbose output for each debuginfod \
+query.\nTo disable, set to zero.  Verbose output is displayed by default."),
+			     set_debuginfod_verbose_command,
+			     show_debuginfod_verbose_command,
+			     &set_debuginfod_prefix_list,
+			     &show_debuginfod_prefix_list);
+}
diff --git a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
index 93490fce41e..eb12c008d2e 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

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

[Simon Marchi]

On 2021-10-28 18:18, Aaron Merey wrote:
> Hi Simon,
> 
> Thanks for the comments. The revised patch is below. It includes
> revisions based on Lancelot's feedback too.
> 
> Aaron

Thanks for the update.  I have a few minor comments, the patch is OK to
push with that fixed.

> 
> From da52be8a278973bd401a6d4be7fda3ac25568a01 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' commands.  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.

I just thought that it would be nice to say "Enable debuginfod for this
session?", just to make it clear that it's for the duration of the
session.

> +/* 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 (value == nullptr || value[0] == '\0')
> +    fprintf_unfiltered (file, _("Debuginfod URLs have not been set.\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_verbose_command (const char *args, int from_tty,
> +			      struct cmd_list_element *c)

The last line is missing two columns of indent, and the function below
too.

> +/* Register debuginfod commands.  */
> +
> +void _initialize_debuginfod ();
> +void
> +_initialize_debuginfod ()
> +{
> +  /* set debuginfod */
> +  add_basic_prefix_cmd ("debuginfod", class_run,
> +			_("Set debuginfod options"),
> +			&set_debuginfod_prefix_list,
> +			false, &setlist);
> +
> +  /* show debuginfod */
> +  add_prefix_cmd ("debuginfod", class_run,
> +		  show_debuginfod_command,
> +		  _("Show debuginfod options"),
> +		  &show_debuginfod_prefix_list,
> +		  false, &showlist);

I just merged a patch that adds the add_setshow_prefix_cmd function,
that does the equivalent of the two function calls above.  Could you
convert your code to use it?

> +
> +  /* set debuginfod on */
> +  add_cmd ("on", class_run, set_debuginfod_on_command,
> +	   _("Enable debuginfod."), &set_debuginfod_prefix_list);
> +
> +  /* set debuginfod off */
> +  add_cmd ("off", class_run, set_debuginfod_off_command,
> +	   _("Disable debuginfod."), &set_debuginfod_prefix_list);
> +
> +  /* set debuginfod ask */
> +  add_cmd ("ask", class_run, set_debuginfod_ask_command, _("\
> +Ask the user whether to enable debuginfod before performing the next query."),
> +	   &set_debuginfod_prefix_list);
> +
> +  /* set/show debuginfod urls */
> +  add_setshow_string_noescape_cmd ("urls", class_run, _("\
> +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."),

Is it clear to the user that $DEBUGINFOD_URLS refers to the environment
variable?  Perhaps say "from the DEBUGINFOD_URLS environment
variable".

> +				   set_debuginfod_urls_command,
> +				   get_debuginfod_urls_command,
> +				   show_debuginfod_urls_command,
> +				   &set_debuginfod_prefix_list,
> +				   &show_debuginfod_prefix_list);
> +
> +  /* set/show debuginfod verbose */
> +  add_setshow_zuinteger_cmd ("verbose", class_support,
> +			     &debuginfod_verbose, _("\
> +Set verbosity of debuginfod output."), _("\
> +Show debuginfod debugging."), _("\
> +When set to a non-zero value, display verbose output for each debuginfod \
> +query.\nTo disable, set to zero.  Verbose output is displayed by default."),
> +			     set_debuginfod_verbose_command,
> +			     show_debuginfod_verbose_command,
> +			     &set_debuginfod_prefix_list,
> +			     &show_debuginfod_prefix_list);

Just wondering, is it your intent to use an integer here, to allow for
possibly more than one level of verbosity?  If so, then it's ok.  If
not, then it would make more sense to use a bool.

> +}
> diff --git a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
> index 93490fce41e..eb12c008d2e 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

It is happening even before this patch, but for some reason on my
computer I get:


    $ make check TESTS="gdb.debuginfod/fetch_src_and_symbols.exp"
    make[1]: Entering directory '/home/simark/build/binutils-gdb/gdb/testsuite'
    make check-single
    make[2]: Entering directory '/home/simark/build/binutils-gdb/gdb/testsuite'
    rootme=`pwd`; export rootme; srcdir=/home/simark/src/binutils-gdb/gdb/testsuite ; export srcdir ; EXPECT=`if [ "${READ1}" != "" ] ; then echo ${rootme}/expect-read1; elif [ "${READMORE}" != "" ] ; then echo ${rootme}/expect-readmore; elif [ -f ${rootme}/../../expect/expect ] ; then echo ${rootme}/../../expect/expect ; else echo expect ; fi` ; export EXPECT ; EXEEXT= ; export EXEEXT ;    LD_LIBRARY_PATH=$rootme/../../expect:$rootme/../../libstdc++:$rootme/../../tk/unix:$rootme/../../tcl/unix:$rootme/../../bfd:$rootme/../../opcodes:$LD_LIBRARY_PATH; export LD_LIBRARY_PATH; if [ -f ${rootme}/../../expect/expect ] ; then TCL_LIBRARY=${srcdir}/../../tcl/library ; export TCL_LIBRARY ; fi ; runtest --status  gdb.debuginfod/fetch_src_and_symbols.exp 
    WARNING: Couldn't find the global config file.
    Using /home/simark/src/binutils-gdb/gdb/testsuite/lib/gdb.exp as tool init file.
    NOTE: Dejagnu's default_target_compile is missing support for Go, using local override
    NOTE: Dejagnu's default_target_compile is missing support for Rust, using local override
    Test run by simark on Thu Oct 28 21:43:37 2021
    Native configuration is x86_64-pc-linux-gnu
    
                    === gdb tests ===
    
    Schedule of variations:
        unix
    
    Running target unix
    Using /usr/share/dejagnu/baseboards/unix.exp as board description file for target.
    Using /usr/share/dejagnu/config/unix.exp as generic interface file for target.
    Using /home/simark/src/binutils-gdb/gdb/testsuite/config/unix.exp as tool-and-target-specific interface file.
    Running /home/simark/src/binutils-gdb/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp ...
    ERROR: tcl error sourcing /home/simark/src/binutils-gdb/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp.
    ERROR: This GDB was configured as follows:
       configure --host=x86_64-pc-linux-gnu --target=x86_64-pc-linux-gnu
                 --with-auto-load-dir=$debugdir:$datadir/auto-load
                 --with-auto-load-safe-path=$debugdir:$datadir/auto-load
                 --with-expat
                 --with-gdb-datadir=/usr/local/share/gdb (relocatable)
                 --with-jit-reader-dir=/usr/local/lib/gdb (relocatable)
                 --without-libunwind-ia64
                 --with-lzma
                 --with-babeltrace
                 --with-intel-pt
                 --with-mpfr
                 --with-xxhash
                 --with-python=/usr
                 --with-python-libdir=/tmp/foo
                 --with-debuginfod
                 --with-guile
                 --enable-source-highlight
                 --with-separate-debug-dir=/usr/local/lib/debug (relocatable)
    
    ("Relocatable" means the directory can be moved with the GDB installation
    tree, and GDB will still find it.)

I'll have to investigate that later.

Simon

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

[Aaron Merey]

Hi Simon,

On Thu, Oct 28, 2021 at 9:47 PM Simon Marchi <simon.marchi@polymtl.ca> wrote:
> Thanks for the update.  I have a few minor comments, the patch is OK to
> push with that fixed.

Pushed as commit 7811fa5995fc.

> >   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.
>
> I just thought that it would be nice to say "Enable debuginfod for this
> session?", just to make it clear that it's for the duration of the
> session.

Fixed.

> > +static void
> > +set_debuginfod_verbose_command (const char *args, int from_tty,
> > +                           struct cmd_list_element *c)
>
> The last line is missing two columns of indent, and the function below
> too.

Fixed.

> > +  /* set debuginfod */
> > +  add_basic_prefix_cmd ("debuginfod", class_run,
> > +                     _("Set debuginfod options"),
> > +                     &set_debuginfod_prefix_list,
> > +                     false, &setlist);
> > +
> > +  /* show debuginfod */
> > +  add_prefix_cmd ("debuginfod", class_run,
> > +               show_debuginfod_command,
> > +               _("Show debuginfod options"),
> > +               &show_debuginfod_prefix_list,
> > +               false, &showlist);
>
> I just merged a patch that adds the add_setshow_prefix_cmd function,
> that does the equivalent of the two function calls above.  Could you
> convert your code to use it?

Changed to use add_setshow_prefix_cmd. Because this function
doesn't have a parameter for a custom show function I added
a 'show debuginfod status' command so that the user can check
whether debuginfod is set to 'on', 'off' or 'ask'.

> Is it clear to the user that $DEBUGINFOD_URLS refers to the environment
> variable?  Perhaps say "from the DEBUGINFOD_URLS environment
> variable".

Fixed.

> > +  /* set/show debuginfod verbose */
> > +  add_setshow_zuinteger_cmd ("verbose", class_support,
> > +                          &debuginfod_verbose, _("\
> > +Set verbosity of debuginfod output."), _("\
> > +Show debuginfod debugging."), _("\
> > +When set to a non-zero value, display verbose output for each debuginfod \
> > +query.\nTo disable, set to zero.  Verbose output is displayed by default."),
> > +                          set_debuginfod_verbose_command,
> > +                          show_debuginfod_verbose_command,
> > +                          &set_debuginfod_prefix_list,
> > +                          &show_debuginfod_prefix_list);
>
> Just wondering, is it your intent to use an integer here, to allow for
> possibly more than one level of verbosity?  If so, then it's ok.  If
> not, then it would make more sense to use a bool.

I kept it as an integer so that if we decide to add additional levels of
verbosity in the future then we won't have to change the argument
from bool to int.

> It is happening even before this patch, but for some reason on my
> computer I get:
>
>
>     $ make check TESTS="gdb.debuginfod/fetch_src_and_symbols.exp"
>     make[1]: Entering directory '/home/simark/build/binutils-gdb/gdb/testsuite'
>     make check-single
>     make[2]: Entering directory '/home/simark/build/binutils-gdb/gdb/testsuite'
>     rootme=`pwd`; export rootme; srcdir=/home/simark/src/binutils-gdb/gdb/testsuite ; export srcdir ; EXPECT=`if [ "${READ1}" != "" ] ; then echo ${rootme}/expect-read1; elif [ "${READMORE}" != "" ] ; then echo ${rootme}/expect-readmore; elif [ -f ${rootme}/../../expect/expect ] ; then echo ${rootme}/../../expect/expect ; else echo expect ; fi` ; export EXPECT ; EXEEXT= ; export EXEEXT ;    LD_LIBRARY_PATH=$rootme/../../expect:$rootme/../../libstdc++:$rootme/../../tk/unix:$rootme/../../tcl/unix:$rootme/../../bfd:$rootme/../../opcodes:$LD_LIBRARY_PATH; export LD_LIBRARY_PATH; if [ -f ${rootme}/../../expect/expect ] ; then TCL_LIBRARY=${srcdir}/../../tcl/library ; export TCL_LIBRARY ; fi ; runtest --status  gdb.debuginfod/fetch_src_and_symbols.exp
>     WARNING: Couldn't find the global config file.
>     Using /home/simark/src/binutils-gdb/gdb/testsuite/lib/gdb.exp as tool init file.
>     NOTE: Dejagnu's default_target_compile is missing support for Go, using local override
>     NOTE: Dejagnu's default_target_compile is missing support for Rust, using local override
>     Test run by simark on Thu Oct 28 21:43:37 2021
>     Native configuration is x86_64-pc-linux-gnu
>
>                     === gdb tests ===
>
>     Schedule of variations:
>         unix
>
>     Running target unix
>     Using /usr/share/dejagnu/baseboards/unix.exp as board description file for target.
>     Using /usr/share/dejagnu/config/unix.exp as generic interface file for target.
>     Using /home/simark/src/binutils-gdb/gdb/testsuite/config/unix.exp as tool-and-target-specific interface file.
>     Running /home/simark/src/binutils-gdb/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp ...
>     ERROR: tcl error sourcing /home/simark/src/binutils-gdb/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp.
>     ERROR: This GDB was configured as follows:
>        configure --host=x86_64-pc-linux-gnu --target=x86_64-pc-linux-gnu
>                  --with-auto-load-dir=$debugdir:$datadir/auto-load
>                  --with-auto-load-safe-path=$debugdir:$datadir/auto-load
>                  --with-expat
>                  --with-gdb-datadir=/usr/local/share/gdb (relocatable)
>                  --with-jit-reader-dir=/usr/local/lib/gdb (relocatable)
>                  --without-libunwind-ia64
>                  --with-lzma
>                  --with-babeltrace
>                  --with-intel-pt
>                  --with-mpfr
>                  --with-xxhash
>                  --with-python=/usr
>                  --with-python-libdir=/tmp/foo
>                  --with-debuginfod
>                  --with-guile
>                  --enable-source-highlight
>                  --with-separate-debug-dir=/usr/local/lib/debug (relocatable)
>
>     ("Relocatable" means the directory can be moved with the GDB installation
>     tree, and GDB will still find it.)
>
> I'll have to investigate that later.

I have not seen this on my Fedora 33 x86_64 machine.

Thanks,
Aaron

Re: [PATCH 2/2] gdb.texinfo: Expand documentation for debuginfod

[Aaron Merey]

Hi Eli,

On Wed, Oct 20, 2021 at 7:37 AM Eli Zaretskii <eliz@gnu.org> wrote:
> Thanks, this is OK.

Pushed as commit 3ea44f21299. For the record I made a couple small
changes to reflect revisions to patch 1/2 in this series. 'debuginfod-urls'
was renamed to 'debuginfod urls', 'debug debuginfod' was renamed
to 'debuginfod verbose' and a one line description for a
'show debuginfod status' command was added.

Thanks,
Aaron

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

[Simon Marchi]

On 2021-10-29 21:09, Aaron Merey wrote:
> Changed to use add_setshow_prefix_cmd. Because this function
> doesn't have a parameter for a custom show function I added
> a 'show debuginfod status' command so that the user can check
> whether debuginfod is set to 'on', 'off' or 'ask'.

Ah, I had not realized that, sorry.  Well, you could have left it
as-is then :/.

Simon

Re: [PATCH 2/2] gdb.texinfo: Expand documentation for debuginfod

[Tom de Vries]

On 10/30/21 3:18 AM, Aaron Merey via Gdb-patches wrote:
> Hi Eli,
> 
> On Wed, Oct 20, 2021 at 7:37 AM Eli Zaretskii <eliz@gnu.org> wrote:
>> Thanks, this is OK.
> 
> Pushed as commit 3ea44f21299. For the record I made a couple small
> changes to reflect revisions to patch 1/2 in this series. 'debuginfod-urls'
> was renamed to 'debuginfod urls', 'debug debuginfod' was renamed
> to 'debuginfod verbose' and a one line description for a
> 'show debuginfod status' command was added.

After this commit I'm running into:
...
Running selftest help_doc_invariants.^M
help doc broken invariant: command 'info set debuginfod' help doc first
line is not terminated with a '.' character^M
help doc broken invariant: command 'set debuginfod' help doc first line
is not terminated with a '.' character^M
help doc broken invariant: command 'show debuginfod' help doc first line
is not terminated with a '.' character^M
Self test failed: self-test failed at
/home/vries/gdb_versions/devel/src/gdb/unittests/command-def-selftests.c:100^M
...

Thanks,
- Tom

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

[Simon Marchi]

On 2021-10-29 21:54, Simon Marchi via Gdb-patches wrote:
> On 2021-10-29 21:09, Aaron Merey wrote:
>> Changed to use add_setshow_prefix_cmd. Because this function
>> doesn't have a parameter for a custom show function I added
>> a 'show debuginfod status' command so that the user can check
>> whether debuginfod is set to 'on', 'off' or 'ask'.
> 
> Ah, I had not realized that, sorry.  Well, you could have left it
> as-is then :/.

Hi Aaron,

I re-visited how I suggested you to do "set debuginfod", which were
based on how I designed "set index-cache", and I think it's not right
after all.

I don't think it was good idea to overload "set index-cache" as a
boolean setting and a prefix command.  First, I had to add the
information of whether index-cache is enabled as custom message:


    (gdb) show index-cache 
    index-cache directory:  The directory of the index cache is "/home/simark/.cache/gdb".
    index-cache stats:  
        Cache hits (this session): 0
      Cache misses (this session): 0

    The index cache is currently disabled.  <---

This is not good, because in "info set" it looks like this:

    (gdb) info set
    ...
    host-charset:  The host character set is "auto; currently UTF-8".
    index-cache directory:  The directory of the index cache is "/home/simark/.cache/gdb".
    index-cache stats:    Cache hits (this session): 0
    Cache misses (this session): 0
    inferior-tty:  Terminal for future runs of program being debugged is "".
    ...

This just looks weird.  Also, because the index-cache on/off knob isn't
a proper setting, it doesn't appear in MI, a frontend can't know if
index-cache is enabled or not:

    -gdb-show index-cache 
    ~"\n"
    ~"    Cache hits (this session): 0\n"
    ~"  Cache misses (this session): 0\n"
    ~"\n"
    ~"The index cache is currently disabled.\n"
    ^done,showlist={option={name="directory",value="/home/simark/.cache/gdb"},option={name="stats"}}

You solved this by adding "set debuginfod status", which is a bit
better, but also not ideal.  It's weird for the set command used to toggle
debuginfod on and off to not mirror the show command used to show the
state.

So, I think that having dedicated commands "set/show index-cache enabled
on/off" and "set/show debuginfod enabled on/off" is better, in the end.
What do you think?

Another mistake I think I did with the index-cache is "show index-cache
stats".  Set/show commands are supposed to be for settings.  This is
just printing some information, not a setting value.  Using a show
command for this means that the index-cache stats are printed when
doing "info set", which makes no sense.  Also, printing the "setting" in
MI is useless:

    -gdb-show index-cache stats
    ~"  Cache hits (this session): 0\n"
    ~"Cache misses (this session): 0\n"
    ^done

I think that this should have been an info command, "info index-cache
stats" maybe.

I'm considering fixing this for the index-cache (deprecating the old
commands, but keeping them as aliases of the new ones).  If you agree, I
could do the same with "set debuginfo" too.

Simon

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

[Simon Marchi]

On 2021-10-30 10:43 p.m., Simon Marchi via Gdb-patches wrote:
> I'm considering fixing this for the index-cache (deprecating the old
> commands, but keeping them as aliases of the new ones).  If you agree, I
> could do the same with "set debuginfo" too.
>
> Simon
>

I sent a patch series changing the index-cache commands here:

  https://sourceware.org/pipermail/gdb-patches/2021-November/182990.html

Please let met know what you think.

Simon

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

[Aaron Merey]

Hi Simon,

On Mon, Nov 1, 2021 at 11:52 AM Simon Marchi <simark@simark.ca> wrote:
>
> On 2021-10-30 10:43 p.m., Simon Marchi via Gdb-patches wrote:
> > I'm considering fixing this for the index-cache (deprecating the old
> > commands, but keeping them as aliases of the new ones).  If you agree, I
> > could do the same with "set debuginfo" too.
> >
> > Simon
> >
>
> I sent a patch series changing the index-cache commands here:
>
>   https://sourceware.org/pipermail/gdb-patches/2021-November/182990.html
>
> Please let met know what you think.

That patch set is definitely an improvement. I think you're right that
'set/show debuginfod enabled on/off' is better than needing a
'show debuginfod status' command that has no corresponding set
command. I also like that setting getters/setters now return scalars
by value.

Do you think we need to keep and deprecate the existing 'set
debuginfod on/off/ask' and 'show debuginfod status' commands?
Personally I'd rather just replace them considering how recently
they were added.

Aaron

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

[Simon Marchi]

On 2021-11-01 1:39 p.m., Aaron Merey wrote:
> Hi Simon,
> 
> On Mon, Nov 1, 2021 at 11:52 AM Simon Marchi <simark@simark.ca> wrote:
>>
>> On 2021-10-30 10:43 p.m., Simon Marchi via Gdb-patches wrote:
>>> I'm considering fixing this for the index-cache (deprecating the old
>>> commands, but keeping them as aliases of the new ones).  If you agree, I
>>> could do the same with "set debuginfo" too.
>>>
>>> Simon
>>>
>>
>> I sent a patch series changing the index-cache commands here:
>>
>>   https://sourceware.org/pipermail/gdb-patches/2021-November/182990.html
>>
>> Please let met know what you think.
> 
> That patch set is definitely an improvement. I think you're right that
> 'set/show debuginfod enabled on/off' is better than needing a
> 'show debuginfod status' command that has no corresponding set
> command. I also like that setting getters/setters now return scalars
> by value.

Ok, and sorry again for suggesting something and then changing my mind.

> 
> Do you think we need to keep and deprecate the existing 'set
> debuginfod on/off/ask' and 'show debuginfod status' commands?
> Personally I'd rather just replace them considering how recently
> they were added.
No, since they were not part of a release.

Simon

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

[Simon Marchi]

On 2021-11-01 2:00 p.m., Simon Marchi wrote:
> On 2021-11-01 1:39 p.m., Aaron Merey wrote:
>> Hi Simon,
>>
>> On Mon, Nov 1, 2021 at 11:52 AM Simon Marchi <simark@simark.ca> wrote:
>>>
>>> On 2021-10-30 10:43 p.m., Simon Marchi via Gdb-patches wrote:
>>>> I'm considering fixing this for the index-cache (deprecating the old
>>>> commands, but keeping them as aliases of the new ones).  If you agree, I
>>>> could do the same with "set debuginfo" too.
>>>>
>>>> Simon
>>>>
>>>
>>> I sent a patch series changing the index-cache commands here:
>>>
>>>   https://sourceware.org/pipermail/gdb-patches/2021-November/182990.html
>>>
>>> Please let met know what you think.
>>
>> That patch set is definitely an improvement. I think you're right that
>> 'set/show debuginfod enabled on/off' is better than needing a
>> 'show debuginfod status' command that has no corresponding set
>> command. I also like that setting getters/setters now return scalars
>> by value.
> 
> Ok, and sorry again for suggesting something and then changing my mind.
> 
>>
>> Do you think we need to keep and deprecate the existing 'set
>> debuginfod on/off/ask' and 'show debuginfod status' commands?
>> Personally I'd rather just replace them considering how recently
>> they were added.
> No, since they were not part of a release.
> 
> Simon
> 

I have a WIP patch to adjust the debuginfod commands that I can post after
my series reworking the index-cache commands is merged (it depeneds on some
patches of that series).

Simon

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

[Aaron Merey]

On Tue, Nov 2, 2021 at 12:51 PM Simon Marchi <simon.marchi@polymtl.ca> wrote:
> I have a WIP patch to adjust the debuginfod commands that I can post after
> my series reworking the index-cache commands is merged (it depeneds on some
> patches of that series).

Thanks a lot Simon. I'll watch for that patch to be posted.

Aaron