[PATCH][gdb/debuginfod] Ask to cancel further downloads

[PATCH][gdb/debuginfod] Ask to cancel further downloads

[Tom de Vries]

Hi,

Say you're debugging an executable and debug info is being downloaded for
shared libraries, and you want to cancel all downloading, there's no option
but to keep ^C-ing:
...
$ gcc ~/hello.c -g
$ rm -Rf ~/.cache/debuginfod_client
$ gdb -q -iex "set debuginfod enabled on" a.out -ex start
Reading symbols from a.out...
Temporary breakpoint 1 at 0x40113a: file /home/vries/hello.c, line 6.
Starting program: /data/vries/gdb_versions/devel/a.out
Downloading separate debug info for /lib64/ld-linux-x86-64.so.2...
^CCancelling download of separate debug info for /lib64/ld-linux-x86-64.so.2...
Downloading separate debug info for system-supplied DSO at 0x7ffff7fc6000...
^CCancelling download of separate debug info for system-supplied DSO at 0x7ffff7fc6000...
Downloading separate debug info for /lib64/libc.so.6...
^CCancelling download of separate debug info for /lib64/libc.so.6...
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".

Temporary breakpoint 1, main () at /home/vries/hello.c:6
6         printf ("hello\n");
(gdb)
...

While this is doable for the 3 shared libraries in this example session, it
becomes burdensome if there are many.

Add a new command "set debuginfod cancel one/all/ask", where:
- "one" means ^C cancels one download,
- "all" means ^C cancels all further downloads, and
- "ask" means ^C asks whether to cancel all further downloads.  A "yes" implies
  "set debuginfod cancel all", and a "no" implies "set debuginfod cancel one", so
  the question is only asked once.

Note that the behaviour as it was before this patch is equivalent to
"set debuginfod cancel one".

Instead, the new default is "set debuginfod cancel ask".

Note that cancelling all further downloads implies "set debuginfod enabled off".

An example session looks like:
...
$ gdb -q -iex "set debuginfod enabled on" a.out -ex start
Reading symbols from a.out...
Temporary breakpoint 1 at 0x40113a: file /home/vries/hello.c, line 6.
Starting program: /data/vries/gdb_versions/devel/a.out
Downloading separate debug info for /lib64/ld-linux-x86-64.so.2...
^CCancelling download of separate debug info for /lib64/ld-linux-x86-64.so.2...
Cancel further downloading for this session? (y or [n]) y
Debuginfod has been disabled.
To re-enable use the 'set debuginfod enabled on' command.
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".

Temporary breakpoint 1, main () at /home/vries/hello.c:6
6         printf ("hello\n");
(gdb)
...

Tested on x86_64-linux.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29582
Suggested-By: Martin Liška <mliska@suse.cz>
Co-Authored-By: Aaron Merey <amerey@redhat.com>

Any comments?

Thanks,
- Tom

[gdb/debuginfod] Ask to cancel further downloads

---
 gdb/debuginfod-support.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/doc/gdb.texinfo      | 20 ++++++++++++++
 2 files changed, 92 insertions(+)

diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index 5f04a2b38ca..a1180016600 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -33,6 +33,8 @@ 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_one[] = "one";
+static const char debuginfod_all[] = "all";
 
 static const char *debuginfod_enabled_enum[] =
 {
@@ -42,6 +44,20 @@ static const char *debuginfod_enabled_enum[] =
   nullptr
 };
 
+/* Valid values for set debuginfod cancel command.  */
+
+static const char *debuginfod_cancel_enum[] =
+{
+  debuginfod_one,
+  debuginfod_all,
+  debuginfod_ask,
+  nullptr
+};
+
+/* Value of debuginfod cancellation mode.  */
+
+static const char *debuginfod_cancel = debuginfod_ask;
+
 static const char *debuginfod_enabled =
 #if defined(HAVE_LIBDEBUGINFOD)
   debuginfod_ask;
@@ -119,6 +135,20 @@ progressfn (debuginfod_client *c, long cur, long total)
       gdb_printf ("Cancelling download of %s %ps...\n",
 		  data->desc,
 		  styled_string (file_name_style.style (), data->fname));
