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

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

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