[PATCH v4] gdb/debuginfod: Ctrl-C asks to cancel further downloads

[PATCH v4] gdb/debuginfod: Ctrl-C asks to cancel further downloads

[Aaron Merey]

v3: https://sourceware.org/pipermail/gdb-patches/2023-March/197679.html

v4 includes changes needed to apply the patch to the current master branch.

Currently gdb only allows for debuginfod downloads to be cancelled one at
a time via Ctrl-C.  This can be a burden if one wishes to cancel a large
series of downloads.  Additionally there can also be ambiguity between whether
Ctrl-C during a download was intended to cancel a download or interrupt the
inferior.

This patch addresses these issues by adding a new debuginfod setting and
changing the behavior of Ctrl-C during a download.

Add a new command "set debuginfod cancel one/all/ask", where:
- "one" means Ctrl-C cancels one download,
- "all" means Ctrl-C cancels all further downloads, and
- "ask" means Ctrl-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".

A single Ctrl-C during downloading now sets the quit_flag and proceeds with
all downloads.  If the inferior has the terminal, then a second Ctrl-C during
downloading triggers a query asking whether to cancel the download or
interrupt the inferior.  If the user wishes to cancel the download then
the setting of 'set debuginfod cancel' determines whether one or all
downloads are cancelled.  In the case of "set debuginfod cancel ask",
there will be another query at this point asking whether to cancel one
or all downloads.

If the inferior does not have the terminal, then a second Ctrl-C during
downloading simply cancels the download according to the setting of
"set debuginfod cancel".  In this case there is no query asking whether
to interrupt the inferior or cancel a download.