+      if (debuginfod_cancel == debuginfod_ask)
+	{
+	  int resp = nquery (_("Cancel further downloading for this session? "));
+	  if (resp)
+	    debuginfod_cancel = debuginfod_all;
+	  else
+	    debuginfod_cancel = debuginfod_one;
+	}
+      if (debuginfod_cancel == debuginfod_all)
+	{
+	  gdb_printf (_("Debuginfod has been disabled.\nTo re-enable use the"
+			" 'set debuginfod enabled on' command.\n"));
+	  debuginfod_enabled = debuginfod_off;
+	}
       return 1;
     }
 
@@ -393,6 +423,33 @@ show_debuginfod_enabled (ui_file *file, int from_tty, cmd_list_element *cmd,
 		"\"%s\".\n"), debuginfod_enabled);
 }
 
+/* Set callback for "set debuginfod cancel".  */
+
+static void
+set_debuginfod_cancel (const char *value)
+{
+  debuginfod_cancel = value;
+}
+
+/* Get callback for "set debuginfod cancel".  */
+
+static const char *
+get_debuginfod_cancel ()
+{
+  return debuginfod_cancel;
+}
+
+/* Show callback for "set debuginfod cancel".  */
+
+static void
+show_debuginfod_cancel (ui_file *file, int from_tty, cmd_list_element *cmd,
+			 const char *value)
+{
+  gdb_printf (file,
+	      _("Debuginfod cancellation mode is currently set to "
+		"\"%s\".\n"), debuginfod_cancel);
+}
+
 /* Set callback for "set debuginfod urls".  */
 
 static void
@@ -473,6 +530,21 @@ source files."),
 			&set_debuginfod_prefix_list,
 			&show_debuginfod_prefix_list);
 
