[PATCH v2] Ensure index cache entry written in test

[PATCH v2] Ensure index cache entry written in test

[Tom Tromey]

Now that index cache files are written in the background, one test in
index-cache.exp is racy -- it assumes that the cache file will have
been written during startup.

This patch fixes the problem by introducing a new maintenance command
to wait for all pending writes to the index cache.
---
 gdb/NEWS                               |  3 +++
 gdb/doc/gdb.texinfo                    |  6 ++++++
 gdb/dwarf2/cooked-index.c              | 15 +++++++++++++++
 gdb/testsuite/gdb.base/index-cache.exp |  2 ++
 4 files changed, 26 insertions(+)

diff --git a/gdb/NEWS b/gdb/NEWS
index c32ff92c98a..8827efaefb6 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -68,6 +68,9 @@ maintenance info frame-unwinders
   List the frame unwinders currently in effect, starting with the highest
   priority.
 
+maintenance wait-for-index-cache
+  Wait until all pending writes to the index cache have completed.
+
 set always-read-ctf on|off
 show always-read-ctf
   When off, CTF is only read if DWARF is not present.  When on, CTF is
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index bfda7edc4f7..954f1481dae 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -41374,6 +41374,12 @@ give an error.
 For platforms that do support creating the backtrace this feature is
 @code{on} by default.
 
+@kindex maint wait-for-index-cache
+@item maint wait-for-index-cache
+Wait until all pending writes to the index cache have completed.  This
+is used by the test suite to avoid races when the index cache is being
+updated by a worker thread.
+
 @kindex maint with
 @item maint with @var{setting} [@var{value}] [-- @var{command}]
 Like the @code{with} command, but works with @code{maintenance set}
diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c
index 47746aa9905..d09faed268d 100644
--- a/gdb/dwarf2/cooked-index.c
+++ b/gdb/dwarf2/cooked-index.c
@@ -33,6 +33,7 @@
 #include "gdbsupport/selftest.h"
 #include <chrono>
 #include <unordered_set>
+#include "cli/cli-cmds.h"
 
 /* We don't want gdb to exit while it is in the process of writing to
    the index cache.  So, all live cooked index vectors are stored
@@ -640,6 +641,14 @@ wait_for_index_cache (int)
     item->wait_completely ();
 }
 
+/* A maint command to wait for the cache.  */
+
+static void
+maintenance_wait_for_index_cache (const char *args, int from_tty)
+{
+  wait_for_index_cache (0);
+}
+
 void _initialize_cooked_index ();
 void
 _initialize_cooked_index ()
@@ -648,5 +657,11 @@ _initialize_cooked_index ()
   selftests::register_test ("cooked_index_entry::compare", test_compare);
 #endif
 
+  add_cmd ("wait-for-index-cache", class_maintenance,
+	   maintenance_wait_for_index_cache, _("\
+Usage: maintenance wait-for-index-cache\n\
+Wait until all pending writes to the index cache have completed."),
+	   &maintenancelist);
+
   gdb::observers::gdb_exiting.attach (wait_for_index_cache, "cooked-index");
 }
