[v2] Ensure index cache entry written in test
Commit Message
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(+)
Comments
> 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
> 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>
>>>>> "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
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
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
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
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
@@ -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
@@ -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}
@@ -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");
}
@@ -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 } {