+  add_setshow_enum_cmd ("cancel", class_run, debuginfod_cancel_enum,
+			_("Set cancellation mode for debuginfod."),
+			_("Show cancellation mode for debuginfod."),
+			_("\
+The cancellation mode controls the behavior of ^C while a file is downloading\
+ from debuginfod.\n\
+When set to one, ^C cancels a single download.\n\
+When set to all, ^C cancels all further downloads.\n\
+When set to ask, ^C asks what to do."),
+			set_debuginfod_cancel,
+			get_debuginfod_cancel,
+			show_debuginfod_cancel,
+			&set_debuginfod_prefix_list,
+			&show_debuginfod_prefix_list);
+
   /* set/show debuginfod urls */
   add_setshow_string_noescape_cmd ("urls", class_run, _("\
 Set the list of debuginfod server URLs."), _("\
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 238a49b027d..718f42fbc02 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -48035,6 +48035,26 @@ is set to @code{ask} for interactive sessions.
 Display whether @code{debuginfod enabled} is set to @code{on}, @code{off} or
 @code{ask}.
 
+@kindex set debuginfod cancel
+@anchor{set debuginfod cancel}
+@item set debuginfod cancel
+@itemx set debuginfod cancel one
+@cindex debuginfod cancellation mode
+Pressing @code{^C} will cancel one download.
+
+@item set debuginfod cancel all
+Pressing @code{^C} will cancel all further downloads.
+
+@item set debuginfod cancel ask
+Pressing @code{^C} will prompt the user to cancel all further downloads before
+attempting to perform the next query.  By default, @code{debuginfod cancel}
+is set to @code{ask} for interactive sessions.
+
+@kindex show debuginfod cancel
+@item show debuginfod cancel
+Display whether @code{debuginfod cancel} is set to @code{one}, @code{all} or
+@code{ask}.
+
 @kindex set debuginfod urls
 @cindex configure debuginfod URLs
 @item set debuginfod urls

Re: [PATCH][gdb/debuginfod] Ask to cancel further downloads

[Aaron Merey]

On Thu, Sep 29, 2022 at 4:34 AM Tom de Vries <tdevries@suse.de> wrote:
> Add a new command "set debuginfod cancel one/all/ask", where:
> - "one" means ^C cancels one download,
> - "all" means ^C cancels all further downloads, and
> - "ask" means ^C asks whether to cancel all further downloads.  A "yes" implies
>   "set debuginfod cancel all", and a "no" implies "set debuginfod cancel one", so
>   the question is only asked once.
>
> Note that the behaviour as it was before this patch is equivalent to
> "set debuginfod cancel one".
>
> Instead, the new default is "set debuginfod cancel ask".
>
> Note that cancelling all further downloads implies "set debuginfod enabled off".

LGTM. It's a usability improvement and it worked as intended during testing.

Aaron

Re: [PATCH][gdb/debuginfod] Ask to cancel further downloads

[Pedro Alves]

On 2022-09-29 8:29 p.m., Aaron Merey via Gdb-patches wrote:
> On Thu, Sep 29, 2022 at 4:34 AM Tom de Vries <tdevries@suse.de> wrote:
>> Add a new command "set debuginfod cancel one/all/ask", where:
>> - "one" means ^C cancels one download,
>> - "all" means ^C cancels all further downloads, and
>> - "ask" means ^C asks whether to cancel all further downloads.  A "yes" implies
>>   "set debuginfod cancel all", and a "no" implies "set debuginfod cancel one", so
>>   the question is only asked once.
>>
>> Note that the behaviour as it was before this patch is equivalent to
>> "set debuginfod cancel one".
>>
>> Instead, the new default is "set debuginfod cancel ask".
>>
>> Note that cancelling all further downloads implies "set debuginfod enabled off".
> 
> LGTM. It's a usability improvement and it worked as intended during testing.

Cool.  I think the thing missing (not in this patch, but in gdb) is handling
^C pressed while the inferior is running and GDB decides to download from debuginfod.

I mean, something like:

(gdb) c
...
# inferior is running, dlopens a library, and gdb downloads debug info from debuginfod

# just while gdb starts downloading one of the libraries, the user presses ^C, intending
# to pause the inferior.

I think gdb always switches to terminal-ours before downloading with debuginfod, and
assumes the ^C indicates the user wanted to cancel the download?

I think in this scenario, we should do something similar to target remote's "pressed twice"
handling.

- user presses once ctrl-c while download is happening _and_ inferior has terminal
    => Do nothing, set quit flag again, so ctrl-c isn't lost.  Set a global flag indicating
       ctrl-c was already pressed once.  The idea is that maybe the download finishes quickly,
       and then gdb soon elsewhere notices the quit_flag and stops the inferior.
       Everything Just Works.

- user is impacient and presses ctrl-c a second time while download is happening _and_
  inferior has terminal
    => gdb asks user what to do: 

       - interrupt program?
       - cancel download?

Pedro Alves

Re: [PATCH][gdb/debuginfod] Ask to cancel further downloads

[Aaron Merey]

On Fri, Sep 30, 2022 at 12:15 PM Pedro Alves <pedro@palves.net> wrote:
> Cool.  I think the thing missing (not in this patch, but in gdb) is handling
> ^C pressed while the inferior is running and GDB decides to download from debuginfod.
>
> I mean, something like:
>
> (gdb) c
> ...
> # inferior is running, dlopens a library, and gdb downloads debug info from debuginfod
>
> # just while gdb starts downloading one of the libraries, the user presses ^C, intending
> # to pause the inferior.
>
> I think gdb always switches to terminal-ours before downloading with debuginfod, and
> assumes the ^C indicates the user wanted to cancel the download?

That's correct.

> I think in this scenario, we should do something similar to target remote's "pressed twice"
> handling.
>
> - user presses once ctrl-c while download is happening _and_ inferior has terminal
>     => Do nothing, set quit flag again, so ctrl-c isn't lost.  Set a global flag indicating
>        ctrl-c was already pressed once.  The idea is that maybe the download finishes quickly,
>        and then gdb soon elsewhere notices the quit_flag and stops the inferior.
>        Everything Just Works.
>
> - user is impacient and presses ctrl-c a second time while download is happening _and_
>   inferior has terminal
>     => gdb asks user what to do:
>
>        - interrupt program?
>        - cancel download?

This should help eliminate some ambiguity around ctrl-c.  If there are two
ctrl-c during a download we can first ask whether to interrupt the program
or cancel the download.  If the user wishes to cancel a download then we
can ask whether to cancel all downloads or just the current one, like
what's been implemented in this patch.

This patch should still be worth merging alone though, with the ctrl-c
tweak added in a separate patch.

Aaron