[PATCH v2] Capture warnings when writing to the index cache

[PATCH v2] Capture warnings when writing to the index cache

[Tom Tromey]

PR symtab/30837 points out a race that can occur when writing to the
index cache: a call to ada_encode can cause a warning, which is
forbidden on a worker thread.

This patch fixes the problem by arranging to capture any such
warnings.

This is v2 of the patch.  It is rebased on top of some other changes
in the same area.  v1 was here:

    https://sourceware.org/pipermail/gdb-patches/2024-February/206595.html

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30837
---
 gdb/dwarf2/cooked-index.c     | 18 +++++++++++++-----
 gdb/dwarf2/cooked-index.h     |  9 ++++++---
 gdb/dwarf2/read-debug-names.c |  2 +-
 gdb/dwarf2/read.c             |  2 +-
 gdb/utils.h                   | 14 +++++++++++++-
 5 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c
index 2a1ca6fd225..f78b00df446 100644
--- a/gdb/dwarf2/cooked-index.c
+++ b/gdb/dwarf2/cooked-index.c
@@ -581,10 +581,18 @@ cooked_index_worker::set (cooked_state desired_state)
 /* See cooked-index.h.  */
 
 void
-cooked_index_worker::write_to_cache (const cooked_index *idx) const
+cooked_index_worker::write_to_cache (const cooked_index *idx,
+				     deferred_warnings *warn) const
 {
   if (idx != nullptr)
-    m_cache_store.store ();
+    {
+      /* Writing to the index cache may cause a warning to be emitted.
+	 See PR symtab/30837.  This arranges to capture all such
+	 warnings.  This is safe because we know the deferred_warnings
+	 object isn't in use by any other thread at this point.  */
+      scoped_restore_warning_hook defer (*warn);
+      m_cache_store.store ();
+    }
 }
 
 cooked_index::cooked_index (dwarf2_per_objfile *per_objfile,
@@ -623,7 +631,7 @@ cooked_index::wait (cooked_state desired_state, bool allow_quit)
 }
 
 void
-cooked_index::set_contents (vec_type &&vec)
+cooked_index::set_contents (vec_type &&vec, deferred_warnings *warn)
 {
   gdb_assert (m_vector.empty ());
   m_vector = std::move (vec);
@@ -635,10 +643,10 @@ cooked_index::set_contents (vec_type &&vec)
      finalization.  However, that would take a slot in the global
      thread pool, and if enough such tasks were submitted at once, it
      would cause a livelock.  */
-  gdb::task_group finalizers ([this] ()
+  gdb::task_group finalizers ([=] ()
   {
     m_state->set (cooked_state::FINALIZED);
-    m_state->write_to_cache (index_for_writing ());
+    m_state->write_to_cache (index_for_writing (), warn);
     m_state->set (cooked_state::CACHE_DONE);
   });
 
diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h
index 91419b79edc..19720c256db 100644
--- a/gdb/dwarf2/cooked-index.h
+++ b/gdb/dwarf2/cooked-index.h
@@ -494,7 +494,8 @@ class cooked_index_worker
   void set (cooked_state desired_state);
 
   /* Write to the index cache.  */
-  void write_to_cache (const cooked_index *idx) const;
+  void write_to_cache (const cooked_index *idx,
+		       deferred_warnings *warn) const;
 
   /* Helper function that does the work of reading.  This must be able
      to be run in a worker thread without problems.  */
@@ -615,8 +616,10 @@ class cooked_index : public dwarf_scanner_base
   void start_reading ();
 
   /* Called by cooked_index_worker to set the contents of this index
-     and transition to the MAIN_AVAILABLE state.  */
-  void set_contents (vec_type &&vec);
+     and transition to the MAIN_AVAILABLE state.  WARN is used to
+     collect any warnings that may arise when writing to the
+     cache.  */
+  void set_contents (vec_type &&vec, deferred_warnings *warn);
 
   /* A range over a vector of subranges.  */
   using range = range_chain<cooked_index_shard::range>;
diff --git a/gdb/dwarf2/read-debug-names.c b/gdb/dwarf2/read-debug-names.c
index 0add8040894..0d60b01f408 100644
--- a/gdb/dwarf2/read-debug-names.c
+++ b/gdb/dwarf2/read-debug-names.c
@@ -352,7 +352,7 @@ cooked_index_debug_names::do_reading ()
   cooked_index *table
     = (gdb::checked_static_cast<cooked_index *>
        (per_bfd->index_table.get ()));
-  table->set_contents (std::move (indexes));
+  table->set_contents (std::move (indexes), &m_warnings);
 
   bfd_thread_cleanup ();
 }
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 4afb026b8ce..390224dfe1c 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -4923,7 +4923,7 @@ cooked_index_debug_info::done_reading ()
   cooked_index *table
     = (gdb::checked_static_cast<cooked_index *>
        (per_bfd->index_table.get ()));
-  table->set_contents (std::move (indexes));
+  table->set_contents (std::move (indexes), &m_warnings);
 }
 
 void
