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

[Aaron Merey <amerey@redhat.com>]

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