[PING*3][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 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
> > >