Let user C-c when waiting for DWARF index finalization

  In PR gdb/29854, Simon pointed out that it would be good to be able to
use C-c when the DWARF cooked index is waiting for finalization.  The
idea here is to be able to interrupt a command like "break" -- not to
stop the finalization process itself, which runs in a worker thread.

This patch implements this idea, by changing the index wait functions
to, by default, allow a quit.  Polling is done, because there doesn't
seem to be a better way to interrupt a wait on a std::future.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29854
---
 gdb/dwarf2/cooked-index.c | 17 +++++++++++++++++
 gdb/dwarf2/cooked-index.h | 11 +++++------
 2 files changed, 22 insertions(+), 6 deletions(-)
  

On 12/6/22 14:27, Tom Tromey via Gdb-patches wrote:
> In PR gdb/29854, Simon pointed out that it would be good to be able to
> use C-c when the DWARF cooked index is waiting for finalization.  The
> idea here is to be able to interrupt a command like "break" -- not to
> stop the finalization process itself, which runs in a worker thread.
> 
> This patch implements this idea, by changing the index wait functions
> to, by default, allow a quit.  Polling is done, because there doesn't
> seem to be a better way to interrupt a wait on a std::future.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29854

Thanks for doing it.  It's not mandatory, but it fixes a little
annoyance I hit while debugging GDB with an unoptimized GDB (the
gdb.gdb/selftest.exp test).  It's unfortunate that there isn't a nicer
way, but this works.

The patch LGTM, but since I was the one to request to the feature, we
could maybe let someone else review and ack.

Simon
  

On Tue, Dec 06, 2022 at 12:27:43PM -0700, Tom Tromey via Gdb-patches wrote:
> In PR gdb/29854, Simon pointed out that it would be good to be able to
> use C-c when the DWARF cooked index is waiting for finalization.  The
> idea here is to be able to interrupt a command like "break" -- not to
> stop the finalization process itself, which runs in a worker thread.
> 
> This patch implements this idea, by changing the index wait functions
> to, by default, allow a quit.  Polling is done, because there doesn't
> seem to be a better way to interrupt a wait on a std::future.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29854
> ---
>  gdb/dwarf2/cooked-index.c | 17 +++++++++++++++++
>  gdb/dwarf2/cooked-index.h | 11 +++++------
>  2 files changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c
> index 0aa026c7779..a0b75a311b1 100644
> --- a/gdb/dwarf2/cooked-index.c
> +++ b/gdb/dwarf2/cooked-index.c
> @@ -25,6 +25,7 @@
>  #include "ada-lang.h"
>  #include "split-name.h"
>  #include <algorithm>
> +#include <chrono>
>  
>  /* Hash function for cooked_index_entry.  */
>  
> @@ -282,6 +283,22 @@ cooked_index::find (gdb::string_view name, bool completing)
>    return range (lower, upper);
>  }
>  
> +/* See cooked-index.h.  */
> +
> +void
> +cooked_index::wait (bool allow_quit)
> +{
> +  if (allow_quit)
> +    {
> +      using namespace std::chrono_literals;
> +      auto duration = 15ms;
> +      while (m_future.wait_for (duration) == std::future_status::timeout)
> +	QUIT;

Hi,

Thanks for doing this!

I think the "ms" suffix from chrono_literals have been introduced in
c++14 which we do not support. 

I guess that something like this should work for c++11:

    std::chrono::milliseconds duration { 15 };

I still have on my todo list to ensure that major distros do have a
decent and easily installable c++14 compiler so we can bump this
requirement in gdb, but I have not done this yet…

Except for this, and for what it is worth, this patch looks nice!

Best,
Lancelot.

> +    }
> +  else
> +    m_future.wait ();
> +}
> +
>  cooked_index_vector::cooked_index_vector (vec_type &&vec)
>    : m_vector (std::move (vec))
>  {
> diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h
> index 2ea32781be5..5412149588a 100644
> --- a/gdb/dwarf2/cooked-index.h
> +++ b/gdb/dwarf2/cooked-index.h
> @@ -212,10 +212,7 @@ class cooked_index
>    void finalize ();
>  
>    /* Wait for this index's finalization to be complete.  */
> -  void wait ()
> -  {
> -    m_future.wait ();
> -  }
> +  void wait (bool allow_quit = true);
>  
>    friend class cooked_index_vector;
>  
> @@ -325,8 +322,10 @@ class cooked_index_vector : public dwarf_scanner_base
>         end up writing to freed memory.  Waiting for finalization to
>         complete avoids this problem; and the cost seems ignorable
>         because creating and immediately destroying the debug info is a
> -       relatively rare thing to do.  */
> -    wait ();
> +       relatively rare thing to do.  Do not allow quitting from this
> +       wait.  */
> +    for (auto &item : m_vector)
> +      item->wait (false);
>    }
>  
>    /* A range over a vector of subranges.  */
> -- 
> 2.34.3
>
  

Lancelot> I think the "ms" suffix from chrono_literals have been introduced in
Lancelot> c++14 which we do not support. 

Yeah, thanks for pointing that out.  I misread the cppreference page and
thought they were C++11.

Lancelot> I guess that something like this should work for c++11:

Lancelot>     std::chrono::milliseconds duration { 15 };

I've made this change.

Lancelot> I still have on my todo list to ensure that major distros do have a
Lancelot> decent and easily installable c++14 compiler so we can bump this
Lancelot> requirement in gdb, but I have not done this yet…

Yeah, that sounds nice.

Tom
  

>>>>> "Tom" == Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> In PR gdb/29854, Simon pointed out that it would be good to be able to
Tom> use C-c when the DWARF cooked index is waiting for finalization.  The
Tom> idea here is to be able to interrupt a command like "break" -- not to
Tom> stop the finalization process itself, which runs in a worker thread.

Tom> This patch implements this idea, by changing the index wait functions
Tom> to, by default, allow a quit.  Polling is done, because there doesn't
Tom> seem to be a better way to interrupt a wait on a std::future.

I'm not totally sure what to do with this patch.
Should it go in GDB 13?

Tom> +void
Tom> +cooked_index::wait (bool allow_quit)
Tom> +{
Tom> +  if (allow_quit)
Tom> +    {
Tom> +      using namespace std::chrono_literals;
Tom> +      auto duration = 15ms;
Tom> +      while (m_future.wait_for (duration) == std::future_status::timeout)

I did belatedly realize this will need an update to the gdb::future
compatibility wrapper.

I'll do a --disable-threading build before patching this.

Tom