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

[Aaron Merey <amerey@redhat.com>]

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
> >