[PATCH] Let user C-c when waiting for DWARF index finalization

[PATCH] Let user C-c when waiting for DWARF index finalization

[Tom Tromey]

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;
+    }
+  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

Re: [PATCH] Let user C-c when waiting for DWARF index finalization

[Simon Marchi]

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

Re: [PATCH] Let user C-c when waiting for DWARF index finalization

[Lancelot SIX]

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
> 

Re: [PATCH] Let user C-c when waiting for DWARF index finalization

[Tom Tromey]

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

Re: [PATCH] Let user C-c when waiting for DWARF index finalization

[Tom Tromey]

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