diff --git a/gdb/testsuite/gdb.base/index-cache.exp b/gdb/testsuite/gdb.base/index-cache.exp
index 0614d4ee2db..7fdf90c2c24 100644
--- a/gdb/testsuite/gdb.base/index-cache.exp
+++ b/gdb/testsuite/gdb.base/index-cache.exp
@@ -159,6 +159,8 @@ proc_with_prefix test_cache_enabled_miss { cache_dir } {
 
     run_test_with_flags $cache_dir on {
 
+	gdb_test_no_output "maintenance wait-for-index-cache"
+
 	lassign [ls_host $cache_dir] ret files_after
 	set nfiles_created [expr [llength $files_after] - [llength $files_before]]
 	if { $expecting_index_cache_use } {
-- 
2.39.1

Re: [PATCH v2] Ensure index cache entry written in test

[Simon Marchi]

> diff --git a/gdb/testsuite/gdb.base/index-cache.exp b/gdb/testsuite/gdb.base/index-cache.exp
> index 0614d4ee2db..7fdf90c2c24 100644
> --- a/gdb/testsuite/gdb.base/index-cache.exp
> +++ b/gdb/testsuite/gdb.base/index-cache.exp
> @@ -159,6 +159,8 @@ proc_with_prefix test_cache_enabled_miss { cache_dir } {
>  
>      run_test_with_flags $cache_dir on {
>  
> +	gdb_test_no_output "maintenance wait-for-index-cache"

I would perhaps put this command in the run_test_with_flags proc, so
that it applies to other tests as well.  It shouldn't be necessary for
those tests, but if there's a bug somewhere, it would make it reproduce
more reliably.

Otherwise:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

Re: [PATCH v2] Ensure index cache entry written in test

[Tom Tromey]

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

>> +	gdb_test_no_output "maintenance wait-for-index-cache"

Simon> I would perhaps put this command in the run_test_with_flags proc, so
Simon> that it applies to other tests as well.  It shouldn't be necessary for
Simon> those tests, but if there's a bug somewhere, it would make it reproduce
Simon> more reliably.

I made this change.  It required the appended hunk as well, to avoid a
duplicate test name.

I'm going to check it in now.

Tom

@@ -203,7 +205,9 @@ proc_with_prefix test_cache_enabled_hit { cache_dir } {
     global expecting_index_cache_use
 
     # Just to populate the cache.
-    run_test_with_flags $cache_dir on {}
+    with_test_prefix "populate cache" {
+	run_test_with_flags $cache_dir on {}
+    }
 
     lassign [ls_host $cache_dir] ret files_before
 

Re: [PATCH v2] Ensure index cache entry written in test

[Simon Marchi]

On 3/7/23 09:58, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
>>> +	gdb_test_no_output "maintenance wait-for-index-cache"
> 
> Simon> I would perhaps put this command in the run_test_with_flags proc, so
> Simon> that it applies to other tests as well.  It shouldn't be necessary for
> Simon> those tests, but if there's a bug somewhere, it would make it reproduce
> Simon> more reliably.
> 
> I made this change.  It required the appended hunk as well, to avoid a
> duplicate test name.
> 
> I'm going to check it in now.

Heh, we get that now:

(gdb) maintenance selftest help
Running selftest help_doc_invariants.
help doc broken invariant: command 'maintenance wait-for-index-cache' help doc first line is not terminated with a '.' character
Self test failed: self-test failed at /home/simark/src/binutils-gdb/gdb/unittests/command-def-selftests.c:99
Ran 1 unit tests, 1 failed

I guess swap the lines?  That's how other commands do it:

    Print value of expression EXP.
    Usage: print [[OPTION]... --] [/FMT] [EXP]

    Print backtrace of all stack frames, or innermost COUNT frames.
    Usage: backtrace [OPTION]... [QUALIFIER]... [COUNT | -COUNT]

Simon

Re: [PATCH v2] Ensure index cache entry written in test

[Tom Tromey]

Simon> Heh, we get that now:

Simon> (gdb) maintenance selftest help
Simon> Running selftest help_doc_invariants.
Simon> help doc broken invariant: command 'maintenance wait-for-index-cache' help doc first line is not terminated with a '.' character
Simon> Self test failed: self-test failed at /home/simark/src/binutils-gdb/gdb/unittests/command-def-selftests.c:99
Simon> Ran 1 unit tests, 1 failed

Ugh, sorry.
I'll send a fix in a second.

Tom

Re: [PATCH v2] Ensure index cache entry written in test

[Simon Marchi]

On 3/7/23 13:25, Tom Tromey wrote:
> Simon> Heh, we get that now:
> 
> Simon> (gdb) maintenance selftest help
> Simon> Running selftest help_doc_invariants.
> Simon> help doc broken invariant: command 'maintenance wait-for-index-cache' help doc first line is not terminated with a '.' character
> Simon> Self test failed: self-test failed at /home/simark/src/binutils-gdb/gdb/unittests/command-def-selftests.c:99
> Simon> Ran 1 unit tests, 1 failed
> 
> Ugh, sorry.
> I'll send a fix in a second.
> 
> Tom

For the record, I still see this intermittently on my CI:

FAIL: gdb.base/index-cache.exp: test_cache_enabled_hit: no files were created
FAIL: gdb.base/index-cache.exp: test_cache_enabled_hit: check index-cache stats

So there must be something else... I'll try to investigate.

Simon

Re: [PATCH v2] Ensure index cache entry written in test

[Tom Tromey]

Simon> For the record, I still see this intermittently on my CI:

Simon> FAIL: gdb.base/index-cache.exp: test_cache_enabled_hit: no files were created
Simon> FAIL: gdb.base/index-cache.exp: test_cache_enabled_hit: check index-cache stats

Simon> So there must be something else... I'll try to investigate.

The patch I sent a bit earlier should probably fix this.

Tom

Re: [PATCH v2] Ensure index cache entry written in test

[Eli Zaretskii]

> Cc: Tom Tromey <tromey@adacore.com>
> Date: Mon,  6 Mar 2023 09:45:53 -0700
> From: Tom Tromey via Gdb-patches <gdb-patches@sourceware.org>
> 
> Now that index cache files are written in the background, one test in
> index-cache.exp is racy -- it assumes that the cache file will have
> been written during startup.
> 
> This patch fixes the problem by introducing a new maintenance command
> to wait for all pending writes to the index cache.
> ---
>  gdb/NEWS                               |  3 +++
>  gdb/doc/gdb.texinfo                    |  6 ++++++
>  gdb/dwarf2/cooked-index.c              | 15 +++++++++++++++
>  gdb/testsuite/gdb.base/index-cache.exp |  2 ++
>  4 files changed, 26 insertions(+)

OK for the documentation parts.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>