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 0C0A33858D39 for ; Tue, 26 Oct 2021 16:08:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 0C0A33858D39 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 19QG8GkA022510 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 26 Oct 2021 12:08:21 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 19QG8GkA022510 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 053261ECEB; Tue, 26 Oct 2021 12:08:15 -0400 (EDT) Message-ID: Date: Tue, 26 Oct 2021 12:08:15 -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 , gdb-patches@sourceware.org Cc: simark@simark.ca References: <20211018230133.265619-1-amerey@redhat.com> <20211018230133.265619-2-amerey@redhat.com> From: Simon Marchi In-Reply-To: <20211018230133.265619-2-amerey@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Tue, 26 Oct 2021 16:08:16 +0000 X-Spam-Status: No, score=-10.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, 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: Tue, 26 Oct 2021 16:08:25 -0000 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: > ... > 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 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 > -#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 > > struct user_data > @@ -68,6 +69,63 @@ struct debuginfod_client_deleter > using debuginfod_client_up > = std::unique_ptr; > > +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