diff --git a/gdb/utils.h b/gdb/utils.h
index 2acd1fc4624..d7db1d84e2f 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -21,6 +21,7 @@
 
 #include "exceptions.h"
 #include "gdbsupport/array-view.h"
+#include "gdbsupport/function-view.h"
 #include "gdbsupport/scoped_restore.h"
 #include <chrono>
 
@@ -374,7 +375,7 @@ assign_return_if_changed (T &lval, const T &val)
 }
 
 /* A function that can be used to intercept warnings.  */
-typedef void (*warning_hook_handler) (const char *, va_list);
+typedef gdb::function_view<void (const char *, va_list)> warning_hook_handler;
 
 /* Set the thread-local warning hook, and restore the old value when
    finished.  */
@@ -439,6 +440,17 @@ struct deferred_warnings
     m_warnings.emplace_back (std::move (msg));
   }
 
+  /* A variant of 'warn' so that this object can be used as a warning
+     hook; see scoped_restore_warning_hook.  Note that no locking is
+     done, so users have to be careful to only install this into a
+     single thread at a time.  */
+  void operator() (const char *format, va_list args) ATTRIBUTE_PRINTF (2, 0)
+  {
+    string_file msg (m_can_style);
+    msg.vprintf (format, args);
+    m_warnings.emplace_back (std::move (msg));
+  }
+
   /* Emit all warnings.  */
   void emit () const
   {
-- 
2.43.0

Re: [PATCH v2] Capture warnings when writing to the index cache

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes:

Tom> PR symtab/30837 points out a race that can occur when writing to the
Tom> index cache: a call to ada_encode can cause a warning, which is
Tom> forbidden on a worker thread.

Tom> This patch fixes the problem by arranging to capture any such
Tom> warnings.

Tom> This is v2 of the patch.  It is rebased on top of some other changes
Tom> in the same area.  v1 was here:

Tom>     https://sourceware.org/pipermail/gdb-patches/2024-February/206595.html

Tom> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30837

I'm checking this in now.

Tom

Re: [PATCH v2] Capture warnings when writing to the index cache

[Simon Marchi]

On 2024-03-26 11:49, Tom Tromey wrote:
>>>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes:
> 
> Tom> PR symtab/30837 points out a race that can occur when writing to the
> Tom> index cache: a call to ada_encode can cause a warning, which is
> Tom> forbidden on a worker thread.
> 
> Tom> This patch fixes the problem by arranging to capture any such
> Tom> warnings.
> 
> Tom> This is v2 of the patch.  It is rebased on top of some other changes
> Tom> in the same area.  v1 was here:
> 
> Tom>     https://sourceware.org/pipermail/gdb-patches/2024-February/206595.html
> 
> Tom> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30837
> 
> I'm checking this in now.
> 
> Tom

Hi Tom,

When building with Clang 17, I see:

    /home/simark/src/binutils-gdb/gdb/../gdbsupport/function-view.h:300:37: error: format string is not a string literal [-Werror,-Wformat-nonliteral]
      300 |       noexcept (noexcept (callable (std::forward<Args> (args)...))) -> Res
          |                                     ^~~~~~~~~~~~~~~~~~~~~~~~~
    /home/simark/src/binutils-gdb/gdb/../gdbsupport/function-view.h:299:17: note: while substituting into a lambda expression here
      299 |     m_invoker = [] (fv_detail::erased_callable ecall, Args... args)
          |                 ^
    /home/simark/src/binutils-gdb/gdb/../gdbsupport/function-view.h:265:5: note: in instantiation of function template specialization 'gdb::function_view<void (const char *, __va_list_tag *)>::bind<deferred_warnings>' requested here
      265 |     bind (callable);
          |     ^
    /home/simark/src/binutils-gdb/gdb/dwarf2/cooked-index.c:592:42: note: in instantiation of function template specialization 'gdb::function_view<void (const char *, __va_list_tag *)>::function_view<deferred_warnings &, void, void>' requested here
      592 |       scoped_restore_warning_hook defer (*warn);
          |                                          ^
    In file included from <built-in>:1:
    In file included from /home/simark/src/binutils-gdb/gdb/defs.h:620:
    In file included from /home/simark/src/binutils-gdb/gdb/utils.h:24:
    /home/simark/src/binutils-gdb/gdb/../gdbsupport/function-view.h:305:34: error: format string is not a string literal [-Werror,-Wformat-nonliteral]
      305 |         return (Res) restored_callable (std::forward<Args> (args)...);
          |                                         ^~~~~~~~~~~~~~~~~~~~~~~~~

My interpretation of this is that we bind a function marked with
`ATTRIBUTE_PRINTF (2,0)` (deferred_warnings::operator()) to a function
view that doesn't convey the attribute.  So I guess the compiler is
telling us that whenever the function view is invoked, there won't be a
format string check, and that's dangerous.

I didn't find a good fix that :(.

Simon

Re: [PATCH v2] Capture warnings when writing to the index cache

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> My interpretation of this is that we bind a function marked with
Simon> `ATTRIBUTE_PRINTF (2,0)` (deferred_warnings::operator()) to a function
Simon> view that doesn't convey the attribute.  So I guess the compiler is
Simon> telling us that whenever the function view is invoked, there won't be a
Simon> format string check, and that's dangerous.

Simon> I didn't find a good fix that :(.

Please try the appended.

thanks,
Tom

commit 8be8061ea5d0ec37289070f16717d3d0aa0c93af
Author: Tom Tromey <tromey@adacore.com>
Date:   Wed Mar 27 07:21:56 2024 -0600

    patch

diff --git a/gdb/utils.h b/gdb/utils.h
index d7db1d84e2f..875a2583179 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -444,10 +444,18 @@ struct deferred_warnings
      hook; see scoped_restore_warning_hook.  Note that no locking is
      done, so users have to be careful to only install this into a
      single thread at a time.  */
-  void operator() (const char *format, va_list args) ATTRIBUTE_PRINTF (2, 0)
+  void operator() (const char *format, va_list args)
   {
     string_file msg (m_can_style);
+    /* Clang warns if we add ATTRIBUTE_PRINTF to this method (because
+       the function-view wrapper doesn't also have the attribute), but
+       then warns again if we remove it, because this vprintf call
+       does not use a literal format string.  So, suppress the
+       warnings here.  */
+    DIAGNOSTIC_PUSH
+    DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL
     msg.vprintf (format, args);
+    DIAGNOSTIC_POP
     m_warnings.emplace_back (std::move (msg));
   }
 

Re: [PATCH v2] Capture warnings when writing to the index cache

[Simon Marchi]

On 2024-03-27 09:25, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
> Simon> My interpretation of this is that we bind a function marked with
> Simon> `ATTRIBUTE_PRINTF (2,0)` (deferred_warnings::operator()) to a function
> Simon> view that doesn't convey the attribute.  So I guess the compiler is
> Simon> telling us that whenever the function view is invoked, there won't be a
> Simon> format string check, and that's dangerous.
> 
> Simon> I didn't find a good fix that :(.
> 
> Please try the appended.
> 
> thanks,
> Tom
> 
> commit 8be8061ea5d0ec37289070f16717d3d0aa0c93af
> Author: Tom Tromey <tromey@adacore.com>
> Date:   Wed Mar 27 07:21:56 2024 -0600
> 
>     patch
> 
> diff --git a/gdb/utils.h b/gdb/utils.h
> index d7db1d84e2f..875a2583179 100644
> --- a/gdb/utils.h
> +++ b/gdb/utils.h
> @@ -444,10 +444,18 @@ struct deferred_warnings
>       hook; see scoped_restore_warning_hook.  Note that no locking is
>       done, so users have to be careful to only install this into a
>       single thread at a time.  */
> -  void operator() (const char *format, va_list args) ATTRIBUTE_PRINTF (2, 0)
> +  void operator() (const char *format, va_list args)
>    {
>      string_file msg (m_can_style);
> +    /* Clang warns if we add ATTRIBUTE_PRINTF to this method (because
> +       the function-view wrapper doesn't also have the attribute), but
> +       then warns again if we remove it, because this vprintf call
> +       does not use a literal format string.  So, suppress the
> +       warnings here.  */
> +    DIAGNOSTIC_PUSH
> +    DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL
>      msg.vprintf (format, args);
> +    DIAGNOSTIC_POP
>      m_warnings.emplace_back (std::move (msg));
>    }
>  

It builds fine with that patch.

Thanks,

Simon

Re: [PATCH v2] Capture warnings when writing to the index cache

[Simon Marchi]

On 2024-03-26 11:49, Tom Tromey wrote:
>>>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes:
> 
> Tom> PR symtab/30837 points out a race that can occur when writing to the
> Tom> index cache: a call to ada_encode can cause a warning, which is
> Tom> forbidden on a worker thread.
> 
> Tom> This patch fixes the problem by arranging to capture any such
> Tom> warnings.
> 
> Tom> This is v2 of the patch.  It is rebased on top of some other changes
> Tom> in the same area.  v1 was here:
> 
> Tom>     https://sourceware.org/pipermail/gdb-patches/2024-February/206595.html
> 
> Tom> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30837
> 
> I'm checking this in now.
> 
> Tom

Not sure if this has been reported already, but when I build for
--host=x86_64-w64-mingw32, I get:

  CXX    gdb.o
In file included from /home/simark/src/binutils-gdb/gdb/defs.h:620,
                 from <command-line>:
/home/simark/src/binutils-gdb/gdb/utils.h:378:56: error: ignoring attributes on template argument ‘void(const char*, va_list)’ {aka ‘void(const char*, char*)’} [-Werror=ignored-attributes]
  378 | typedef gdb::function_view<void (const char *, va_list)> warning_hook_handler;
      |                                                        ^

I'm guessing that the problem is some attributes on va_list, on
mingw-w64?  I wouldn't know how to find that.

With that issue and the other one that broke the clang build, using that
function signature in a function view seems pretty fragile.  It would
perhaps be simpler for `warning_hook_handler` to be an abstract base
class with one virtual method (with the same signature as the current
warning_hook_handler).  That method could have whatever attribute it
needs, it wouldn't be a problem.

Simon

Re: [PATCH v2] Capture warnings when writing to the index cache

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> Not sure if this has been reported already, but when I build for
Simon> --host=x86_64-w64-mingw32, I get:

Which compiler is this?

Tom

Re: [PATCH v2] Capture warnings when writing to the index cache

[Simon Marchi]

On 2024-04-08 08:59, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
> Simon> Not sure if this has been reported already, but when I build for
> Simon> --host=x86_64-w64-mingw32, I get:
> 
> Which compiler is this?

x86_64-w64-mingw32-gcc 13.1.0, from the Arch Linux package:

https://archlinux.org/packages/extra/x86_64/mingw-w64-gcc/

Simon

Re: [PATCH v2] Capture warnings when writing to the index cache

[Tom Tromey]

Simon> x86_64-w64-mingw32-gcc 13.1.0, from the Arch Linux package:
Simon> https://archlinux.org/packages/extra/x86_64/mingw-w64-gcc/

Thanks.  I have a patch I'll send shortly.

Tom