From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id B7E5B3857C47 for ; Fri, 29 Oct 2021 01:47:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B7E5B3857C47 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 19T1lglI001877 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 28 Oct 2021 21:47:47 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 19T1lglI001877 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 268BF1ECEB; Thu, 28 Oct 2021 21:47:42 -0400 (EDT) Message-ID: <6137615d-9b33-8e24-4d81-5aa998d7fc1b@polymtl.ca> Date: Thu, 28 Oct 2021 21:47:41 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.1 Subject: Re: [PATCH 1/2] gdb: add set/show commands for managing debuginfod Content-Language: en-US To: Aaron Merey Cc: gdb-patches@sourceware.org References: <20211028221823.574570-1-amerey@redhat.com> From: Simon Marchi In-Reply-To: <20211028221823.574570-1-amerey@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Fri, 29 Oct 2021 01:47:42 +0000 X-Spam-Status: No, score=-10.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_BADIPHTTP, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 29 Oct 2021 01:47:52 -0000 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 > 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: > ... > 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