Example session where inferior has terminal:

    (gdb) run
    [...]
    Downloading separate debug info for /lib64/libxyz.so
    [###                                                            ]^C^C
    Cancel the current download?
    If no, then Ctrl-C will be sent to the target process. ([y] or n) y
    Cancelling download of separate debug info for /lib64/libxyz.so...
    Cancel further downloading for this session? (y or [n]) n
    Downloading separate debug info for /lib64/libabcd.so

Example session where inferior does not have terminal:

    (gdb) run
    [...]
    Downloading separate debug info for /lib64/libxyz.so
    [###                                                            ]^C^C
    Cancelling download of separate debug info for /lib64/libxyz.so...
    Cancel further downloading for this session? (y or [n]) y
    Debuginfod has been disabled.
    To re-enable use the 'set debuginfod enabled' command.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29582
Suggested-By: Martin Liška <mliska@suse.cz>
Co-Authored-By: Tom de Vries <tdevries@suse.de>
---
 gdb/debuginfod-support.c | 183 +++++++++++++++++++++++++++++++++++++--
 gdb/doc/gdb.texinfo      |  21 +++++
 2 files changed, 199 insertions(+), 5 deletions(-)

diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index 7d8ada39e96..97d4b2f2bac 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -38,6 +38,8 @@ static cmd_list_element *maint_show_debuginfod_cmdlist;
 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[] =
 {
@@ -47,6 +49,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;
@@ -109,9 +125,11 @@ debuginfod_section_query (const unsigned char *build_id,
 struct user_data
 {
   user_data (const char *desc, const char *fname)
-    : desc (desc), fname (fname)
+    : pass_quit_flag (false), inf_had_term (false), desc (desc), fname (fname)
   { }
 
+  bool pass_quit_flag;
+  bool inf_had_term;
   const char * const desc;
   const char * const fname;
   ui_out::progress_update progress;
@@ -156,10 +174,83 @@ progressfn (debuginfod_client *c, long cur, long total)
   if (check_quit_flag ())
     {
       ui_file *outstream = get_unbuffered (gdb_stdout);
-      gdb_printf (outstream, _("Cancelling download of %s %s...\n"),
-		  data->desc, styled_fname.c_str ());
-      return 1;
-    }
+
+      /* If a single Ctrl-C occurs during downloading, let it propagate to the
+	 target.  If more than one Ctrl-C occurs, ask whether to cancel the
+	 current download or interrupt the target.  If the download is
+	 cancelled, the setting of debuginfod_cancel will determine whether
+	 the current download is cancelled or debuginfod is disabled.  */
+      if (!data->pass_quit_flag)
+	data->pass_quit_flag = true;
+      else
+	{
+	  int resp = 1;
+	  bool extra_nl = false;
+
+	  if (data->inf_had_term)
+	    {
+	      /* If Ctrl-C occurs during the following prompts, catch the
+		 exception to prevent unsafe early returns to gdb's main
+		 event loop.  During these prompts, Ctrl-C is equivalent to
+		 answering 'y'.  */
+	      try
+		{
+		  resp = yquery (_("Cancel the current download?\nIf no, "
+				   "then Ctrl-C will be sent to the target "
+				   "process. "));
+		}
+	      catch (const gdb_exception &)
+		{
+		  /* If the query doesn't complete, then we need an additional
+		     newline to get "Cancelling download of..." printed on a
+		     separate line.  */
+		  extra_nl = true;
+		}
+	    }
+	  if (resp)
+	    {
+	      if (extra_nl)
+		{
+		  gdb_printf (outstream, "\n");
+		  extra_nl = false;
+		}
+
+	      gdb_printf (outstream, _("Cancelling download of %s %s...\n"),
+				       data->desc, styled_fname.c_str ());
+	      if (debuginfod_cancel == debuginfod_ask)
+		{
+		  try
+		    {
+		      resp = nquery
+			(_("Cancel further downloading for this session? "));
+		    }
+		  catch (const gdb_exception &)
+		    {
+		      resp = 1;
+		      extra_nl = true;
+		    }
+
+		  if (resp)
+		    debuginfod_cancel = debuginfod_all;
+		  else
+		    debuginfod_cancel = debuginfod_one;
+		}
+	      if (debuginfod_cancel == debuginfod_all)
+		{
+		  if (extra_nl)
+		    gdb_printf (outstream, "\n");
+
+		  gdb_printf (outstream,
+			      _("Debuginfod has been disabled.\nTo re-enable "
+				"use the 'set debuginfod enabled' command.\n"));
+		  debuginfod_enabled = debuginfod_off;
+		}
+
+	    data->pass_quit_flag = false;
+	    return 1;
+	  }
+      }
+  }
 
   if (debuginfod_verbose == 0)
     return 0;
@@ -330,6 +421,10 @@ debuginfod_source_query (const unsigned char *build_id,
     user_data data ("source file", srcpath);
 
     debuginfod_set_user_data (c, &data);
+
+   if (!target_terminal::is_ours ())
+     data.inf_had_term = true;
+
     if (target_supports_terminal_ours ())
       {
 	term_state.emplace ();
@@ -341,6 +436,11 @@ debuginfod_source_query (const unsigned char *build_id,
 					    build_id_len,
 					    srcpath,
 					    &dname));
+    if (data.pass_quit_flag)
+      set_quit_flag ();
+    if (data.inf_had_term && term_state.has_value ())
+      target_terminal::inferior ();
+
     debuginfod_set_user_data (c, nullptr);
   }
 
@@ -376,6 +476,10 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
     user_data data ("separate debug info for", filename);
 
     debuginfod_set_user_data (c, &data);
+
+    if (!target_terminal::is_ours ())
+      data.inf_had_term = true;
+
     if (target_supports_terminal_ours ())
       {
 	term_state.emplace ();
@@ -384,6 +488,12 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
 
     fd = scoped_fd (debuginfod_find_debuginfo (c, build_id, build_id_len,
 					       &dname));
+
+    if (data.pass_quit_flag)
+      set_quit_flag ();
+    if (data.inf_had_term && term_state.has_value ())
+      target_terminal::inferior ();
+
     debuginfod_set_user_data (c, nullptr);
   }
 
@@ -419,6 +529,10 @@ debuginfod_exec_query (const unsigned char *build_id,
     user_data data ("executable for", filename);
 
     debuginfod_set_user_data (c, &data);
+
+    if (!target_terminal::is_ours ())
+      data.inf_had_term = true;
+
     if (target_supports_terminal_ours ())
       {
 	term_state.emplace ();
@@ -427,6 +541,11 @@ debuginfod_exec_query (const unsigned char *build_id,
 
     fd = scoped_fd (debuginfod_find_executable (c, build_id, build_id_len,
 						&dname));
+    if (data.pass_quit_flag)
+      set_quit_flag ();
+    if (data.inf_had_term && term_state.has_value ())
+      target_terminal::inferior ();
+
     debuginfod_set_user_data (c, nullptr);
   }
 
@@ -467,6 +586,10 @@ debuginfod_section_query (const unsigned char *build_id,
   {
     user_data data (desc.c_str (), filename);
     debuginfod_set_user_data (c, &data);
+
+    if (!target_terminal::is_ours ())
+      data.inf_had_term = true;
+
     if (target_supports_terminal_ours ())
       {
 	term_state.emplace ();
@@ -475,6 +598,12 @@ debuginfod_section_query (const unsigned char *build_id,
 
     fd = scoped_fd (debuginfod_find_section (c, build_id, build_id_len,
 					     section_name, &dname));
+
+    if (data.pass_quit_flag)
+      set_quit_flag ();
+    if (data.inf_had_term && term_state.has_value ())
+      target_terminal::inferior ();
+
     debuginfod_set_user_data (c, nullptr);
   }
 
@@ -523,6 +652,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
@@ -627,6 +783,23 @@ When set to \"ask\", prompt whether to enable or disable debuginfod." ),
 			&set_debuginfod_prefix_list,
 			&show_debuginfod_prefix_list);
 
+  add_setshow_enum_cmd ("cancel", class_run, debuginfod_cancel_enum,
+			_("Set Ctrl-C behaviour for debuginfod."),
+			_("Show Ctrl-C behaviour for debuginfod."),
+			_("\
+When set to \'one\', pressing Ctrl-C twice cancels a single \
+download.\nWhen set to \'all\', pressing Ctrl-C twice cancels all further downloads.\n\
+When set to \'ask\', pressing Ctrl-C twice asks what to do.\nA single Ctrl-C during \
+downloading is passed to the target process being debugged.\nA second Ctrl-C \
+during downloading may raise a prompt asking whether to cancel the download or \
+send Ctrl-C to the target.\nIf the download is cancelled, then no Ctrl-C is \
+sent to the target."),
+			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 e98c15242bc..d6df28896b1 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -50259,6 +50259,27 @@ 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 @kbd{Ctrl-C} behaviour
+Pressing @kbd{Ctrl-C} twice during downloading will cancel the current
+download.
+
+@item set debuginfod cancel all
+Pressing @kbd{Ctrl-C} twice during downloading will cancel this and all
+further downloads.
+
+@item set debuginfod cancel ask
+Pressing @kbd{Ctrl-C} twice during downloading will cancel the current
+download and prompt whether to cancel all further downloads.  By default,
+@code{debuginfod cancel} is set to @code{ask} for interactive sessions.
+
+@kindex show debuginfod cancel
+@item show debuginfod cancel
+Display the current setting of @code{debuginfod cancel}.
+
 @kindex set debuginfod urls
 @cindex configure debuginfod URLs
 @item set debuginfod urls
-- 
2.43.0

[PING][PATCH v4] gdb/debuginfod: Ctrl-C asks to cancel further downloads

[Aaron Merey]

Ping

Thanks,
Aaron

On Fri, Feb 2, 2024 at 9:15 PM Aaron Merey <amerey@redhat.com> wrote:
>
> v3: https://sourceware.org/pipermail/gdb-patches/2023-March/197679.html
>
> v4 includes changes needed to apply the patch to the current master branch.
>
> Currently gdb only allows for debuginfod downloads to be cancelled one at
> a time via Ctrl-C.  This can be a burden if one wishes to cancel a large
> series of downloads.  Additionally there can also be ambiguity between whether
> Ctrl-C during a download was intended to cancel a download or interrupt the
> inferior.
>
> This patch addresses these issues by adding a new debuginfod setting and
> changing the behavior of Ctrl-C during a download.
>
> Add a new command "set debuginfod cancel one/all/ask", where:
> - "one" means Ctrl-C cancels one download,
> - "all" means Ctrl-C cancels all further downloads, and
> - "ask" means Ctrl-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".
>
> A single Ctrl-C during downloading now sets the quit_flag and proceeds with
> all downloads.  If the inferior has the terminal, then a second Ctrl-C during
> downloading triggers a query asking whether to cancel the download or
> interrupt the inferior.  If the user wishes to cancel the download then
> the setting of 'set debuginfod cancel' determines whether one or all
> downloads are cancelled.  In the case of "set debuginfod cancel ask",
> there will be another query at this point asking whether to cancel one
> or all downloads.
>
> If the inferior does not have the terminal, then a second Ctrl-C during
> downloading simply cancels the download according to the setting of
> "set debuginfod cancel".  In this case there is no query asking whether
> to interrupt the inferior or cancel a download.
>
> Example session where inferior has terminal:
>
>     (gdb) run
>     [...]
>     Downloading separate debug info for /lib64/libxyz.so
>     [###                                                            ]^C^C
>     Cancel the current download?
>     If no, then Ctrl-C will be sent to the target process. ([y] or n) y
>     Cancelling download of separate debug info for /lib64/libxyz.so...
>     Cancel further downloading for this session? (y or [n]) n
>     Downloading separate debug info for /lib64/libabcd.so
>
> Example session where inferior does not have terminal:
>
>     (gdb) run
>     [...]
>     Downloading separate debug info for /lib64/libxyz.so
>     [###                                                            ]^C^C
>     Cancelling download of separate debug info for /lib64/libxyz.so...
>     Cancel further downloading for this session? (y or [n]) y
>     Debuginfod has been disabled.
>     To re-enable use the 'set debuginfod enabled' command.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29582
> Suggested-By: Martin Liška <mliska@suse.cz>
> Co-Authored-By: Tom de Vries <tdevries@suse.de>
> ---
>  gdb/debuginfod-support.c | 183 +++++++++++++++++++++++++++++++++++++--
>  gdb/doc/gdb.texinfo      |  21 +++++
>  2 files changed, 199 insertions(+), 5 deletions(-)
>
> diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
> index 7d8ada39e96..97d4b2f2bac 100644
> --- a/gdb/debuginfod-support.c
> +++ b/gdb/debuginfod-support.c
> @@ -38,6 +38,8 @@ static cmd_list_element *maint_show_debuginfod_cmdlist;
>  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[] =
>  {
> @@ -47,6 +49,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;
> @@ -109,9 +125,11 @@ debuginfod_section_query (const unsigned char *build_id,
>  struct user_data
>  {
>    user_data (const char *desc, const char *fname)
> -    : desc (desc), fname (fname)
> +    : pass_quit_flag (false), inf_had_term (false), desc (desc), fname (fname)
>    { }
>
> +  bool pass_quit_flag;
> +  bool inf_had_term;
>    const char * const desc;
>    const char * const fname;
>    ui_out::progress_update progress;
> @@ -156,10 +174,83 @@ progressfn (debuginfod_client *c, long cur, long total)
>    if (check_quit_flag ())
>      {
>        ui_file *outstream = get_unbuffered (gdb_stdout);
> -      gdb_printf (outstream, _("Cancelling download of %s %s...\n"),
> -                 data->desc, styled_fname.c_str ());
> -      return 1;
> -    }
> +
> +      /* If a single Ctrl-C occurs during downloading, let it propagate to the
> +        target.  If more than one Ctrl-C occurs, ask whether to cancel the
> +        current download or interrupt the target.  If the download is
> +        cancelled, the setting of debuginfod_cancel will determine whether
> +        the current download is cancelled or debuginfod is disabled.  */
> +      if (!data->pass_quit_flag)
> +       data->pass_quit_flag = true;
> +      else
> +       {
> +         int resp = 1;
> +         bool extra_nl = false;
> +
> +         if (data->inf_had_term)
> +           {
> +             /* If Ctrl-C occurs during the following prompts, catch the
> +                exception to prevent unsafe early returns to gdb's main
> +                event loop.  During these prompts, Ctrl-C is equivalent to
> +                answering 'y'.  */
> +             try
> +               {
> +                 resp = yquery (_("Cancel the current download?\nIf no, "
> +                                  "then Ctrl-C will be sent to the target "
> +                                  "process. "));
> +               }
> +             catch (const gdb_exception &)
> +               {
> +                 /* If the query doesn't complete, then we need an additional
> +                    newline to get "Cancelling download of..." printed on a
> +                    separate line.  */
> +                 extra_nl = true;
> +               }
> +           }
> +         if (resp)
> +           {
> +             if (extra_nl)
> +               {
> +                 gdb_printf (outstream, "\n");
> +                 extra_nl = false;
> +               }
> +
> +             gdb_printf (outstream, _("Cancelling download of %s %s...\n"),
> +                                      data->desc, styled_fname.c_str ());
> +             if (debuginfod_cancel == debuginfod_ask)
> +               {
> +                 try
> +                   {
> +                     resp = nquery
> +                       (_("Cancel further downloading for this session? "));
> +                   }
> +                 catch (const gdb_exception &)
> +                   {
> +                     resp = 1;
> +                     extra_nl = true;
> +                   }
> +
> +                 if (resp)
> +                   debuginfod_cancel = debuginfod_all;
> +                 else
> +                   debuginfod_cancel = debuginfod_one;
> +               }
> +             if (debuginfod_cancel == debuginfod_all)
> +               {
> +                 if (extra_nl)
> +                   gdb_printf (outstream, "\n");
> +
> +                 gdb_printf (outstream,
> +                             _("Debuginfod has been disabled.\nTo re-enable "
> +                               "use the 'set debuginfod enabled' command.\n"));
> +                 debuginfod_enabled = debuginfod_off;
> +               }
> +
> +           data->pass_quit_flag = false;
> +           return 1;
> +         }
> +      }
> +  }
>
>    if (debuginfod_verbose == 0)
>      return 0;
> @@ -330,6 +421,10 @@ debuginfod_source_query (const unsigned char *build_id,
>      user_data data ("source file", srcpath);
>
>      debuginfod_set_user_data (c, &data);
> +
> +   if (!target_terminal::is_ours ())
> +     data.inf_had_term = true;
> +
>      if (target_supports_terminal_ours ())
>        {
>         term_state.emplace ();
> @@ -341,6 +436,11 @@ debuginfod_source_query (const unsigned char *build_id,
>                                             build_id_len,
>                                             srcpath,
>                                             &dname));
> +    if (data.pass_quit_flag)
> +      set_quit_flag ();
> +    if (data.inf_had_term && term_state.has_value ())
> +      target_terminal::inferior ();
> +
>      debuginfod_set_user_data (c, nullptr);
>    }
>
> @@ -376,6 +476,10 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
>      user_data data ("separate debug info for", filename);
>
>      debuginfod_set_user_data (c, &data);
> +
> +    if (!target_terminal::is_ours ())
> +      data.inf_had_term = true;
> +
>      if (target_supports_terminal_ours ())
>        {
>         term_state.emplace ();
> @@ -384,6 +488,12 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
>
>      fd = scoped_fd (debuginfod_find_debuginfo (c, build_id, build_id_len,
>                                                &dname));
> +
> +    if (data.pass_quit_flag)
> +      set_quit_flag ();
> +    if (data.inf_had_term && term_state.has_value ())
> +      target_terminal::inferior ();
> +
>      debuginfod_set_user_data (c, nullptr);
>    }
>
> @@ -419,6 +529,10 @@ debuginfod_exec_query (const unsigned char *build_id,
>      user_data data ("executable for", filename);
>
>      debuginfod_set_user_data (c, &data);
> +
> +    if (!target_terminal::is_ours ())
> +      data.inf_had_term = true;
> +
>      if (target_supports_terminal_ours ())
>        {
>         term_state.emplace ();
> @@ -427,6 +541,11 @@ debuginfod_exec_query (const unsigned char *build_id,
>
>      fd = scoped_fd (debuginfod_find_executable (c, build_id, build_id_len,
>                                                 &dname));
> +    if (data.pass_quit_flag)
> +      set_quit_flag ();
> +    if (data.inf_had_term && term_state.has_value ())
> +      target_terminal::inferior ();
> +
>      debuginfod_set_user_data (c, nullptr);
>    }
>
> @@ -467,6 +586,10 @@ debuginfod_section_query (const unsigned char *build_id,
>    {
>      user_data data (desc.c_str (), filename);
>      debuginfod_set_user_data (c, &data);
> +
> +    if (!target_terminal::is_ours ())
> +      data.inf_had_term = true;
> +
>      if (target_supports_terminal_ours ())
>        {
>         term_state.emplace ();
> @@ -475,6 +598,12 @@ debuginfod_section_query (const unsigned char *build_id,
>
>      fd = scoped_fd (debuginfod_find_section (c, build_id, build_id_len,
>                                              section_name, &dname));
> +
> +    if (data.pass_quit_flag)
> +      set_quit_flag ();
> +    if (data.inf_had_term && term_state.has_value ())
> +      target_terminal::inferior ();
> +
>      debuginfod_set_user_data (c, nullptr);
>    }
>
> @@ -523,6 +652,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
> @@ -627,6 +783,23 @@ When set to \"ask\", prompt whether to enable or disable debuginfod." ),
>                         &set_debuginfod_prefix_list,
>                         &show_debuginfod_prefix_list);
>
> +  add_setshow_enum_cmd ("cancel", class_run, debuginfod_cancel_enum,
> +                       _("Set Ctrl-C behaviour for debuginfod."),
> +                       _("Show Ctrl-C behaviour for debuginfod."),
> +                       _("\
> +When set to \'one\', pressing Ctrl-C twice cancels a single \
> +download.\nWhen set to \'all\', pressing Ctrl-C twice cancels all further downloads.\n\
> +When set to \'ask\', pressing Ctrl-C twice asks what to do.\nA single Ctrl-C during \
> +downloading is passed to the target process being debugged.\nA second Ctrl-C \
> +during downloading may raise a prompt asking whether to cancel the download or \
> +send Ctrl-C to the target.\nIf the download is cancelled, then no Ctrl-C is \
> +sent to the target."),
> +                       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 e98c15242bc..d6df28896b1 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -50259,6 +50259,27 @@ 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 @kbd{Ctrl-C} behaviour
> +Pressing @kbd{Ctrl-C} twice during downloading will cancel the current
> +download.
> +
> +@item set debuginfod cancel all
> +Pressing @kbd{Ctrl-C} twice during downloading will cancel this and all
> +further downloads.
> +
> +@item set debuginfod cancel ask
> +Pressing @kbd{Ctrl-C} twice during downloading will cancel the current
> +download and prompt whether to cancel all further downloads.  By default,
> +@code{debuginfod cancel} is set to @code{ask} for interactive sessions.
> +
> +@kindex show debuginfod cancel
> +@item show debuginfod cancel
> +Display the current setting of @code{debuginfod cancel}.
> +
>  @kindex set debuginfod urls
>  @cindex configure debuginfod URLs
>  @item set debuginfod urls
> --
> 2.43.0
>

Re: [PING][PATCH v4] gdb/debuginfod: Ctrl-C asks to cancel further downloads

[Eli Zaretskii]

> From: Aaron Merey <amerey@redhat.com>
> Date: Thu, 22 Feb 2024 17:26:27 -0500
> Cc: Tom de Vries <tdevries@suse.de>
> 
> Ping

Sorry for missing this.  The documentation parts of this patch are
okay.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>

[PING*2][PATCH v4] gdb/debuginfod: Ctrl-C asks to cancel further downloads

[Aaron Merey]

On Fri, Feb 23, 2024 at 3:07 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> Sorry for missing this.  The documentation parts of this patch are
> okay.
>
> Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Thanks Eli.

Ping re. the non-documentation parts of the patch.

Aaron

On Thu, Feb 22, 2024 at 5:26 PM Aaron Merey <amerey@redhat.com> wrote:
>
> Ping
>
> Thanks,
> Aaron
>
> On Fri, Feb 2, 2024 at 9:15 PM Aaron Merey <amerey@redhat.com> wrote:
> >
> > v3: https://sourceware.org/pipermail/gdb-patches/2023-March/197679.html
> >
> > v4 includes changes needed to apply the patch to the current master branch.
> >
> > Currently gdb only allows for debuginfod downloads to be cancelled one at
> > a time via Ctrl-C.  This can be a burden if one wishes to cancel a large
> > series of downloads.  Additionally there can also be ambiguity between whether
> > Ctrl-C during a download was intended to cancel a download or interrupt the
> > inferior.
> >
> > This patch addresses these issues by adding a new debuginfod setting and
> > changing the behavior of Ctrl-C during a download.
> >
> > Add a new command "set debuginfod cancel one/all/ask", where:
> > - "one" means Ctrl-C cancels one download,
> > - "all" means Ctrl-C cancels all further downloads, and
> > - "ask" means Ctrl-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".
> >
> > A single Ctrl-C during downloading now sets the quit_flag and proceeds with
> > all downloads.  If the inferior has the terminal, then a second Ctrl-C during
> > downloading triggers a query asking whether to cancel the download or
> > interrupt the inferior.  If the user wishes to cancel the download then
> > the setting of 'set debuginfod cancel' determines whether one or all
> > downloads are cancelled.  In the case of "set debuginfod cancel ask",
> > there will be another query at this point asking whether to cancel one
> > or all downloads.
> >
> > If the inferior does not have the terminal, then a second Ctrl-C during
> > downloading simply cancels the download according to the setting of
> > "set debuginfod cancel".  In this case there is no query asking whether
> > to interrupt the inferior or cancel a download.
> >
> > Example session where inferior has terminal:
> >
> >     (gdb) run
> >     [...]
> >     Downloading separate debug info for /lib64/libxyz.so
> >     [###                                                            ]^C^C
> >     Cancel the current download?
> >     If no, then Ctrl-C will be sent to the target process. ([y] or n) y
> >     Cancelling download of separate debug info for /lib64/libxyz.so...
> >     Cancel further downloading for this session? (y or [n]) n
> >     Downloading separate debug info for /lib64/libabcd.so
> >
> > Example session where inferior does not have terminal:
> >
> >     (gdb) run
> >     [...]
> >     Downloading separate debug info for /lib64/libxyz.so
> >     [###                                                            ]^C^C
> >     Cancelling download of separate debug info for /lib64/libxyz.so...
> >     Cancel further downloading for this session? (y or [n]) y
> >     Debuginfod has been disabled.
> >     To re-enable use the 'set debuginfod enabled' command.
> >
> > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29582
> > Suggested-By: Martin Liška <mliska@suse.cz>
> > Co-Authored-By: Tom de Vries <tdevries@suse.de>
> > ---
> >  gdb/debuginfod-support.c | 183 +++++++++++++++++++++++++++++++++++++--
> >  gdb/doc/gdb.texinfo      |  21 +++++
> >  2 files changed, 199 insertions(+), 5 deletions(-)
> >
> > diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
> > index 7d8ada39e96..97d4b2f2bac 100644
> > --- a/gdb/debuginfod-support.c
> > +++ b/gdb/debuginfod-support.c
> > @@ -38,6 +38,8 @@ static cmd_list_element *maint_show_debuginfod_cmdlist;
> >  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[] =
> >  {
> > @@ -47,6 +49,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;
> > @@ -109,9 +125,11 @@ debuginfod_section_query (const unsigned char *build_id,
> >  struct user_data
> >  {
> >    user_data (const char *desc, const char *fname)
> > -    : desc (desc), fname (fname)
> > +    : pass_quit_flag (false), inf_had_term (false), desc (desc), fname (fname)
> >    { }
> >
> > +  bool pass_quit_flag;
> > +  bool inf_had_term;
> >    const char * const desc;
> >    const char * const fname;
> >    ui_out::progress_update progress;
> > @@ -156,10 +174,83 @@ progressfn (debuginfod_client *c, long cur, long total)
> >    if (check_quit_flag ())
> >      {
> >        ui_file *outstream = get_unbuffered (gdb_stdout);
> > -      gdb_printf (outstream, _("Cancelling download of %s %s...\n"),
> > -                 data->desc, styled_fname.c_str ());
> > -      return 1;
> > -    }
> > +
> > +      /* If a single Ctrl-C occurs during downloading, let it propagate to the
> > +        target.  If more than one Ctrl-C occurs, ask whether to cancel the
> > +        current download or interrupt the target.  If the download is
> > +        cancelled, the setting of debuginfod_cancel will determine whether
> > +        the current download is cancelled or debuginfod is disabled.  */
> > +      if (!data->pass_quit_flag)
> > +       data->pass_quit_flag = true;
> > +      else
> > +       {
> > +         int resp = 1;
> > +         bool extra_nl = false;
> > +
> > +         if (data->inf_had_term)
> > +           {
> > +             /* If Ctrl-C occurs during the following prompts, catch the
> > +                exception to prevent unsafe early returns to gdb's main
> > +                event loop.  During these prompts, Ctrl-C is equivalent to
> > +                answering 'y'.  */
> > +             try
> > +               {
> > +                 resp = yquery (_("Cancel the current download?\nIf no, "
> > +                                  "then Ctrl-C will be sent to the target "
> > +                                  "process. "));
> > +               }
> > +             catch (const gdb_exception &)
> > +               {
> > +                 /* If the query doesn't complete, then we need an additional
> > +                    newline to get "Cancelling download of..." printed on a
> > +                    separate line.  */
> > +                 extra_nl = true;
> > +               }
> > +           }
> > +         if (resp)
> > +           {
> > +             if (extra_nl)
> > +               {
> > +                 gdb_printf (outstream, "\n");
> > +                 extra_nl = false;
> > +               }
> > +
> > +             gdb_printf (outstream, _("Cancelling download of %s %s...\n"),
> > +                                      data->desc, styled_fname.c_str ());
> > +             if (debuginfod_cancel == debuginfod_ask)
> > +               {
> > +                 try
> > +                   {
> > +                     resp = nquery
> > +                       (_("Cancel further downloading for this session? "));
> > +                   }
> > +                 catch (const gdb_exception &)
> > +                   {
> > +                     resp = 1;
> > +                     extra_nl = true;
> > +                   }
> > +
> > +                 if (resp)
> > +                   debuginfod_cancel = debuginfod_all;
> > +                 else
> > +                   debuginfod_cancel = debuginfod_one;
> > +               }
> > +             if (debuginfod_cancel == debuginfod_all)
> > +               {
> > +                 if (extra_nl)
> > +                   gdb_printf (outstream, "\n");
> > +
> > +                 gdb_printf (outstream,
> > +                             _("Debuginfod has been disabled.\nTo re-enable "
> > +                               "use the 'set debuginfod enabled' command.\n"));
> > +                 debuginfod_enabled = debuginfod_off;
> > +               }
> > +
> > +           data->pass_quit_flag = false;
> > +           return 1;
> > +         }
> > +      }
> > +  }
> >
> >    if (debuginfod_verbose == 0)
> >      return 0;
> > @@ -330,6 +421,10 @@ debuginfod_source_query (const unsigned char *build_id,
> >      user_data data ("source file", srcpath);
> >
> >      debuginfod_set_user_data (c, &data);
> > +
> > +   if (!target_terminal::is_ours ())
> > +     data.inf_had_term = true;
> > +
> >      if (target_supports_terminal_ours ())
> >        {
> >         term_state.emplace ();
> > @@ -341,6 +436,11 @@ debuginfod_source_query (const unsigned char *build_id,
> >                                             build_id_len,
> >                                             srcpath,
> >                                             &dname));
> > +    if (data.pass_quit_flag)
> > +      set_quit_flag ();
> > +    if (data.inf_had_term && term_state.has_value ())
> > +      target_terminal::inferior ();
> > +
> >      debuginfod_set_user_data (c, nullptr);
> >    }
> >
> > @@ -376,6 +476,10 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
> >      user_data data ("separate debug info for", filename);
> >
> >      debuginfod_set_user_data (c, &data);
> > +
> > +    if (!target_terminal::is_ours ())
> > +      data.inf_had_term = true;
> > +
> >      if (target_supports_terminal_ours ())
> >        {
> >         term_state.emplace ();
> > @@ -384,6 +488,12 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
> >
> >      fd = scoped_fd (debuginfod_find_debuginfo (c, build_id, build_id_len,
> >                                                &dname));
> > +
> > +    if (data.pass_quit_flag)
> > +      set_quit_flag ();
> > +    if (data.inf_had_term && term_state.has_value ())
> > +      target_terminal::inferior ();
> > +
> >      debuginfod_set_user_data (c, nullptr);
> >    }
> >
> > @@ -419,6 +529,10 @@ debuginfod_exec_query (const unsigned char *build_id,
> >      user_data data ("executable for", filename);
> >
> >      debuginfod_set_user_data (c, &data);
> > +
> > +    if (!target_terminal::is_ours ())
> > +      data.inf_had_term = true;
> > +
> >      if (target_supports_terminal_ours ())
> >        {
> >         term_state.emplace ();
> > @@ -427,6 +541,11 @@ debuginfod_exec_query (const unsigned char *build_id,
> >
> >      fd = scoped_fd (debuginfod_find_executable (c, build_id, build_id_len,
> >                                                 &dname));
> > +    if (data.pass_quit_flag)
> > +      set_quit_flag ();
> > +    if (data.inf_had_term && term_state.has_value ())
> > +      target_terminal::inferior ();
> > +
> >      debuginfod_set_user_data (c, nullptr);
> >    }
> >
> > @@ -467,6 +586,10 @@ debuginfod_section_query (const unsigned char *build_id,
> >    {
> >      user_data data (desc.c_str (), filename);
> >      debuginfod_set_user_data (c, &data);
> > +
> > +    if (!target_terminal::is_ours ())
> > +      data.inf_had_term = true;
> > +
> >      if (target_supports_terminal_ours ())
> >        {
> >         term_state.emplace ();
> > @@ -475,6 +598,12 @@ debuginfod_section_query (const unsigned char *build_id,
> >
> >      fd = scoped_fd (debuginfod_find_section (c, build_id, build_id_len,
> >                                              section_name, &dname));
> > +
> > +    if (data.pass_quit_flag)
> > +      set_quit_flag ();
> > +    if (data.inf_had_term && term_state.has_value ())
> > +      target_terminal::inferior ();
> > +
> >      debuginfod_set_user_data (c, nullptr);
> >    }
> >
> > @@ -523,6 +652,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
> > @@ -627,6 +783,23 @@ When set to \"ask\", prompt whether to enable or disable debuginfod." ),
> >                         &set_debuginfod_prefix_list,
> >                         &show_debuginfod_prefix_list);
> >
> > +  add_setshow_enum_cmd ("cancel", class_run, debuginfod_cancel_enum,
> > +                       _("Set Ctrl-C behaviour for debuginfod."),
> > +                       _("Show Ctrl-C behaviour for debuginfod."),
> > +                       _("\
> > +When set to \'one\', pressing Ctrl-C twice cancels a single \
> > +download.\nWhen set to \'all\', pressing Ctrl-C twice cancels all further downloads.\n\
> > +When set to \'ask\', pressing Ctrl-C twice asks what to do.\nA single Ctrl-C during \
> > +downloading is passed to the target process being debugged.\nA second Ctrl-C \
> > +during downloading may raise a prompt asking whether to cancel the download or \
> > +send Ctrl-C to the target.\nIf the download is cancelled, then no Ctrl-C is \
> > +sent to the target."),
> > +                       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 e98c15242bc..d6df28896b1 100644
> > --- a/gdb/doc/gdb.texinfo
> > +++ b/gdb/doc/gdb.texinfo
> > @@ -50259,6 +50259,27 @@ 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 @kbd{Ctrl-C} behaviour
> > +Pressing @kbd{Ctrl-C} twice during downloading will cancel the current
> > +download.
> > +
> > +@item set debuginfod cancel all
> > +Pressing @kbd{Ctrl-C} twice during downloading will cancel this and all
> > +further downloads.
> > +
> > +@item set debuginfod cancel ask
> > +Pressing @kbd{Ctrl-C} twice during downloading will cancel the current
> > +download and prompt whether to cancel all further downloads.  By default,
> > +@code{debuginfod cancel} is set to @code{ask} for interactive sessions.
> > +
> > +@kindex show debuginfod cancel
> > +@item show debuginfod cancel
> > +Display the current setting of @code{debuginfod cancel}.
> > +
> >  @kindex set debuginfod urls
> >  @cindex configure debuginfod URLs
> >  @item set debuginfod urls
> > --
> > 2.43.0
> >

[PING*2][PATCH v4] gdb/debuginfod: Ctrl-C asks to cancel further downloads

[Aaron Merey]

Ping re. non-documentation parts of the patch

Thanks,
Aaron

On Thu, Feb 22, 2024 at 5:26 PM Aaron Merey <amerey@redhat.com> wrote:
>
> Ping
>
> Thanks,
> Aaron
>
> On Fri, Feb 2, 2024 at 9:15 PM Aaron Merey <amerey@redhat.com> wrote:
> >
> > v3: https://sourceware.org/pipermail/gdb-patches/2023-March/197679.html
> >
> > v4 includes changes needed to apply the patch to the current master branch.
> >
> > Currently gdb only allows for debuginfod downloads to be cancelled one at
> > a time via Ctrl-C.  This can be a burden if one wishes to cancel a large
> > series of downloads.  Additionally there can also be ambiguity between whether
> > Ctrl-C during a download was intended to cancel a download or interrupt the
> > inferior.
> >
> > This patch addresses these issues by adding a new debuginfod setting and
> > changing the behavior of Ctrl-C during a download.
> >
> > Add a new command "set debuginfod cancel one/all/ask", where:
> > - "one" means Ctrl-C cancels one download,
> > - "all" means Ctrl-C cancels all further downloads, and
> > - "ask" means Ctrl-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".
> >
> > A single Ctrl-C during downloading now sets the quit_flag and proceeds with
> > all downloads.  If the inferior has the terminal, then a second Ctrl-C during
> > downloading triggers a query asking whether to cancel the download or
> > interrupt the inferior.  If the user wishes to cancel the download then
> > the setting of 'set debuginfod cancel' determines whether one or all
> > downloads are cancelled.  In the case of "set debuginfod cancel ask",
> > there will be another query at this point asking whether to cancel one
> > or all downloads.
> >
> > If the inferior does not have the terminal, then a second Ctrl-C during
> > downloading simply cancels the download according to the setting of
> > "set debuginfod cancel".  In this case there is no query asking whether
> > to interrupt the inferior or cancel a download.
> >
> > Example session where inferior has terminal:
> >
> >     (gdb) run
> >     [...]
> >     Downloading separate debug info for /lib64/libxyz.so
> >     [###                                                            ]^C^C
> >     Cancel the current download?
> >     If no, then Ctrl-C will be sent to the target process. ([y] or n) y
> >     Cancelling download of separate debug info for /lib64/libxyz.so...
> >     Cancel further downloading for this session? (y or [n]) n
> >     Downloading separate debug info for /lib64/libabcd.so
> >
> > Example session where inferior does not have terminal:
> >
> >     (gdb) run
> >     [...]
> >     Downloading separate debug info for /lib64/libxyz.so
> >     [###                                                            ]^C^C
> >     Cancelling download of separate debug info for /lib64/libxyz.so...
> >     Cancel further downloading for this session? (y or [n]) y
> >     Debuginfod has been disabled.
> >     To re-enable use the 'set debuginfod enabled' command.
> >
> > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29582
> > Suggested-By: Martin Liška <mliska@suse.cz>
> > Co-Authored-By: Tom de Vries <tdevries@suse.de>
> > ---
> >  gdb/debuginfod-support.c | 183 +++++++++++++++++++++++++++++++++++++--
> >  gdb/doc/gdb.texinfo      |  21 +++++
> >  2 files changed, 199 insertions(+), 5 deletions(-)
> >
> > diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
> > index 7d8ada39e96..97d4b2f2bac 100644
> > --- a/gdb/debuginfod-support.c
> > +++ b/gdb/debuginfod-support.c
> > @@ -38,6 +38,8 @@ static cmd_list_element *maint_show_debuginfod_cmdlist;
> >  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[] =
> >  {
> > @@ -47,6 +49,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;
> > @@ -109,9 +125,11 @@ debuginfod_section_query (const unsigned char *build_id,
> >  struct user_data
> >  {
> >    user_data (const char *desc, const char *fname)
> > -    : desc (desc), fname (fname)
> > +    : pass_quit_flag (false), inf_had_term (false), desc (desc), fname (fname)
> >    { }
> >
> > +  bool pass_quit_flag;
> > +  bool inf_had_term;
> >    const char * const desc;
> >    const char * const fname;
> >    ui_out::progress_update progress;
> > @@ -156,10 +174,83 @@ progressfn (debuginfod_client *c, long cur, long total)
> >    if (check_quit_flag ())
> >      {
> >        ui_file *outstream = get_unbuffered (gdb_stdout);
> > -      gdb_printf (outstream, _("Cancelling download of %s %s...\n"),
> > -                 data->desc, styled_fname.c_str ());
> > -      return 1;
> > -    }
> > +
> > +      /* If a single Ctrl-C occurs during downloading, let it propagate to the
> > +        target.  If more than one Ctrl-C occurs, ask whether to cancel the
> > +        current download or interrupt the target.  If the download is
> > +        cancelled, the setting of debuginfod_cancel will determine whether
> > +        the current download is cancelled or debuginfod is disabled.  */
> > +      if (!data->pass_quit_flag)
> > +       data->pass_quit_flag = true;
> > +      else
> > +       {
> > +         int resp = 1;
> > +         bool extra_nl = false;
> > +
> > +         if (data->inf_had_term)
> > +           {
> > +             /* If Ctrl-C occurs during the following prompts, catch the
> > +                exception to prevent unsafe early returns to gdb's main
> > +                event loop.  During these prompts, Ctrl-C is equivalent to
> > +                answering 'y'.  */
> > +             try
> > +               {
> > +                 resp = yquery (_("Cancel the current download?\nIf no, "
> > +                                  "then Ctrl-C will be sent to the target "
> > +                                  "process. "));
> > +               }
> > +             catch (const gdb_exception &)
> > +               {
> > +                 /* If the query doesn't complete, then we need an additional
> > +                    newline to get "Cancelling download of..." printed on a
> > +                    separate line.  */
> > +                 extra_nl = true;
> > +               }
> > +           }
> > +         if (resp)
> > +           {
> > +             if (extra_nl)
> > +               {
> > +                 gdb_printf (outstream, "\n");
> > +                 extra_nl = false;
> > +               }
> > +
> > +             gdb_printf (outstream, _("Cancelling download of %s %s...\n"),
> > +                                      data->desc, styled_fname.c_str ());
> > +             if (debuginfod_cancel == debuginfod_ask)
> > +               {
> > +                 try
> > +                   {
> > +                     resp = nquery
> > +                       (_("Cancel further downloading for this session? "));
> > +                   }
> > +                 catch (const gdb_exception &)
> > +                   {
> > +                     resp = 1;
> > +                     extra_nl = true;
> > +                   }
> > +
> > +                 if (resp)
> > +                   debuginfod_cancel = debuginfod_all;
> > +                 else
> > +                   debuginfod_cancel = debuginfod_one;
> > +               }
> > +             if (debuginfod_cancel == debuginfod_all)
> > +               {
> > +                 if (extra_nl)
> > +                   gdb_printf (outstream, "\n");
> > +
> > +                 gdb_printf (outstream,
> > +                             _("Debuginfod has been disabled.\nTo re-enable "
> > +                               "use the 'set debuginfod enabled' command.\n"));
> > +                 debuginfod_enabled = debuginfod_off;
> > +               }
> > +
> > +           data->pass_quit_flag = false;
> > +           return 1;
> > +         }
> > +      }
> > +  }
> >
> >    if (debuginfod_verbose == 0)
> >      return 0;
> > @@ -330,6 +421,10 @@ debuginfod_source_query (const unsigned char *build_id,
> >      user_data data ("source file", srcpath);
> >
> >      debuginfod_set_user_data (c, &data);
> > +
> > +   if (!target_terminal::is_ours ())
> > +     data.inf_had_term = true;
> > +
> >      if (target_supports_terminal_ours ())
> >        {
> >         term_state.emplace ();
> > @@ -341,6 +436,11 @@ debuginfod_source_query (const unsigned char *build_id,
> >                                             build_id_len,
> >                                             srcpath,
> >                                             &dname));
> > +    if (data.pass_quit_flag)
> > +      set_quit_flag ();
> > +    if (data.inf_had_term && term_state.has_value ())
> > +      target_terminal::inferior ();
> > +
> >      debuginfod_set_user_data (c, nullptr);
> >    }
> >
> > @@ -376,6 +476,10 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
> >      user_data data ("separate debug info for", filename);
> >
> >      debuginfod_set_user_data (c, &data);
> > +
> > +    if (!target_terminal::is_ours ())
> > +      data.inf_had_term = true;
> > +
> >      if (target_supports_terminal_ours ())
> >        {
> >         term_state.emplace ();
> > @@ -384,6 +488,12 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
> >
> >      fd = scoped_fd (debuginfod_find_debuginfo (c, build_id, build_id_len,
> >                                                &dname));
> > +
> > +    if (data.pass_quit_flag)
> > +      set_quit_flag ();
> > +    if (data.inf_had_term && term_state.has_value ())
> > +      target_terminal::inferior ();
> > +
> >      debuginfod_set_user_data (c, nullptr);
> >    }
> >
> > @@ -419,6 +529,10 @@ debuginfod_exec_query (const unsigned char *build_id,
> >      user_data data ("executable for", filename);
> >
> >      debuginfod_set_user_data (c, &data);
> > +
> > +    if (!target_terminal::is_ours ())
> > +      data.inf_had_term = true;
> > +
> >      if (target_supports_terminal_ours ())
> >        {
> >         term_state.emplace ();
> > @@ -427,6 +541,11 @@ debuginfod_exec_query (const unsigned char *build_id,
> >
> >      fd = scoped_fd (debuginfod_find_executable (c, build_id, build_id_len,
> >                                                 &dname));
> > +    if (data.pass_quit_flag)
> > +      set_quit_flag ();
> > +    if (data.inf_had_term && term_state.has_value ())
> > +      target_terminal::inferior ();
> > +
> >      debuginfod_set_user_data (c, nullptr);
> >    }
> >
> > @@ -467,6 +586,10 @@ debuginfod_section_query (const unsigned char *build_id,
> >    {
> >      user_data data (desc.c_str (), filename);
> >      debuginfod_set_user_data (c, &data);
> > +
> > +    if (!target_terminal::is_ours ())
> > +      data.inf_had_term = true;
> > +
> >      if (target_supports_terminal_ours ())
> >        {
> >         term_state.emplace ();
> > @@ -475,6 +598,12 @@ debuginfod_section_query (const unsigned char *build_id,
> >
> >      fd = scoped_fd (debuginfod_find_section (c, build_id, build_id_len,
> >                                              section_name, &dname));
> > +
> > +    if (data.pass_quit_flag)
> > +      set_quit_flag ();
> > +    if (data.inf_had_term && term_state.has_value ())
> > +      target_terminal::inferior ();
> > +
> >      debuginfod_set_user_data (c, nullptr);
> >    }
> >
> > @@ -523,6 +652,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
> > @@ -627,6 +783,23 @@ When set to \"ask\", prompt whether to enable or disable debuginfod." ),
> >                         &set_debuginfod_prefix_list,
> >                         &show_debuginfod_prefix_list);
> >
> > +  add_setshow_enum_cmd ("cancel", class_run, debuginfod_cancel_enum,
> > +                       _("Set Ctrl-C behaviour for debuginfod."),
> > +                       _("Show Ctrl-C behaviour for debuginfod."),
> > +                       _("\
> > +When set to \'one\', pressing Ctrl-C twice cancels a single \
> > +download.\nWhen set to \'all\', pressing Ctrl-C twice cancels all further downloads.\n\
> > +When set to \'ask\', pressing Ctrl-C twice asks what to do.\nA single Ctrl-C during \
> > +downloading is passed to the target process being debugged.\nA second Ctrl-C \
> > +during downloading may raise a prompt asking whether to cancel the download or \
> > +send Ctrl-C to the target.\nIf the download is cancelled, then no Ctrl-C is \
> > +sent to the target."),
> > +                       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 e98c15242bc..d6df28896b1 100644
> > --- a/gdb/doc/gdb.texinfo
> > +++ b/gdb/doc/gdb.texinfo
> > @@ -50259,6 +50259,27 @@ 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 @kbd{Ctrl-C} behaviour
> > +Pressing @kbd{Ctrl-C} twice during downloading will cancel the current
> > +download.
> > +
> > +@item set debuginfod cancel all
> > +Pressing @kbd{Ctrl-C} twice during downloading will cancel this and all
> > +further downloads.
> > +
> > +@item set debuginfod cancel ask
> > +Pressing @kbd{Ctrl-C} twice during downloading will cancel the current
> > +download and prompt whether to cancel all further downloads.  By default,
> > +@code{debuginfod cancel} is set to @code{ask} for interactive sessions.
> > +
> > +@kindex show debuginfod cancel
> > +@item show debuginfod cancel
> > +Display the current setting of @code{debuginfod cancel}.
> > +
> >  @kindex set debuginfod urls
> >  @cindex configure debuginfod URLs
> >  @item set debuginfod urls
> > --
> > 2.43.0
> >

[PING*3][PATCH v4] gdb/debuginfod: Ctrl-C asks to cancel further downloads

[Aaron Merey]

Ping re. non-documentation parts of the patch

Thanks,
Aaron

On Fri, May 17, 2024 at 10:11 AM Aaron Merey <amerey@redhat.com> wrote:
>
> Ping re. non-documentation parts of the patch
>
> Thanks,
> Aaron
>
> On Thu, Feb 22, 2024 at 5:26 PM Aaron Merey <amerey@redhat.com> wrote:
> >
> > Ping
> >
> > Thanks,
> > Aaron
> >
> > On Fri, Feb 2, 2024 at 9:15 PM Aaron Merey <amerey@redhat.com> wrote:
> > >
> > > v3: https://sourceware.org/pipermail/gdb-patches/2023-March/197679.html
> > >
> > > v4 includes changes needed to apply the patch to the current master branch.
> > >
> > > Currently gdb only allows for debuginfod downloads to be cancelled one at
> > > a time via Ctrl-C.  This can be a burden if one wishes to cancel a large
> > > series of downloads.  Additionally there can also be ambiguity between whether
> > > Ctrl-C during a download was intended to cancel a download or interrupt the
> > > inferior.
> > >
> > > This patch addresses these issues by adding a new debuginfod setting and
> > > changing the behavior of Ctrl-C during a download.
> > >
> > > Add a new command "set debuginfod cancel one/all/ask", where:
> > > - "one" means Ctrl-C cancels one download,
> > > - "all" means Ctrl-C cancels all further downloads, and
> > > - "ask" means Ctrl-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".
> > >
> > > A single Ctrl-C during downloading now sets the quit_flag and proceeds with
> > > all downloads.  If the inferior has the terminal, then a second Ctrl-C during
> > > downloading triggers a query asking whether to cancel the download or
> > > interrupt the inferior.  If the user wishes to cancel the download then
> > > the setting of 'set debuginfod cancel' determines whether one or all
> > > downloads are cancelled.  In the case of "set debuginfod cancel ask",
> > > there will be another query at this point asking whether to cancel one
> > > or all downloads.
> > >
> > > If the inferior does not have the terminal, then a second Ctrl-C during
> > > downloading simply cancels the download according to the setting of
> > > "set debuginfod cancel".  In this case there is no query asking whether
> > > to interrupt the inferior or cancel a download.
> > >
> > > Example session where inferior has terminal:
> > >
> > >     (gdb) run
> > >     [...]
> > >     Downloading separate debug info for /lib64/libxyz.so
> > >     [###                                                            ]^C^C
> > >     Cancel the current download?
> > >     If no, then Ctrl-C will be sent to the target process. ([y] or n) y
> > >     Cancelling download of separate debug info for /lib64/libxyz.so...
> > >     Cancel further downloading for this session? (y or [n]) n
> > >     Downloading separate debug info for /lib64/libabcd.so
> > >
> > > Example session where inferior does not have terminal:
> > >
> > >     (gdb) run
> > >     [...]
> > >     Downloading separate debug info for /lib64/libxyz.so
> > >     [###                                                            ]^C^C
> > >     Cancelling download of separate debug info for /lib64/libxyz.so...
> > >     Cancel further downloading for this session? (y or [n]) y
> > >     Debuginfod has been disabled.
> > >     To re-enable use the 'set debuginfod enabled' command.
> > >
> > > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29582
> > > Suggested-By: Martin Liška <mliska@suse.cz>
> > > Co-Authored-By: Tom de Vries <tdevries@suse.de>
> > > ---
> > >  gdb/debuginfod-support.c | 183 +++++++++++++++++++++++++++++++++++++--
> > >  gdb/doc/gdb.texinfo      |  21 +++++
> > >  2 files changed, 199 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
> > > index 7d8ada39e96..97d4b2f2bac 100644
> > > --- a/gdb/debuginfod-support.c
> > > +++ b/gdb/debuginfod-support.c
> > > @@ -38,6 +38,8 @@ static cmd_list_element *maint_show_debuginfod_cmdlist;
> > >  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[] =
> > >  {
> > > @@ -47,6 +49,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;
> > > @@ -109,9 +125,11 @@ debuginfod_section_query (const unsigned char *build_id,
> > >  struct user_data
> > >  {
> > >    user_data (const char *desc, const char *fname)
> > > -    : desc (desc), fname (fname)
> > > +    : pass_quit_flag (false), inf_had_term (false), desc (desc), fname (fname)
> > >    { }
> > >
> > > +  bool pass_quit_flag;
> > > +  bool inf_had_term;
> > >    const char * const desc;
> > >    const char * const fname;
> > >    ui_out::progress_update progress;
> > > @@ -156,10 +174,83 @@ progressfn (debuginfod_client *c, long cur, long total)
> > >    if (check_quit_flag ())
> > >      {
> > >        ui_file *outstream = get_unbuffered (gdb_stdout);
> > > -      gdb_printf (outstream, _("Cancelling download of %s %s...\n"),
> > > -                 data->desc, styled_fname.c_str ());
> > > -      return 1;
> > > -    }
> > > +
> > > +      /* If a single Ctrl-C occurs during downloading, let it propagate to the
> > > +        target.  If more than one Ctrl-C occurs, ask whether to cancel the
> > > +        current download or interrupt the target.  If the download is
> > > +        cancelled, the setting of debuginfod_cancel will determine whether
> > > +        the current download is cancelled or debuginfod is disabled.  */
> > > +      if (!data->pass_quit_flag)
> > > +       data->pass_quit_flag = true;
> > > +      else
> > > +       {
> > > +         int resp = 1;
> > > +         bool extra_nl = false;
> > > +
> > > +         if (data->inf_had_term)
> > > +           {
> > > +             /* If Ctrl-C occurs during the following prompts, catch the
> > > +                exception to prevent unsafe early returns to gdb's main
> > > +                event loop.  During these prompts, Ctrl-C is equivalent to
> > > +                answering 'y'.  */
> > > +             try
> > > +               {
> > > +                 resp = yquery (_("Cancel the current download?\nIf no, "
> > > +                                  "then Ctrl-C will be sent to the target "
> > > +                                  "process. "));
> > > +               }
> > > +             catch (const gdb_exception &)
> > > +               {
> > > +                 /* If the query doesn't complete, then we need an additional
> > > +                    newline to get "Cancelling download of..." printed on a
> > > +                    separate line.  */
> > > +                 extra_nl = true;
> > > +               }
> > > +           }
> > > +         if (resp)
> > > +           {
> > > +             if (extra_nl)
> > > +               {
> > > +                 gdb_printf (outstream, "\n");
> > > +                 extra_nl = false;
> > > +               }
> > > +
> > > +             gdb_printf (outstream, _("Cancelling download of %s %s...\n"),
> > > +                                      data->desc, styled_fname.c_str ());
> > > +             if (debuginfod_cancel == debuginfod_ask)
> > > +               {
> > > +                 try
> > > +                   {
> > > +                     resp = nquery
> > > +                       (_("Cancel further downloading for this session? "));
> > > +                   }
> > > +                 catch (const gdb_exception &)
> > > +                   {
> > > +                     resp = 1;
> > > +                     extra_nl = true;
> > > +                   }
> > > +
> > > +                 if (resp)
> > > +                   debuginfod_cancel = debuginfod_all;
> > > +                 else
> > > +                   debuginfod_cancel = debuginfod_one;
> > > +               }
> > > +             if (debuginfod_cancel == debuginfod_all)
> > > +               {
> > > +                 if (extra_nl)
> > > +                   gdb_printf (outstream, "\n");
> > > +
> > > +                 gdb_printf (outstream,
> > > +                             _("Debuginfod has been disabled.\nTo re-enable "
> > > +                               "use the 'set debuginfod enabled' command.\n"));
> > > +                 debuginfod_enabled = debuginfod_off;
> > > +               }
> > > +
> > > +           data->pass_quit_flag = false;
> > > +           return 1;
> > > +         }
> > > +      }
> > > +  }
> > >
> > >    if (debuginfod_verbose == 0)
> > >      return 0;
> > > @@ -330,6 +421,10 @@ debuginfod_source_query (const unsigned char *build_id,
> > >      user_data data ("source file", srcpath);
> > >
> > >      debuginfod_set_user_data (c, &data);
> > > +
> > > +   if (!target_terminal::is_ours ())
> > > +     data.inf_had_term = true;
> > > +
> > >      if (target_supports_terminal_ours ())
> > >        {
> > >         term_state.emplace ();
> > > @@ -341,6 +436,11 @@ debuginfod_source_query (const unsigned char *build_id,
> > >                                             build_id_len,
> > >                                             srcpath,
> > >                                             &dname));
> > > +    if (data.pass_quit_flag)
> > > +      set_quit_flag ();
> > > +    if (data.inf_had_term && term_state.has_value ())
> > > +      target_terminal::inferior ();
> > > +
> > >      debuginfod_set_user_data (c, nullptr);
> > >    }
> > >
> > > @@ -376,6 +476,10 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
> > >      user_data data ("separate debug info for", filename);
> > >
> > >      debuginfod_set_user_data (c, &data);
> > > +
> > > +    if (!target_terminal::is_ours ())
> > > +      data.inf_had_term = true;
> > > +
> > >      if (target_supports_terminal_ours ())
> > >        {
> > >         term_state.emplace ();
> > > @@ -384,6 +488,12 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
> > >
> > >      fd = scoped_fd (debuginfod_find_debuginfo (c, build_id, build_id_len,
> > >                                                &dname));
> > > +
> > > +    if (data.pass_quit_flag)
> > > +      set_quit_flag ();
> > > +    if (data.inf_had_term && term_state.has_value ())
> > > +      target_terminal::inferior ();
> > > +
> > >      debuginfod_set_user_data (c, nullptr);
> > >    }
> > >
> > > @@ -419,6 +529,10 @@ debuginfod_exec_query (const unsigned char *build_id,
> > >      user_data data ("executable for", filename);
> > >
> > >      debuginfod_set_user_data (c, &data);
> > > +
> > > +    if (!target_terminal::is_ours ())
> > > +      data.inf_had_term = true;
> > > +
> > >      if (target_supports_terminal_ours ())
> > >        {
> > >         term_state.emplace ();
> > > @@ -427,6 +541,11 @@ debuginfod_exec_query (const unsigned char *build_id,
> > >
> > >      fd = scoped_fd (debuginfod_find_executable (c, build_id, build_id_len,
> > >                                                 &dname));
> > > +    if (data.pass_quit_flag)
> > > +      set_quit_flag ();
> > > +    if (data.inf_had_term && term_state.has_value ())
> > > +      target_terminal::inferior ();
> > > +
> > >      debuginfod_set_user_data (c, nullptr);
> > >    }
> > >
> > > @@ -467,6 +586,10 @@ debuginfod_section_query (const unsigned char *build_id,
> > >    {
> > >      user_data data (desc.c_str (), filename);
> > >      debuginfod_set_user_data (c, &data);
> > > +
> > > +    if (!target_terminal::is_ours ())
> > > +      data.inf_had_term = true;
> > > +
> > >      if (target_supports_terminal_ours ())
> > >        {
> > >         term_state.emplace ();
> > > @@ -475,6 +598,12 @@ debuginfod_section_query (const unsigned char *build_id,
> > >
> > >      fd = scoped_fd (debuginfod_find_section (c, build_id, build_id_len,
> > >                                              section_name, &dname));
> > > +
> > > +    if (data.pass_quit_flag)
> > > +      set_quit_flag ();
> > > +    if (data.inf_had_term && term_state.has_value ())
> > > +      target_terminal::inferior ();
> > > +
> > >      debuginfod_set_user_data (c, nullptr);
> > >    }
> > >
> > > @@ -523,6 +652,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
> > > @@ -627,6 +783,23 @@ When set to \"ask\", prompt whether to enable or disable debuginfod." ),
> > >                         &set_debuginfod_prefix_list,
> > >                         &show_debuginfod_prefix_list);
> > >
> > > +  add_setshow_enum_cmd ("cancel", class_run, debuginfod_cancel_enum,
> > > +                       _("Set Ctrl-C behaviour for debuginfod."),
> > > +                       _("Show Ctrl-C behaviour for debuginfod."),
> > > +                       _("\
> > > +When set to \'one\', pressing Ctrl-C twice cancels a single \
> > > +download.\nWhen set to \'all\', pressing Ctrl-C twice cancels all further downloads.\n\
> > > +When set to \'ask\', pressing Ctrl-C twice asks what to do.\nA single Ctrl-C during \
> > > +downloading is passed to the target process being debugged.\nA second Ctrl-C \
> > > +during downloading may raise a prompt asking whether to cancel the download or \
> > > +send Ctrl-C to the target.\nIf the download is cancelled, then no Ctrl-C is \
> > > +sent to the target."),
> > > +                       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 e98c15242bc..d6df28896b1 100644
> > > --- a/gdb/doc/gdb.texinfo
> > > +++ b/gdb/doc/gdb.texinfo
> > > @@ -50259,6 +50259,27 @@ 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 @kbd{Ctrl-C} behaviour
> > > +Pressing @kbd{Ctrl-C} twice during downloading will cancel the current
> > > +download.
> > > +
> > > +@item set debuginfod cancel all
> > > +Pressing @kbd{Ctrl-C} twice during downloading will cancel this and all
> > > +further downloads.
> > > +
> > > +@item set debuginfod cancel ask
> > > +Pressing @kbd{Ctrl-C} twice during downloading will cancel the current
> > > +download and prompt whether to cancel all further downloads.  By default,
> > > +@code{debuginfod cancel} is set to @code{ask} for interactive sessions.
> > > +
> > > +@kindex show debuginfod cancel
> > > +@item show debuginfod cancel
> > > +Display the current setting of @code{debuginfod cancel}.
> > > +
> > >  @kindex set debuginfod urls
> > >  @cindex configure debuginfod URLs
> > >  @item set debuginfod urls
> > > --
> > > 2.43.0
> > >

[PING*4][PATCH v4] gdb/debuginfod: Ctrl-C asks to cancel further downloads

[Aaron Merey]

Ping re. non-documentation parts of the patch

Thanks,
Aaron

On Fri, May 31, 2024 at 4:57 PM Aaron Merey <amerey@redhat.com> wrote:
>
> Ping re. non-documentation parts of the patch
>
> Thanks,
> Aaron
>
> On Fri, May 17, 2024 at 10:11 AM Aaron Merey <amerey@redhat.com> wrote:
> >
> > Ping re. non-documentation parts of the patch
> >
> > Thanks,
> > Aaron
> >
> > On Thu, Feb 22, 2024 at 5:26 PM Aaron Merey <amerey@redhat.com> wrote:
> > >
> > > Ping
> > >
> > > Thanks,
> > > Aaron
> > >
> > > On Fri, Feb 2, 2024 at 9:15 PM Aaron Merey <amerey@redhat.com> wrote:
> > > >
> > > > v3: https://sourceware.org/pipermail/gdb-patches/2023-March/197679.html
> > > >
> > > > v4 includes changes needed to apply the patch to the current master branch.
> > > >
> > > > Currently gdb only allows for debuginfod downloads to be cancelled one at
> > > > a time via Ctrl-C.  This can be a burden if one wishes to cancel a large
> > > > series of downloads.  Additionally there can also be ambiguity between whether
> > > > Ctrl-C during a download was intended to cancel a download or interrupt the
> > > > inferior.
> > > >
> > > > This patch addresses these issues by adding a new debuginfod setting and
> > > > changing the behavior of Ctrl-C during a download.
> > > >
> > > > Add a new command "set debuginfod cancel one/all/ask", where:
> > > > - "one" means Ctrl-C cancels one download,
> > > > - "all" means Ctrl-C cancels all further downloads, and
> > > > - "ask" means Ctrl-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".
> > > >
> > > > A single Ctrl-C during downloading now sets the quit_flag and proceeds with
> > > > all downloads.  If the inferior has the terminal, then a second Ctrl-C during
> > > > downloading triggers a query asking whether to cancel the download or
> > > > interrupt the inferior.  If the user wishes to cancel the download then
> > > > the setting of 'set debuginfod cancel' determines whether one or all
> > > > downloads are cancelled.  In the case of "set debuginfod cancel ask",
> > > > there will be another query at this point asking whether to cancel one
> > > > or all downloads.
> > > >
> > > > If the inferior does not have the terminal, then a second Ctrl-C during
> > > > downloading simply cancels the download according to the setting of
> > > > "set debuginfod cancel".  In this case there is no query asking whether
> > > > to interrupt the inferior or cancel a download.
> > > >
> > > > Example session where inferior has terminal:
> > > >
> > > >     (gdb) run
> > > >     [...]
> > > >     Downloading separate debug info for /lib64/libxyz.so
> > > >     [###                                                            ]^C^C
> > > >     Cancel the current download?
> > > >     If no, then Ctrl-C will be sent to the target process. ([y] or n) y
> > > >     Cancelling download of separate debug info for /lib64/libxyz.so...
> > > >     Cancel further downloading for this session? (y or [n]) n
> > > >     Downloading separate debug info for /lib64/libabcd.so
> > > >
> > > > Example session where inferior does not have terminal:
> > > >
> > > >     (gdb) run
> > > >     [...]
> > > >     Downloading separate debug info for /lib64/libxyz.so
> > > >     [###                                                            ]^C^C
> > > >     Cancelling download of separate debug info for /lib64/libxyz.so...
> > > >     Cancel further downloading for this session? (y or [n]) y
> > > >     Debuginfod has been disabled.
> > > >     To re-enable use the 'set debuginfod enabled' command.
> > > >
> > > > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29582
> > > > Suggested-By: Martin Liška <mliska@suse.cz>
> > > > Co-Authored-By: Tom de Vries <tdevries@suse.de>
> > > > ---
> > > >  gdb/debuginfod-support.c | 183 +++++++++++++++++++++++++++++++++++++--
> > > >  gdb/doc/gdb.texinfo      |  21 +++++
> > > >  2 files changed, 199 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
> > > > index 7d8ada39e96..97d4b2f2bac 100644
> > > > --- a/gdb/debuginfod-support.c
> > > > +++ b/gdb/debuginfod-support.c
> > > > @@ -38,6 +38,8 @@ static cmd_list_element *maint_show_debuginfod_cmdlist;
> > > >  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[] =
> > > >  {
> > > > @@ -47,6 +49,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;
> > > > @@ -109,9 +125,11 @@ debuginfod_section_query (const unsigned char *build_id,
> > > >  struct user_data
> > > >  {
> > > >    user_data (const char *desc, const char *fname)
> > > > -    : desc (desc), fname (fname)
> > > > +    : pass_quit_flag (false), inf_had_term (false), desc (desc), fname (fname)
> > > >    { }
> > > >
> > > > +  bool pass_quit_flag;
> > > > +  bool inf_had_term;
> > > >    const char * const desc;
> > > >    const char * const fname;
> > > >    ui_out::progress_update progress;
> > > > @@ -156,10 +174,83 @@ progressfn (debuginfod_client *c, long cur, long total)
> > > >    if (check_quit_flag ())
> > > >      {
> > > >        ui_file *outstream = get_unbuffered (gdb_stdout);
> > > > -      gdb_printf (outstream, _("Cancelling download of %s %s...\n"),
> > > > -                 data->desc, styled_fname.c_str ());
> > > > -      return 1;
> > > > -    }
> > > > +
> > > > +      /* If a single Ctrl-C occurs during downloading, let it propagate to the
> > > > +        target.  If more than one Ctrl-C occurs, ask whether to cancel the
> > > > +        current download or interrupt the target.  If the download is
> > > > +        cancelled, the setting of debuginfod_cancel will determine whether
> > > > +        the current download is cancelled or debuginfod is disabled.  */
> > > > +      if (!data->pass_quit_flag)
> > > > +       data->pass_quit_flag = true;
> > > > +      else
> > > > +       {
> > > > +         int resp = 1;
> > > > +         bool extra_nl = false;
> > > > +
> > > > +         if (data->inf_had_term)
> > > > +           {
> > > > +             /* If Ctrl-C occurs during the following prompts, catch the
> > > > +                exception to prevent unsafe early returns to gdb's main
> > > > +                event loop.  During these prompts, Ctrl-C is equivalent to
> > > > +                answering 'y'.  */
> > > > +             try
> > > > +               {
> > > > +                 resp = yquery (_("Cancel the current download?\nIf no, "
> > > > +                                  "then Ctrl-C will be sent to the target "
> > > > +                                  "process. "));
> > > > +               }
> > > > +             catch (const gdb_exception &)
> > > > +               {
> > > > +                 /* If the query doesn't complete, then we need an additional
> > > > +                    newline to get "Cancelling download of..." printed on a
> > > > +                    separate line.  */
> > > > +                 extra_nl = true;
> > > > +               }
> > > > +           }
> > > > +         if (resp)
> > > > +           {
> > > > +             if (extra_nl)
> > > > +               {
> > > > +                 gdb_printf (outstream, "\n");
> > > > +                 extra_nl = false;
> > > > +               }
> > > > +
> > > > +             gdb_printf (outstream, _("Cancelling download of %s %s...\n"),
> > > > +                                      data->desc, styled_fname.c_str ());
> > > > +             if (debuginfod_cancel == debuginfod_ask)
> > > > +               {
> > > > +                 try
> > > > +                   {
> > > > +                     resp = nquery
> > > > +                       (_("Cancel further downloading for this session? "));
> > > > +                   }
> > > > +                 catch (const gdb_exception &)
> > > > +                   {
> > > > +                     resp = 1;
> > > > +                     extra_nl = true;
> > > > +                   }
> > > > +
> > > > +                 if (resp)
> > > > +                   debuginfod_cancel = debuginfod_all;
> > > > +                 else
> > > > +                   debuginfod_cancel = debuginfod_one;
> > > > +               }
> > > > +             if (debuginfod_cancel == debuginfod_all)
> > > > +               {
> > > > +                 if (extra_nl)
> > > > +                   gdb_printf (outstream, "\n");
> > > > +
> > > > +                 gdb_printf (outstream,
> > > > +                             _("Debuginfod has been disabled.\nTo re-enable "
> > > > +                               "use the 'set debuginfod enabled' command.\n"));
> > > > +                 debuginfod_enabled = debuginfod_off;
> > > > +               }
> > > > +
> > > > +           data->pass_quit_flag = false;
> > > > +           return 1;
> > > > +         }
> > > > +      }
> > > > +  }
> > > >
> > > >    if (debuginfod_verbose == 0)
> > > >      return 0;
> > > > @@ -330,6 +421,10 @@ debuginfod_source_query (const unsigned char *build_id,
> > > >      user_data data ("source file", srcpath);
> > > >
> > > >      debuginfod_set_user_data (c, &data);
> > > > +
> > > > +   if (!target_terminal::is_ours ())
> > > > +     data.inf_had_term = true;
> > > > +
> > > >      if (target_supports_terminal_ours ())
> > > >        {
> > > >         term_state.emplace ();
> > > > @@ -341,6 +436,11 @@ debuginfod_source_query (const unsigned char *build_id,
> > > >                                             build_id_len,
> > > >                                             srcpath,
> > > >                                             &dname));
> > > > +    if (data.pass_quit_flag)
> > > > +      set_quit_flag ();
> > > > +    if (data.inf_had_term && term_state.has_value ())
> > > > +      target_terminal::inferior ();
> > > > +
> > > >      debuginfod_set_user_data (c, nullptr);
> > > >    }
> > > >
> > > > @@ -376,6 +476,10 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
> > > >      user_data data ("separate debug info for", filename);
> > > >
> > > >      debuginfod_set_user_data (c, &data);
> > > > +
> > > > +    if (!target_terminal::is_ours ())
> > > > +      data.inf_had_term = true;
> > > > +
> > > >      if (target_supports_terminal_ours ())
> > > >        {
> > > >         term_state.emplace ();
> > > > @@ -384,6 +488,12 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
> > > >
> > > >      fd = scoped_fd (debuginfod_find_debuginfo (c, build_id, build_id_len,
> > > >                                                &dname));
> > > > +
> > > > +    if (data.pass_quit_flag)
> > > > +      set_quit_flag ();
> > > > +    if (data.inf_had_term && term_state.has_value ())
> > > > +      target_terminal::inferior ();
> > > > +
> > > >      debuginfod_set_user_data (c, nullptr);
> > > >    }
> > > >
> > > > @@ -419,6 +529,10 @@ debuginfod_exec_query (const unsigned char *build_id,
> > > >      user_data data ("executable for", filename);
> > > >
> > > >      debuginfod_set_user_data (c, &data);
> > > > +
> > > > +    if (!target_terminal::is_ours ())
> > > > +      data.inf_had_term = true;
> > > > +
> > > >      if (target_supports_terminal_ours ())
> > > >        {
> > > >         term_state.emplace ();
> > > > @@ -427,6 +541,11 @@ debuginfod_exec_query (const unsigned char *build_id,
> > > >
> > > >      fd = scoped_fd (debuginfod_find_executable (c, build_id, build_id_len,
> > > >                                                 &dname));
> > > > +    if (data.pass_quit_flag)
> > > > +      set_quit_flag ();
> > > > +    if (data.inf_had_term && term_state.has_value ())
> > > > +      target_terminal::inferior ();
> > > > +
> > > >      debuginfod_set_user_data (c, nullptr);
> > > >    }
> > > >
> > > > @@ -467,6 +586,10 @@ debuginfod_section_query (const unsigned char *build_id,
> > > >    {
> > > >      user_data data (desc.c_str (), filename);
> > > >      debuginfod_set_user_data (c, &data);
> > > > +
> > > > +    if (!target_terminal::is_ours ())
> > > > +      data.inf_had_term = true;
> > > > +
> > > >      if (target_supports_terminal_ours ())
> > > >        {
> > > >         term_state.emplace ();
> > > > @@ -475,6 +598,12 @@ debuginfod_section_query (const unsigned char *build_id,
> > > >
> > > >      fd = scoped_fd (debuginfod_find_section (c, build_id, build_id_len,
> > > >                                              section_name, &dname));
> > > > +
> > > > +    if (data.pass_quit_flag)
> > > > +      set_quit_flag ();
> > > > +    if (data.inf_had_term && term_state.has_value ())
> > > > +      target_terminal::inferior ();
> > > > +
> > > >      debuginfod_set_user_data (c, nullptr);
> > > >    }
> > > >
> > > > @@ -523,6 +652,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
> > > > @@ -627,6 +783,23 @@ When set to \"ask\", prompt whether to enable or disable debuginfod." ),
> > > >                         &set_debuginfod_prefix_list,
> > > >                         &show_debuginfod_prefix_list);
> > > >
> > > > +  add_setshow_enum_cmd ("cancel", class_run, debuginfod_cancel_enum,
> > > > +                       _("Set Ctrl-C behaviour for debuginfod."),
> > > > +                       _("Show Ctrl-C behaviour for debuginfod."),
> > > > +                       _("\
> > > > +When set to \'one\', pressing Ctrl-C twice cancels a single \
> > > > +download.\nWhen set to \'all\', pressing Ctrl-C twice cancels all further downloads.\n\
> > > > +When set to \'ask\', pressing Ctrl-C twice asks what to do.\nA single Ctrl-C during \
> > > > +downloading is passed to the target process being debugged.\nA second Ctrl-C \
> > > > +during downloading may raise a prompt asking whether to cancel the download or \
> > > > +send Ctrl-C to the target.\nIf the download is cancelled, then no Ctrl-C is \
> > > > +sent to the target."),
> > > > +                       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 e98c15242bc..d6df28896b1 100644
> > > > --- a/gdb/doc/gdb.texinfo
> > > > +++ b/gdb/doc/gdb.texinfo
> > > > @@ -50259,6 +50259,27 @@ 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 @kbd{Ctrl-C} behaviour
> > > > +Pressing @kbd{Ctrl-C} twice during downloading will cancel the current
> > > > +download.
> > > > +
> > > > +@item set debuginfod cancel all
> > > > +Pressing @kbd{Ctrl-C} twice during downloading will cancel this and all
> > > > +further downloads.
> > > > +
> > > > +@item set debuginfod cancel ask
> > > > +Pressing @kbd{Ctrl-C} twice during downloading will cancel the current
> > > > +download and prompt whether to cancel all further downloads.  By default,
> > > > +@code{debuginfod cancel} is set to @code{ask} for interactive sessions.
> > > > +
> > > > +@kindex show debuginfod cancel
> > > > +@item show debuginfod cancel
> > > > +Display the current setting of @code{debuginfod cancel}.
> > > > +
> > > >  @kindex set debuginfod urls
> > > >  @cindex configure debuginfod URLs
> > > >  @item set debuginfod urls
> > > > --
> > > > 2.43.0
> > > >

Re: [PING*4][PATCH v4] gdb/debuginfod: Ctrl-C asks to cancel further downloads

[Keith Seitz]

Hi, Aaron,

Thank you so much for your continued patience.

I *really* like this feature, and I may remove "set debuginfod enabled
off" from my .gdbinit if/when this lands...

In an effort to nudge this along, I've played with this a bit, and
perused the code. Everything looks okay.

I just have one VERY minor request -- which you are free to ignore.

Consider the case where the user interrupts at the prompt. In the
below example, I use emacs. It looks something like this (my
comments appear inline, HTML-like):

$ gdb g -nx -iex "set height 0" /usr/bin/emacs
Reading symbols from /usr/bin/emacs...

This GDB supports auto-downloading debuginfo from the following URLs:
   <https://debuginfod.fedoraproject.org/>
Enable debuginfod for this session? (y or [n]) y
Debuginfod has been enabled.
To make this setting permanent, add 'set debuginfod enabled on' to .gdbinit.

<!-- interrupt this download -->
Downloading separate debug info for /usr/bin/emacs-29.3-gtk+x11
[####                                                         ]   8% 
(19.03 M)^CCancelling download of separate debug info for 
/usr/bin/emacs-29.3-gtk+x11...
Cancel further downloading for this session? (y or [n])
   <!-- ^C this query -->
Debuginfod has been disabled.
To re-enable use the 'set debuginfod enabled' command.
Reading symbols from .gnu_debugdata for /usr/bin/emacs-29.3-gtk+x11... 

(No debugging symbols found in .gnu_debugdata for 
/usr/bin/emacs-29.3-gtk+x11)

I have to admit, I find the "Debuginfod has been disabled." message a
bit user unfriendly:

 >>>>> +             if (debuginfod_cancel == debuginfod_all)
 >>>>> +               {
 >>>>> +                 if (extra_nl)
 >>>>> +                   gdb_printf (outstream, "\n");
 >>>>> +
 >>>>> +                 gdb_printf (outstream,
 >>>>> +                             _("Debuginfod has been 
disabled.\nTo re-enable "
 >>>>> +                               "use the 'set debuginfod enabled' 
command.\n"));
 >>>>> +                 debuginfod_enabled = debuginfod_off;
 >>>>> +               }

Might I cajole you to instead consider something like
"Interrupted -- debuginfod will be disabled." (and then keep
the rest of the messaging)

This way, the user isn't quite so surprised by the message. Gdb could
just have as easily just ignored sigint at the prompt. [Although
that paradigm is less gdb-y to do than other programs.]

Just a thought.

Reviewed-by: Keith Seitz <keiths@redhat.com>

Keith

Re: [PING*4][PATCH v4] gdb/debuginfod: Ctrl-C asks to cancel further downloads

[Aaron Merey]

Hi Keith,

Thanks for the review.

On Fri, Jun 7, 2024 at 7:27 PM Keith Seitz <keiths@redhat.com> wrote:
>
> Hi, Aaron,
>
> Thank you so much for your continued patience.
>
> I *really* like this feature, and I may remove "set debuginfod enabled
> off" from my .gdbinit if/when this lands...
>
> In an effort to nudge this along, I've played with this a bit, and
> perused the code. Everything looks okay.
>
> I just have one VERY minor request -- which you are free to ignore.
>
> Consider the case where the user interrupts at the prompt. In the
> below example, I use emacs. It looks something like this (my
> comments appear inline, HTML-like):
>
> $ gdb g -nx -iex "set height 0" /usr/bin/emacs
> Reading symbols from /usr/bin/emacs...
>
> This GDB supports auto-downloading debuginfo from the following URLs:
>    <https://debuginfod.fedoraproject.org/>
> Enable debuginfod for this session? (y or [n]) y
> Debuginfod has been enabled.
> To make this setting permanent, add 'set debuginfod enabled on' to .gdbinit.
>
> <!-- interrupt this download -->
> Downloading separate debug info for /usr/bin/emacs-29.3-gtk+x11
> [####                                                         ]   8%
> (19.03 M)^CCancelling download of separate debug info for
> /usr/bin/emacs-29.3-gtk+x11...
> Cancel further downloading for this session? (y or [n])
>    <!-- ^C this query -->
> Debuginfod has been disabled.
> To re-enable use the 'set debuginfod enabled' command.
> Reading symbols from .gnu_debugdata for /usr/bin/emacs-29.3-gtk+x11...
>
> (No debugging symbols found in .gnu_debugdata for
> /usr/bin/emacs-29.3-gtk+x11)
>
> I have to admit, I find the "Debuginfod has been disabled." message a
> bit user unfriendly:
>
>  >>>>> +             if (debuginfod_cancel == debuginfod_all)
>  >>>>> +               {
>  >>>>> +                 if (extra_nl)
>  >>>>> +                   gdb_printf (outstream, "\n");
>  >>>>> +
>  >>>>> +                 gdb_printf (outstream,
>  >>>>> +                             _("Debuginfod has been
> disabled.\nTo re-enable "
>  >>>>> +                               "use the 'set debuginfod enabled'
> command.\n"));
>  >>>>> +                 debuginfod_enabled = debuginfod_off;
>  >>>>> +               }
>
> Might I cajole you to instead consider something like
> "Interrupted -- debuginfod will be disabled." (and then keep
> the rest of the messaging)
>
> This way, the user isn't quite so surprised by the message. Gdb could
> just have as easily just ignored sigint at the prompt. [Although
> that paradigm is less gdb-y to do than other programs.]

I like this suggestion and I've included it in the patch.

>
> Just a thought.
>
> Reviewed-by: Keith Seitz <keiths@redhat.com>

Aaron

Re: [PATCH v4] gdb/debuginfod: Ctrl-C asks to cancel further downloads

[Andrew Burgess]

Aaron Merey <amerey@redhat.com> writes:

> v3: https://sourceware.org/pipermail/gdb-patches/2023-March/197679.html
>
> v4 includes changes needed to apply the patch to the current master branch.
>
> Currently gdb only allows for debuginfod downloads to be cancelled one at
> a time via Ctrl-C.  This can be a burden if one wishes to cancel a large
> series of downloads.  Additionally there can also be ambiguity between whether
> Ctrl-C during a download was intended to cancel a download or interrupt the
> inferior.
>
> This patch addresses these issues by adding a new debuginfod setting and
> changing the behavior of Ctrl-C during a download.
>
> Add a new command "set debuginfod cancel one/all/ask", where:
> - "one" means Ctrl-C cancels one download,
> - "all" means Ctrl-C cancels all further downloads, and
> - "ask" means Ctrl-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".
>
> A single Ctrl-C during downloading now sets the quit_flag and proceeds with
> all downloads.  If the inferior has the terminal, then a second Ctrl-C during
> downloading triggers a query asking whether to cancel the download or
> interrupt the inferior.  If the user wishes to cancel the download then
> the setting of 'set debuginfod cancel' determines whether one or all
> downloads are cancelled.  In the case of "set debuginfod cancel ask",
> there will be another query at this point asking whether to cancel one
> or all downloads.
>
> If the inferior does not have the terminal, then a second Ctrl-C during
> downloading simply cancels the download according to the setting of
> "set debuginfod cancel".  In this case there is no query asking whether
> to interrupt the inferior or cancel a download.
>
> Example session where inferior has terminal:
>
>     (gdb) run
>     [...]
>     Downloading separate debug info for /lib64/libxyz.so
>     [###                                                            ]^C^C
>     Cancel the current download?
>     If no, then Ctrl-C will be sent to the target process. ([y] or n) y
>     Cancelling download of separate debug info for /lib64/libxyz.so...
>     Cancel further downloading for this session? (y or [n]) n
>     Downloading separate debug info for /lib64/libabcd.so
>
> Example session where inferior does not have terminal:
>
>     (gdb) run
>     [...]
>     Downloading separate debug info for /lib64/libxyz.so
>     [###                                                            ]^C^C
>     Cancelling download of separate debug info for /lib64/libxyz.so...
>     Cancel further downloading for this session? (y or [n]) y
>     Debuginfod has been disabled.
>     To re-enable use the 'set debuginfod enabled' command.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29582
> Suggested-By: Martin Liška <mliska@suse.cz>
> Co-Authored-By: Tom de Vries <tdevries@suse.de>
> ---
>  gdb/debuginfod-support.c | 183 +++++++++++++++++++++++++++++++++++++--
>  gdb/doc/gdb.texinfo      |  21 +++++
>  2 files changed, 199 insertions(+), 5 deletions(-)
>
> diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
> index 7d8ada39e96..97d4b2f2bac 100644
> --- a/gdb/debuginfod-support.c
> +++ b/gdb/debuginfod-support.c
> @@ -38,6 +38,8 @@ static cmd_list_element *maint_show_debuginfod_cmdlist;
>  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[] =
>  {
> @@ -47,6 +49,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;
> @@ -109,9 +125,11 @@ debuginfod_section_query (const unsigned char *build_id,
>  struct user_data
>  {
>    user_data (const char *desc, const char *fname)
> -    : desc (desc), fname (fname)
> +    : pass_quit_flag (false), inf_had_term (false), desc (desc), fname (fname)
>    { }
>  
> +  bool pass_quit_flag;
> +  bool inf_had_term;
>    const char * const desc;
>    const char * const fname;
>    ui_out::progress_update progress;
> @@ -156,10 +174,83 @@ progressfn (debuginfod_client *c, long cur, long total)
>    if (check_quit_flag ())
>      {
>        ui_file *outstream = get_unbuffered (gdb_stdout);
> -      gdb_printf (outstream, _("Cancelling download of %s %s...\n"),
> -		  data->desc, styled_fname.c_str ());
> -      return 1;
> -    }
> +
> +      /* If a single Ctrl-C occurs during downloading, let it propagate to the
> +	 target.  If more than one Ctrl-C occurs, ask whether to cancel the
> +	 current download or interrupt the target.  If the download is
> +	 cancelled, the setting of debuginfod_cancel will determine whether
> +	 the current download is cancelled or debuginfod is disabled.  */
> +      if (!data->pass_quit_flag)
> +	data->pass_quit_flag = true;
> +      else
> +	{
> +	  int resp = 1;
> +	  bool extra_nl = false;
> +
> +	  if (data->inf_had_term)
> +	    {
> +	      /* If Ctrl-C occurs during the following prompts, catch the
> +		 exception to prevent unsafe early returns to gdb's main
> +		 event loop.  During these prompts, Ctrl-C is equivalent to
> +		 answering 'y'.  */
> +	      try
> +		{
> +		  resp = yquery (_("Cancel the current download?\nIf no, "
> +				   "then Ctrl-C will be sent to the target "
> +				   "process. "));
> +		}
> +	      catch (const gdb_exception &)
> +		{
> +		  /* If the query doesn't complete, then we need an additional
> +		     newline to get "Cancelling download of..." printed on a
> +		     separate line.  */
> +		  extra_nl = true;
> +		}
> +	    }
> +	  if (resp)
> +	    {
> +	      if (extra_nl)
> +		{
> +		  gdb_printf (outstream, "\n");
> +		  extra_nl = false;
> +		}
> +
> +	      gdb_printf (outstream, _("Cancelling download of %s %s...\n"),
> +				       data->desc, styled_fname.c_str ());
> +	      if (debuginfod_cancel == debuginfod_ask)
> +		{
> +		  try
> +		    {
> +		      resp = nquery
> +			(_("Cancel further downloading for this session? "));
> +		    }
> +		  catch (const gdb_exception &)
> +		    {
> +		      resp = 1;
> +		      extra_nl = true;
> +		    }
> +
> +		  if (resp)
> +		    debuginfod_cancel = debuginfod_all;
> +		  else
> +		    debuginfod_cancel = debuginfod_one;
> +		}
> +	      if (debuginfod_cancel == debuginfod_all)
> +		{
> +		  if (extra_nl)
> +		    gdb_printf (outstream, "\n");
> +
> +		  gdb_printf (outstream,
> +			      _("Debuginfod has been disabled.\nTo re-enable "
> +				"use the 'set debuginfod enabled' command.\n"));
> +		  debuginfod_enabled = debuginfod_off;
> +		}
> +
> +	    data->pass_quit_flag = false;
> +	    return 1;
> +	  }
> +      }
> +  }
>  
>    if (debuginfod_verbose == 0)
>      return 0;
> @@ -330,6 +421,10 @@ debuginfod_source_query (const unsigned char *build_id,
>      user_data data ("source file", srcpath);
>  
>      debuginfod_set_user_data (c, &data);
> +
> +   if (!target_terminal::is_ours ())
> +     data.inf_had_term = true;

Every time you create a user_data object you immediately follow up by
setting the ::inf_had_term field.  I think it would be nicer to just add
an extra argument to the constructor and pass target_terminal::is_ours()
through.

... Or maybe user_data should just call target_terminal::is_ours()
directly and you'd just add a comment to the constructor saying that it
records the current ownership of the terminal for the current inferior?

Not sure which would be better.

Thanks,
Andrew


> +
>      if (target_supports_terminal_ours ())
>        {
>  	term_state.emplace ();
> @@ -341,6 +436,11 @@ debuginfod_source_query (const unsigned char *build_id,
>  					    build_id_len,
>  					    srcpath,
>  					    &dname));
> +    if (data.pass_quit_flag)
> +      set_quit_flag ();
> +    if (data.inf_had_term && term_state.has_value ())
> +      target_terminal::inferior ();
> +
>      debuginfod_set_user_data (c, nullptr);
>    }
>  
> @@ -376,6 +476,10 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
>      user_data data ("separate debug info for", filename);
>  
>      debuginfod_set_user_data (c, &data);
> +
> +    if (!target_terminal::is_ours ())
> +      data.inf_had_term = true;
> +
>      if (target_supports_terminal_ours ())
>        {
>  	term_state.emplace ();
> @@ -384,6 +488,12 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
>  
>      fd = scoped_fd (debuginfod_find_debuginfo (c, build_id, build_id_len,
>  					       &dname));
> +
> +    if (data.pass_quit_flag)
> +      set_quit_flag ();
> +    if (data.inf_had_term && term_state.has_value ())
> +      target_terminal::inferior ();
> +
>      debuginfod_set_user_data (c, nullptr);
>    }
>  
> @@ -419,6 +529,10 @@ debuginfod_exec_query (const unsigned char *build_id,
>      user_data data ("executable for", filename);
>  
>      debuginfod_set_user_data (c, &data);
> +
> +    if (!target_terminal::is_ours ())
> +      data.inf_had_term = true;
> +
>      if (target_supports_terminal_ours ())
>        {
>  	term_state.emplace ();
> @@ -427,6 +541,11 @@ debuginfod_exec_query (const unsigned char *build_id,
>  
>      fd = scoped_fd (debuginfod_find_executable (c, build_id, build_id_len,
>  						&dname));
> +    if (data.pass_quit_flag)
> +      set_quit_flag ();
> +    if (data.inf_had_term && term_state.has_value ())
> +      target_terminal::inferior ();
> +
>      debuginfod_set_user_data (c, nullptr);
>    }
>  
> @@ -467,6 +586,10 @@ debuginfod_section_query (const unsigned char *build_id,
>    {
>      user_data data (desc.c_str (), filename);
>      debuginfod_set_user_data (c, &data);
> +
> +    if (!target_terminal::is_ours ())
> +      data.inf_had_term = true;
> +
>      if (target_supports_terminal_ours ())
>        {
>  	term_state.emplace ();
> @@ -475,6 +598,12 @@ debuginfod_section_query (const unsigned char *build_id,
>  
>      fd = scoped_fd (debuginfod_find_section (c, build_id, build_id_len,
>  					     section_name, &dname));
> +
> +    if (data.pass_quit_flag)
> +      set_quit_flag ();
> +    if (data.inf_had_term && term_state.has_value ())
> +      target_terminal::inferior ();
> +
>      debuginfod_set_user_data (c, nullptr);
>    }
>  
> @@ -523,6 +652,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
> @@ -627,6 +783,23 @@ When set to \"ask\", prompt whether to enable or disable debuginfod." ),
>  			&set_debuginfod_prefix_list,
>  			&show_debuginfod_prefix_list);
>  
> +  add_setshow_enum_cmd ("cancel", class_run, debuginfod_cancel_enum,
> +			_("Set Ctrl-C behaviour for debuginfod."),
> +			_("Show Ctrl-C behaviour for debuginfod."),
> +			_("\
> +When set to \'one\', pressing Ctrl-C twice cancels a single \
> +download.\nWhen set to \'all\', pressing Ctrl-C twice cancels all further downloads.\n\
> +When set to \'ask\', pressing Ctrl-C twice asks what to do.\nA single Ctrl-C during \
> +downloading is passed to the target process being debugged.\nA second Ctrl-C \
> +during downloading may raise a prompt asking whether to cancel the download or \
> +send Ctrl-C to the target.\nIf the download is cancelled, then no Ctrl-C is \
> +sent to the target."),
> +			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 e98c15242bc..d6df28896b1 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -50259,6 +50259,27 @@ 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 @kbd{Ctrl-C} behaviour
> +Pressing @kbd{Ctrl-C} twice during downloading will cancel the current
> +download.
> +
> +@item set debuginfod cancel all
> +Pressing @kbd{Ctrl-C} twice during downloading will cancel this and all
> +further downloads.
> +
> +@item set debuginfod cancel ask
> +Pressing @kbd{Ctrl-C} twice during downloading will cancel the current
> +download and prompt whether to cancel all further downloads.  By default,
> +@code{debuginfod cancel} is set to @code{ask} for interactive sessions.
> +
> +@kindex show debuginfod cancel
> +@item show debuginfod cancel
> +Display the current setting of @code{debuginfod cancel}.
> +
>  @kindex set debuginfod urls
>  @cindex configure debuginfod URLs
>  @item set debuginfod urls
> -- 
> 2.43.0

Re: [PATCH v4] gdb/debuginfod: Ctrl-C asks to cancel further downloads

[Aaron Merey]

Hi Andrew,

Thanks for the review.

On Tue, Jun 11, 2024 at 7:43 AM Andrew Burgess <aburgess@redhat.com> wrote:
> > +   if (!target_terminal::is_ours ())
> > +     data.inf_had_term = true;
>
> Every time you create a user_data object you immediately follow up by
> setting the ::inf_had_term field.  I think it would be nicer to just add
> an extra argument to the constructor and pass target_terminal::is_ours()
> through.
>
> ... Or maybe user_data should just call target_terminal::is_ours()
> directly and you'd just add a comment to the constructor saying that it
> records the current ownership of the terminal for the current inferior?
>
> Not sure which would be better.

I changed the user_data constructor to call target_terminal::is_ours
since this results in slightly less code duplication.

I've updated patch below with this change as well as UI text change
that Keith suggested.

Aaron

---
Currently gdb only allows for debuginfod downloads to be cancelled one at
a time via Ctrl-C.  This can be a burden if one wishes to cancel a large
series of downloads.  Additionally there can also be ambiguity between whether
Ctrl-C during a download was intended to cancel a download or interrupt the
inferior.

This patch addresses these issues by adding a new debuginfod setting and
changing the behavior of Ctrl-C during a download.

Add a new command "set debuginfod cancel one/all/ask", where:
- "one" means Ctrl-C cancels one download,
- "all" means Ctrl-C cancels all further downloads, and
- "ask" means Ctrl-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".

A single Ctrl-C during downloading now sets the quit_flag and proceeds with
all downloads.  If the inferior has the terminal, then a second Ctrl-C during
downloading triggers a query asking whether to cancel the download or
interrupt the inferior.  If the user wishes to cancel the download then
the setting of 'set debuginfod cancel' determines whether one or all
downloads are cancelled.  In the case of "set debuginfod cancel ask",
there will be another query at this point asking whether to cancel one
or all downloads.

If the inferior does not have the terminal, then a second Ctrl-C during
downloading simply cancels the download according to the setting of
"set debuginfod cancel".  In this case there is no query asking whether
to interrupt the inferior or cancel a download.

Example session where inferior has terminal:

    (gdb) run
    [...]
    Downloading separate debug info for /lib64/libxyz.so
    [###                                                            ]^C^C
    Cancel the current download?
    If no, then Ctrl-C will be sent to the target process. ([y] or n) y
    Cancelling download of separate debug info for /lib64/libxyz.so...
    Cancel further downloading for this session? (y or [n]) n
    Downloading separate debug info for /lib64/libabcd.so

Example session where inferior does not have terminal:

    (gdb) run
    [...]
    Downloading separate debug info for /lib64/libxyz.so
    [###                                                            ]^C^C
    Cancelling download of separate debug info for /lib64/libxyz.so...
    Cancel further downloading for this session? (y or [n]) y
    Interrupted -- debuginfod will be disabled.
    To re-enable use the 'set debuginfod enabled' command.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29582
Suggested-By: Martin Liška <mliska@suse.cz>
Co-Authored-By: Tom de Vries <tdevries@suse.de>
Reviewed-by: Keith Seitz <keiths@redhat.com>
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
---
 gdb/debuginfod-support.c | 174 +++++++++++++++++++++++++++++++++++++--
 gdb/doc/gdb.texinfo      |  21 +++++
 2 files changed, 189 insertions(+), 6 deletions(-)

diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index 841b6f2078c..28cedf6b7a8 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -37,6 +37,8 @@ static cmd_list_element *maint_show_debuginfod_cmdlist;
 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[] =
 {
@@ -46,6 +48,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;
@@ -108,9 +124,17 @@ debuginfod_section_query (const unsigned char *build_id,
 struct user_data
 {
   user_data (const char *desc, const char *fname)
-    : desc (desc), fname (fname)
-  { }
+    : pass_quit_flag (false), desc (desc), fname (fname)
+  {
+    /* Record whether the current inferior owns the terminal.  */
+    if (!target_terminal::is_ours ())
+      inf_had_term = true;
+    else
+      inf_had_term = false;
+  }
 
+  bool pass_quit_flag;
+  bool inf_had_term;
   const char * const desc;
   const char * const fname;
   ui_out::progress_update progress;
@@ -155,10 +179,84 @@ progressfn (debuginfod_client *c, long cur, long total)
   if (check_quit_flag ())
     {
       ui_file *outstream = get_unbuffered (gdb_stdout);
-      gdb_printf (outstream, _("Cancelling download of %s %s...\n"),
-		  data->desc, styled_fname.c_str ());
-      return 1;
-    }
+
+      /* If a single Ctrl-C occurs during downloading, let it propagate to the
+	 target.  If more than one Ctrl-C occurs, ask whether to cancel the
+	 current download or interrupt the target.  If the download is
+	 cancelled, the setting of debuginfod_cancel will determine whether
+	 the current download is cancelled or debuginfod is disabled.  */
+      if (!data->pass_quit_flag)
+	data->pass_quit_flag = true;
+      else
+	{
+	  int resp = 1;
+	  bool extra_nl = false;
+
+	  if (data->inf_had_term)
+	    {
+	      /* If Ctrl-C occurs during the following prompts, catch the
+		 exception to prevent unsafe early returns to gdb's main
+		 event loop.  During these prompts, Ctrl-C is equivalent to
+		 answering 'y'.  */
+	      try
+		{
+		  resp = yquery (_("Cancel the current download?\nIf no, "
+				   "then Ctrl-C will be sent to the target "
+				   "process. "));
+		}
+	      catch (const gdb_exception &)
+		{
+		  /* If the query doesn't complete, then we need an additional
+		     newline to get "Cancelling download of..." printed on a
+		     separate line.  */
+		  extra_nl = true;
+		}
+	    }
+	  if (resp)
+	    {
+	      if (extra_nl)
+		{
+		  gdb_printf (outstream, "\n");
+		  extra_nl = false;
+		}
+
+	      gdb_printf (outstream, _("Cancelling download of %s %s...\n"),
+				       data->desc, styled_fname.c_str ());
+	      if (debuginfod_cancel == debuginfod_ask)
+		{
+		  try
+		    {
+		      resp = nquery
+			(_("Cancel further downloading for this session? "));
+		    }
+		  catch (const gdb_exception &)
+		    {
+		      resp = 1;
+		      extra_nl = true;
+		    }
+
+		  if (resp)
+		    debuginfod_cancel = debuginfod_all;
+		  else
+		    debuginfod_cancel = debuginfod_one;
+		}
+	      if (debuginfod_cancel == debuginfod_all)
+		{
+		  if (extra_nl)
+		    gdb_printf (outstream, "\n");
+
+		  gdb_printf (outstream,
+			      _("Interrupted -- debuginfod will be disabled.\n"
+				"To re-enable use the 'set debuginfod "
+				"enabled' command.\n"));
+		  debuginfod_enabled = debuginfod_off;
+		}
+
+	    data->pass_quit_flag = false;
+	    return 1;
+	  }
+      }
+  }
 
   if (debuginfod_verbose == 0)
     return 0;
@@ -334,6 +432,11 @@ debuginfod_source_query (const unsigned char *build_id,
 					    build_id_len,
 					    srcpath,
 					    &dname));
+    if (data.pass_quit_flag)
+      set_quit_flag ();
+    if (data.inf_had_term && term_state.has_value ())
+      target_terminal::inferior ();
+
     debuginfod_set_user_data (c, nullptr);
   }
 
@@ -377,6 +480,11 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
 
     fd = scoped_fd (debuginfod_find_debuginfo (c, build_id, build_id_len,
 					       &dname));
+    if (data.pass_quit_flag)
+      set_quit_flag ();
+    if (data.inf_had_term && term_state.has_value ())
+      target_terminal::inferior ();
+
     debuginfod_set_user_data (c, nullptr);
   }
 
@@ -420,6 +528,11 @@ debuginfod_exec_query (const unsigned char *build_id,
 
     fd = scoped_fd (debuginfod_find_executable (c, build_id, build_id_len,
 						&dname));
+    if (data.pass_quit_flag)
+      set_quit_flag ();
+    if (data.inf_had_term && term_state.has_value ())
+      target_terminal::inferior ();
+
     debuginfod_set_user_data (c, nullptr);
   }
 
@@ -468,6 +581,11 @@ debuginfod_section_query (const unsigned char *build_id,
 
     fd = scoped_fd (debuginfod_find_section (c, build_id, build_id_len,
 					     section_name, &dname));
+    if (data.pass_quit_flag)
+      set_quit_flag ();
+    if (data.inf_had_term && term_state.has_value ())
+      target_terminal::inferior ();
+
     debuginfod_set_user_data (c, nullptr);
   }
 
@@ -516,6 +634,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
@@ -620,6 +765,23 @@ When set to \"ask\", prompt whether to enable or disable debuginfod." ),
 			&set_debuginfod_prefix_list,
 			&show_debuginfod_prefix_list);
 
+  add_setshow_enum_cmd ("cancel", class_run, debuginfod_cancel_enum,
+			_("Set Ctrl-C behaviour for debuginfod."),
+			_("Show Ctrl-C behaviour for debuginfod."),
+			_("\
+When set to \'one\', pressing Ctrl-C twice cancels a single \
+download.\nWhen set to \'all\', pressing Ctrl-C twice cancels all further downloads.\n\
+When set to \'ask\', pressing Ctrl-C twice asks what to do.\nA single Ctrl-C during \
+downloading is passed to the target process being debugged.\nA second Ctrl-C \
+during downloading may raise a prompt asking whether to cancel the download or \
+send Ctrl-C to the target.\nIf the download is cancelled, then no Ctrl-C is \
+sent to the target."),
+			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 750f368f980..42b7459a25c 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -50320,6 +50320,27 @@ 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 @kbd{Ctrl-C} behaviour
+Pressing @kbd{Ctrl-C} twice during downloading will cancel the current
+download.
+
+@item set debuginfod cancel all
+Pressing @kbd{Ctrl-C} twice during downloading will cancel this and all
+further downloads.
+
+@item set debuginfod cancel ask
+Pressing @kbd{Ctrl-C} twice during downloading will cancel the current
+download and prompt whether to cancel all further downloads.  By default,
+@code{debuginfod cancel} is set to @code{ask} for interactive sessions.
+
+@kindex show debuginfod cancel
+@item show debuginfod cancel
+Display the current setting of @code{debuginfod cancel}.
+
 @kindex set debuginfod urls
 @cindex configure debuginfod URLs
 @item set debuginfod urls
-- 
2.45.2

Re: [PATCH v4] gdb/debuginfod: Ctrl-C asks to cancel further downloads

[Eli Zaretskii]

> From: Aaron Merey <amerey@redhat.com>
> Cc: gdb-patches@sourceware.org,
> 	mliska@suse.cz,
> 	tdevries@suse.de,
> 	keiths@redhat.com,
> 	Aaron Merey <amerey@redhat.com>,
> 	Eli Zaretskii <eliz@gnu.org>
> Date: Tue, 11 Jun 2024 16:56:12 -0400
> 
> Hi Andrew,
> 
> Thanks for the review.
> 
> On Tue, Jun 11, 2024 at 7:43 AM Andrew Burgess <aburgess@redhat.com> wrote:
> > > +   if (!target_terminal::is_ours ())
> > > +     data.inf_had_term = true;
> >
> > Every time you create a user_data object you immediately follow up by
> > setting the ::inf_had_term field.  I think it would be nicer to just add
> > an extra argument to the constructor and pass target_terminal::is_ours()
> > through.
> >
> > ... Or maybe user_data should just call target_terminal::is_ours()
> > directly and you'd just add a comment to the constructor saying that it
> > records the current ownership of the terminal for the current inferior?
> >
> > Not sure which would be better.
> 
> I changed the user_data constructor to call target_terminal::is_ours
> since this results in slightly less code duplication.
> 
> I've updated patch below with this change as well as UI text change
> that Keith suggested.

Thanks, the documentation parts are okay.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Re: [PATCH v4] gdb/debuginfod: Ctrl-C asks to cancel further downloads

[Aaron Merey]

On Wed, Jun 12, 2024 at 3:23 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Aaron Merey <amerey@redhat.com>
> > Cc: gdb-patches@sourceware.org,
> >       mliska@suse.cz,
> >       tdevries@suse.de,
> >       keiths@redhat.com,
> >       Aaron Merey <amerey@redhat.com>,
> >       Eli Zaretskii <eliz@gnu.org>
> > Date: Tue, 11 Jun 2024 16:56:12 -0400
> >
> > I changed the user_data constructor to call target_terminal::is_ours
> > since this results in slightly less code duplication.
> >
> > I've updated patch below with this change as well as UI text change
> > that Keith suggested.
>
> Thanks, the documentation parts are okay.
>
> Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Ok to merge?

Aaron

Re: [PATCH v4] gdb/debuginfod: Ctrl-C asks to cancel further downloads

[Aaron Merey]

On Fri, Jun 14, 2024 at 2:22 PM Aaron Merey <amerey@redhat.com> wrote:
>
> On Wed, Jun 12, 2024 at 3:23 AM Eli Zaretskii <eliz@gnu.org> wrote:
> >
> > > From: Aaron Merey <amerey@redhat.com>
> > > Cc: gdb-patches@sourceware.org,
> > >       mliska@suse.cz,
> > >       tdevries@suse.de,
> > >       keiths@redhat.com,
> > >       Aaron Merey <amerey@redhat.com>,
> > >       Eli Zaretskii <eliz@gnu.org>
> > > Date: Tue, 11 Jun 2024 16:56:12 -0400
> > >
> > > I changed the user_data constructor to call target_terminal::is_ours
> > > since this results in slightly less code duplication.
> > >
> > > I've updated patch below with this change as well as UI text change
> > > that Keith suggested.
> >
> > Thanks, the documentation parts are okay.
> >
> > Reviewed-By: Eli Zaretskii <eliz@gnu.org>
>
> Ok to merge?

Ping. Ok to merge?

Aaron