* [PATCH 1/3] gdbsupport/selftest: Replace for_each_selftest with an iterator_range
2022-04-01 13:00 [PATCH 0/3] Fix register_test_foreach_arch Lancelot SIX
@ 2022-04-01 13:00 ` Lancelot SIX
2022-04-01 13:00 ` [PATCH 2/3] gdbsupport/selftest: Allow lazy registration Lancelot SIX
` (3 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Lancelot SIX @ 2022-04-01 13:00 UTC (permalink / raw)
To: gdb-patches; +Cc: lsix, Lancelot SIX
Remove the callback-based selftests::for_each_selftest function and use
an iterator_range instead.
Also use this iterator range in run_tests so all iterations over the
selftests are done in a consistent way. This will become useful in a
later commit.
Change-Id: I0b3a5349a7987fbcb0071f11c394e353df986583
---
gdb/maint.c | 13 ++++++-------
gdbsupport/selftest.cc | 27 ++++++++++++---------------
gdbsupport/selftest.h | 31 ++++++++++++++++++++++++-------
3 files changed, 42 insertions(+), 29 deletions(-)
diff --git a/gdb/maint.c b/gdb/maint.c
index 8cebd6ab5af..eca8f68d49b 100644
--- a/gdb/maint.c
+++ b/gdb/maint.c
@@ -1184,11 +1184,11 @@ maintenance_selftest_completer (cmd_list_element *cmd,
return;
#if GDB_SELF_TEST
- selftests::for_each_selftest ([&tracker, text] (const std::string &name)
+ for (const auto &test : selftests::all_selftests ())
{
- if (startswith (name.c_str (), text))
- tracker.add_completion (make_unique_xstrdup (name.c_str ()));
- });
+ if (startswith (test.name.c_str (), text))
+ tracker.add_completion (make_unique_xstrdup (test.name.c_str ()));
+ }
#endif
}
@@ -1197,9 +1197,8 @@ maintenance_info_selftests (const char *arg, int from_tty)
{
#if GDB_SELF_TEST
gdb_printf ("Registered selftests:\n");
- selftests::for_each_selftest ([] (const std::string &name) {
- gdb_printf (" - %s\n", name.c_str ());
- });
+ for (const auto &test : selftests::all_selftests ())
+ gdb_printf (" - %s\n", test.name.c_str ());
#else
gdb_printf (_("\
Selftests have been disabled for this build.\n"));
diff --git a/gdbsupport/selftest.cc b/gdbsupport/selftest.cc
index 466d7cfeab5..7077f113f97 100644
--- a/gdbsupport/selftest.cc
+++ b/gdbsupport/selftest.cc
@@ -20,16 +20,15 @@
#include "common-exceptions.h"
#include "common-debug.h"
#include "selftest.h"
-#include <map>
#include <functional>
namespace selftests
{
-/* All the tests that have been registered. Using an std::map allows keeping
+/* All the tests that have been registered. Using an std::set allows keeping
the order of tests stable and easily looking up whether a test name
exists. */
-static std::map<std::string, std::function<void(void)>> tests;
+static selftests_registry tests;
/* See selftest.h. */
@@ -38,9 +37,9 @@ register_test (const std::string &name,
std::function<void(void)> function)
{
/* Check that no test with this name already exist. */
- gdb_assert (tests.find (name) == tests.end ());
-
- tests[name] = function;
+ auto status = tests.emplace (name, std::move (function));
+ if (!status.second)
+ gdb_assert_not_reached ("Test already registered");
}
/* See selftest.h. */
@@ -63,10 +62,8 @@ run_tests (gdb::array_view<const char *const> filters, bool verbose)
int ran = 0, failed = 0;
run_verbose_ = verbose;
- for (const auto &pair : tests)
+ for (const auto &test : all_selftests ())
{
- const std::string &name = pair.first;
- const auto &test = pair.second;
bool run = false;
if (filters.empty ())
@@ -75,7 +72,7 @@ run_tests (gdb::array_view<const char *const> filters, bool verbose)
{
for (const char *filter : filters)
{
- if (name.find (filter) != std::string::npos)
+ if (test.name.find (filter) != std::string::npos)
run = true;
}
}
@@ -85,9 +82,9 @@ run_tests (gdb::array_view<const char *const> filters, bool verbose)
try
{
- debug_printf (_("Running selftest %s.\n"), name.c_str ());
+ debug_printf (_("Running selftest %s.\n"), test.name.c_str ());
++ran;
- test ();
+ test.test ();
}
catch (const gdb_exception_error &ex)
{
@@ -104,10 +101,10 @@ run_tests (gdb::array_view<const char *const> filters, bool verbose)
/* See selftest.h. */
-void for_each_selftest (for_each_selftest_ftype func)
+selftests_range
+all_selftests ()
{
- for (const auto &pair : tests)
- func (pair.first);
+ return selftests_range (tests.cbegin (), tests.cend ());
}
} // namespace selftests
diff --git a/gdbsupport/selftest.h b/gdbsupport/selftest.h
index 3c343e1a761..5ca9bdcc598 100644
--- a/gdbsupport/selftest.h
+++ b/gdbsupport/selftest.h
@@ -21,6 +21,8 @@
#include "gdbsupport/array-view.h"
#include "gdbsupport/function-view.h"
+#include "gdbsupport/iterator-range.h"
+#include <set>
/* A test is just a function that does some checks and throws an
exception if something has gone wrong. */
@@ -28,6 +30,28 @@
namespace selftests
{
+/* Selftests are registered under a unique name. */
+
+struct selftest
+{
+ selftest (std::string name, std::function<void (void)> test)
+ : name { std::move (name) }, test { std::move (test) }
+ { }
+ bool operator< (const selftest &rhs) const
+ { return name < rhs.name; }
+
+ std::string name;
+ std::function<void (void)> test;
+};
+
+/* Type of the container of all the registered selftests. */
+using selftests_registry = std::set<selftest>;
+using selftests_range = iterator_range<selftests_registry::const_iterator>;
+
+/* Create a range to iterate over all registered tests. */
+
+selftests_range all_selftests ();
+
/* True if selftest should run verbosely. */
extern bool run_verbose ();
@@ -48,13 +72,6 @@ extern void run_tests (gdb::array_view<const char *const> filters,
/* Reset GDB or GDBserver's internal state. */
extern void reset ();
-
-using for_each_selftest_ftype
- = gdb::function_view<void(const std::string &name)>;
-
-/* Call FUNC for each registered selftest. */
-
-extern void for_each_selftest (for_each_selftest_ftype func);
}
/* Check that VALUE is true, and, if not, throw an exception. */
--
2.25.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] gdbsupport/selftest: Allow lazy registration
2022-04-01 13:00 [PATCH 0/3] Fix register_test_foreach_arch Lancelot SIX
2022-04-01 13:00 ` [PATCH 1/3] gdbsupport/selftest: Replace for_each_selftest with an iterator_range Lancelot SIX
@ 2022-04-01 13:00 ` Lancelot SIX
2022-04-01 13:00 ` [PATCH 3/3] gdb/selftest-arch: Make register_test_foreach_arch generate arch tests lazily Lancelot SIX
` (2 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Lancelot SIX @ 2022-04-01 13:00 UTC (permalink / raw)
To: gdb-patches; +Cc: lsix, Lancelot SIX
This patch adds a way to delay the registration of tests until the
latest possible moment. This is intended for situations where GDB needs
to be fully initialized in order to decide if a particular selftest can
be executed or not.
This mechanism will be used in the next patch.
Change-Id: I7f6b061f4c0a6832226c7080ab4e3a2523e1b0b0
---
gdbsupport/selftest.cc | 21 +++++++++++++++++++++
gdbsupport/selftest.h | 11 +++++++++++
2 files changed, 32 insertions(+)
diff --git a/gdbsupport/selftest.cc b/gdbsupport/selftest.cc
index 7077f113f97..1b862bb9ce3 100644
--- a/gdbsupport/selftest.cc
+++ b/gdbsupport/selftest.cc
@@ -30,6 +30,11 @@ namespace selftests
static selftests_registry tests;
+/* Set of callback functions used to register selftests after GDB is fully
+ initialized. */
+
+static std::vector<selftests_generator> lazy_generators;
+
/* See selftest.h. */
void
@@ -44,6 +49,14 @@ register_test (const std::string &name,
/* See selftest.h. */
+void
+add_lazy_generator (selftests_generator generator)
+{
+ lazy_generators.push_back (std::move (generator));
+}
+
+/* See selftest.h. */
+
static bool run_verbose_ = false;
/* See selftest.h. */
@@ -104,6 +117,14 @@ run_tests (gdb::array_view<const char *const> filters, bool verbose)
selftests_range
all_selftests ()
{
+ /* Execute any function which might still want to register tests. Once each
+ function has been executed, clear lazy_generators to ensure that
+ callback functions are only executed once. */
+ for (const auto &generator : lazy_generators)
+ for (selftest &test : generator ())
+ register_test (std::move (test.name), std::move (test.test));
+ lazy_generators.clear ();
+
return selftests_range (tests.cbegin (), tests.cend ());
}
diff --git a/gdbsupport/selftest.h b/gdbsupport/selftest.h
index 5ca9bdcc598..661431dbfca 100644
--- a/gdbsupport/selftest.h
+++ b/gdbsupport/selftest.h
@@ -23,6 +23,7 @@
#include "gdbsupport/function-view.h"
#include "gdbsupport/iterator-range.h"
#include <set>
+#include <vector>
/* A test is just a function that does some checks and throws an
exception if something has gone wrong. */
@@ -61,6 +62,16 @@ extern bool run_verbose ();
extern void register_test (const std::string &name,
std::function<void(void)> function);
+/* A selftest generator is a callback function used to delay the generation
+ of selftests. */
+
+using selftests_generator = std::function<std::vector<selftest> (void)>;
+
+/* Register a function which can lazily register selftests once GDB is fully
+ initialized. */
+
+extern void add_lazy_generator (selftests_generator generator);
+
/* Run all the self tests. This print a message describing the number
of test and the number of failures.
--
2.25.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] gdb/selftest-arch: Make register_test_foreach_arch generate arch tests lazily
2022-04-01 13:00 [PATCH 0/3] Fix register_test_foreach_arch Lancelot SIX
2022-04-01 13:00 ` [PATCH 1/3] gdbsupport/selftest: Replace for_each_selftest with an iterator_range Lancelot SIX
2022-04-01 13:00 ` [PATCH 2/3] gdbsupport/selftest: Allow lazy registration Lancelot SIX
@ 2022-04-01 13:00 ` Lancelot SIX
2022-04-15 17:52 ` Tom Tromey
2022-04-19 18:55 ` Simon Marchi
2022-04-12 8:17 ` [PING] [PATCH 0/3] Fix register_test_foreach_arch Lancelot SIX
2022-04-15 17:53 ` Tom Tromey
4 siblings, 2 replies; 12+ messages in thread
From: Lancelot SIX @ 2022-04-01 13:00 UTC (permalink / raw)
To: gdb-patches; +Cc: lsix, Lancelot SIX
The register_test_foreach_arch is used to instantiate a given selftest
for all architectures supported by GDB. It is used in many _initialize_*
functions (under initialize_all_files, called by gdb_init).
Because the call is done during GDB's initialization, and because there
is no guaranty about the order in which all the _initialize_* functions
are executed, when register_test_foreach_arch is called, GDB is not
fully initialized. Specifically, when a particular initialize function
is executed, only the architectures registered at that point are listed
by gdbarch_printable_names.
As a consequence, the list of selftest effectively executed depends on
the order the _initialize_* functions are called. This can be observed
with the following:
$ ./gdb/gdb \
-data-directory ./gdb/data-directory \
-quiet -batch -ex "maint selftest" 2>&1 \
| grep -E "Ran [0-9]+ unit tests"
Ran 145 unit tests, 0 failed
$ GDB_REVERSE_INIT_FUNCTIONS=1 ./gdb/gdb \
-data-directory ./gdb/data-directory \
-quiet -batch -ex "maint selftest" 2>&1 \
| grep -E "Ran [0-9]+ unit tests"
Ran 82 unit tests, 0 failed
To fix this, make register_test_foreach_arch register a lazy selftest
generator. This way when the test generator is eventually executed, all
architectures are registered and we do not have a dependency on the
order the initialize functions are executed in.
Tested on x86_64-linux
Change-Id: I88eefebf7d372ad672f42d3a103e89354bc8a925
---
gdb/selftest-arch.c | 29 +++++++++++++++++++++--------
gdb/selftest-arch.h | 3 +++
gdb/testsuite/gdb.gdb/unittest.exp | 25 +++++++++++++++++++++++--
3 files changed, 47 insertions(+), 10 deletions(-)
diff --git a/gdb/selftest-arch.c b/gdb/selftest-arch.c
index 1375838b0c2..f434da718d5 100644
--- a/gdb/selftest-arch.c
+++ b/gdb/selftest-arch.c
@@ -49,14 +49,15 @@ static bool skip_arch (const char *arch)
return false;
}
-/* Register a kind of selftest that calls the test function once for each
- gdbarch known to GDB. */
+/* Generate a selftest for each gdbarch known to GDB. */
-void
-register_test_foreach_arch (const std::string &name,
- self_test_foreach_arch_function *function)
+static std::vector<selftest>
+foreach_arch_test_generator (const std::string &name,
+ self_test_foreach_arch_function *function)
{
+ std::vector<selftest> tests;
std::vector<const char *> arches = gdbarch_printable_names ();
+ tests.reserve (arches.size ());
for (const char *arch : arches)
{
if (skip_arch (arch))
@@ -73,10 +74,22 @@ register_test_foreach_arch (const std::string &name,
reset ();
});
- std::string test_name
- = name + std::string ("::") + std::string (arch);
- register_test (test_name, test_fn);
+ tests.emplace_back (string_printf ("%s::%s", name.c_str (), arch),
+ test_fn);
}
+ return tests;
+}
+
+/* See selftest-arch.h. */
+
+void
+register_test_foreach_arch (const std::string &name,
+ self_test_foreach_arch_function *function)
+{
+ add_lazy_generator ([=] ()
+ {
+ return foreach_arch_test_generator (name, function);
+ });
}
void
diff --git a/gdb/selftest-arch.h b/gdb/selftest-arch.h
index f359e4a0ed9..6bdef267c44 100644
--- a/gdb/selftest-arch.h
+++ b/gdb/selftest-arch.h
@@ -23,6 +23,9 @@ typedef void self_test_foreach_arch_function (struct gdbarch *);
namespace selftests
{
+
+/* Register a selftest running FUNCTION for each arch supported by GDB. */
+
extern void
register_test_foreach_arch (const std::string &name,
self_test_foreach_arch_function *function);
diff --git a/gdb/testsuite/gdb.gdb/unittest.exp b/gdb/testsuite/gdb.gdb/unittest.exp
index 46583cd3504..2967b994cc3 100644
--- a/gdb/testsuite/gdb.gdb/unittest.exp
+++ b/gdb/testsuite/gdb.gdb/unittest.exp
@@ -58,11 +58,12 @@ proc run_selftests { binfile } {
}
-re "Selftests have been disabled for this build.\r\n$gdb_prompt $" {
unsupported $test
+ set num_ran 0
set enabled 0
}
}
- return $enabled
+ return [list $enabled $num_ran]
}
# Test completion of command "maintenance selftest".
@@ -86,7 +87,27 @@ proc_with_prefix test_completion {} {
}
with_test_prefix "no executable loaded" {
- set self_tests_enabled [run_selftests ""]
+ set res [run_selftests ""]
+ set self_tests_enabled [lindex $res 0]
+ set num_ran [lindex $res 1]
+}
+
+if { $self_tests_enabled && ![is_remote host] } {
+ # Check that we have the same amount of selftests whatever the
+ # initialization order of GDB.
+ with_test_prefix "reversed initialization" {
+ save_vars { env(GDB_REVERSE_INIT_FUNCTIONS) } {
+ if [info exists env(GDB_REVERSE_INIT_FUNCTIONS)] {
+ unset env(GDB_REVERSE_INIT_FUNCTIONS)
+ } else {
+ set env(GDB_REVERSE_INIT_FUNCTIONS) 1
+ }
+
+ set res [run_selftests ""]
+ gdb_assert "$num_ran == [lindex $res 1]" \
+ "selftest not dependent on initialization order"
+ }
+ }
}
with_test_prefix "executable loaded" {
--
2.25.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] gdb/selftest-arch: Make register_test_foreach_arch generate arch tests lazily
2022-04-01 13:00 ` [PATCH 3/3] gdb/selftest-arch: Make register_test_foreach_arch generate arch tests lazily Lancelot SIX
@ 2022-04-15 17:52 ` Tom Tromey
2022-04-15 18:10 ` Lancelot SIX
2022-04-19 18:55 ` Simon Marchi
1 sibling, 1 reply; 12+ messages in thread
From: Tom Tromey @ 2022-04-15 17:52 UTC (permalink / raw)
To: Lancelot SIX via Gdb-patches; +Cc: Lancelot SIX, lsix
>>>>> "Lancelot" == Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org> writes:
Lancelot> To fix this, make register_test_foreach_arch register a lazy selftest
Lancelot> generator. This way when the test generator is eventually executed, all
Lancelot> architectures are registered and we do not have a dependency on the
Lancelot> order the initialize functions are executed in.
Lancelot> -void
Lancelot> -register_test_foreach_arch (const std::string &name,
Lancelot> - self_test_foreach_arch_function *function)
Lancelot> +static std::vector<selftest>
Lancelot> +foreach_arch_test_generator (const std::string &name,
Lancelot> + self_test_foreach_arch_function *function)
Lancelot> {
Is there some deep reason this has to return a vector?
It seems like the old code, just calling register_test directly here,
would also have been fine.
Tom
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] gdb/selftest-arch: Make register_test_foreach_arch generate arch tests lazily
2022-04-15 17:52 ` Tom Tromey
@ 2022-04-15 18:10 ` Lancelot SIX
2022-04-15 18:50 ` Tom Tromey
0 siblings, 1 reply; 12+ messages in thread
From: Lancelot SIX @ 2022-04-15 18:10 UTC (permalink / raw)
To: Tom Tromey, Lancelot SIX via Gdb-patches; +Cc: lsix
On 15/04/2022 18:52, Tom Tromey wrote:
> [CAUTION: External Email]
>
>>>>>> "Lancelot" == Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Lancelot> To fix this, make register_test_foreach_arch register a lazy selftest
> Lancelot> generator. This way when the test generator is eventually executed, all
> Lancelot> architectures are registered and we do not have a dependency on the
> Lancelot> order the initialize functions are executed in.
>
> Lancelot> -void
> Lancelot> -register_test_foreach_arch (const std::string &name,
> Lancelot> - self_test_foreach_arch_function *function)
> Lancelot> +static std::vector<selftest>
> Lancelot> +foreach_arch_test_generator (const std::string &name,
> Lancelot> + self_test_foreach_arch_function *function)
> Lancelot> {
>
> Is there some deep reason this has to return a vector?
> It seems like the old code, just calling register_test directly here,
> would also have been fine.
>
> Tom
Hi,
The reason I went for this is to make the interface clearer about what
it does.
In the current implementation, I have (in selftest.h):
```
using selftests_generator = std::function<std::vector<selftest> (void)>;
extern void add_lazy_generator (selftests_generator generator);
```
My initial implementation did call `register_test` directly in the
callback, but then selftest_generator is "just":
```
using selftest_generator = std::function<void (void)>
```
which in my opinion does not communicate much about its intent or use.
Sure the vector is an overhead, but this machinery is really not on hot
code path so I did not consider this an issue.
That being said, I am OK with calls `register_test` directly in the
callback, depending on what the feedbacks are.
Best,
Lancelot.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] gdb/selftest-arch: Make register_test_foreach_arch generate arch tests lazily
2022-04-15 18:10 ` Lancelot SIX
@ 2022-04-15 18:50 ` Tom Tromey
0 siblings, 0 replies; 12+ messages in thread
From: Tom Tromey @ 2022-04-15 18:50 UTC (permalink / raw)
To: Lancelot SIX via Gdb-patches; +Cc: Tom Tromey, Lancelot SIX, lsix
>>>>> "Lancelot" == Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org> writes:
Lancelot> which in my opinion does not communicate much about its intent or
Lancelot> use. Sure the vector is an overhead, but this machinery is really not
Lancelot> on hot code path so I did not consider this an issue.
Seems fine to me. Thanks.
I think this is ok.
Tom
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] gdb/selftest-arch: Make register_test_foreach_arch generate arch tests lazily
2022-04-01 13:00 ` [PATCH 3/3] gdb/selftest-arch: Make register_test_foreach_arch generate arch tests lazily Lancelot SIX
2022-04-15 17:52 ` Tom Tromey
@ 2022-04-19 18:55 ` Simon Marchi
2022-04-19 21:10 ` Lancelot SIX
1 sibling, 1 reply; 12+ messages in thread
From: Simon Marchi @ 2022-04-19 18:55 UTC (permalink / raw)
To: Lancelot SIX, gdb-patches; +Cc: lsix
On 2022-04-01 09:00, Lancelot SIX via Gdb-patches wrote:
> The register_test_foreach_arch is used to instantiate a given selftest
> for all architectures supported by GDB. It is used in many _initialize_*
> functions (under initialize_all_files, called by gdb_init).
>
> Because the call is done during GDB's initialization, and because there
> is no guaranty about the order in which all the _initialize_* functions
> are executed, when register_test_foreach_arch is called, GDB is not
> fully initialized. Specifically, when a particular initialize function
> is executed, only the architectures registered at that point are listed
> by gdbarch_printable_names.
>
> As a consequence, the list of selftest effectively executed depends on
> the order the _initialize_* functions are called. This can be observed
> with the following:
>
> $ ./gdb/gdb \
> -data-directory ./gdb/data-directory \
> -quiet -batch -ex "maint selftest" 2>&1 \
> | grep -E "Ran [0-9]+ unit tests"
> Ran 145 unit tests, 0 failed
> $ GDB_REVERSE_INIT_FUNCTIONS=1 ./gdb/gdb \
> -data-directory ./gdb/data-directory \
> -quiet -batch -ex "maint selftest" 2>&1 \
> | grep -E "Ran [0-9]+ unit tests"
> Ran 82 unit tests, 0 failed
>
> To fix this, make register_test_foreach_arch register a lazy selftest
> generator. This way when the test generator is eventually executed, all
> architectures are registered and we do not have a dependency on the
> order the initialize functions are executed in.
>
> Tested on x86_64-linux
Hi Lancelot,
I see this new failure, when testing with the native-extended-gdbserver
board:
FAIL: gdb.gdb/unittest.exp: reversed initialization: maintenance selftest, failed none
Note that these two failures were already present before your patch,
with the same board:
FAIL: gdb.gdb/unittest.exp: no executable loaded: maintenance selftest, failed none
FAIL: gdb.gdb/unittest.exp: executable loaded: maintenance selftest, failed none
Simon
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] gdb/selftest-arch: Make register_test_foreach_arch generate arch tests lazily
2022-04-19 18:55 ` Simon Marchi
@ 2022-04-19 21:10 ` Lancelot SIX
0 siblings, 0 replies; 12+ messages in thread
From: Lancelot SIX @ 2022-04-19 21:10 UTC (permalink / raw)
To: Simon Marchi; +Cc: Lancelot SIX, gdb-patches
On Tue, Apr 19, 2022 at 02:55:19PM -0400, Simon Marchi wrote:
> On 2022-04-01 09:00, Lancelot SIX via Gdb-patches wrote:
> > The register_test_foreach_arch is used to instantiate a given selftest
> > for all architectures supported by GDB. It is used in many _initialize_*
> > functions (under initialize_all_files, called by gdb_init).
> >
> > Because the call is done during GDB's initialization, and because there
> > is no guaranty about the order in which all the _initialize_* functions
> > are executed, when register_test_foreach_arch is called, GDB is not
> > fully initialized. Specifically, when a particular initialize function
> > is executed, only the architectures registered at that point are listed
> > by gdbarch_printable_names.
> >
> > As a consequence, the list of selftest effectively executed depends on
> > the order the _initialize_* functions are called. This can be observed
> > with the following:
> >
> > $ ./gdb/gdb \
> > -data-directory ./gdb/data-directory \
> > -quiet -batch -ex "maint selftest" 2>&1 \
> > | grep -E "Ran [0-9]+ unit tests"
> > Ran 145 unit tests, 0 failed
> > $ GDB_REVERSE_INIT_FUNCTIONS=1 ./gdb/gdb \
> > -data-directory ./gdb/data-directory \
> > -quiet -batch -ex "maint selftest" 2>&1 \
> > | grep -E "Ran [0-9]+ unit tests"
> > Ran 82 unit tests, 0 failed
> >
> > To fix this, make register_test_foreach_arch register a lazy selftest
> > generator. This way when the test generator is eventually executed, all
> > architectures are registered and we do not have a dependency on the
> > order the initialize functions are executed in.
> >
> > Tested on x86_64-linux
>
> Hi Lancelot,
>
> I see this new failure, when testing with the native-extended-gdbserver
> board:
>
> FAIL: gdb.gdb/unittest.exp: reversed initialization: maintenance selftest, failed none
>
> Note that these two failures were already present before your patch,
> with the same board:
>
> FAIL: gdb.gdb/unittest.exp: no executable loaded: maintenance selftest, failed none
> FAIL: gdb.gdb/unittest.exp: executable loaded: maintenance selftest, failed none
>
> Simon
Hi,
Thanks for letting me know. From the errors you show I am tempted to
think that the new failure is just the same as the previous ones.
Given that I run the selftests one more time (with gdb launched with
GDB_REVERSE_INIT_FUNCTIONS=1), if the testsuite was unable to launch
selftests before, it makes sence that this new execution produces a new
erroer.
I’ll still try to have a look to check if we can have a good condition
to skip those tests, or at least fail only once and skip the next
similar tests.
Best,
Lancelot.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PING] [PATCH 0/3] Fix register_test_foreach_arch
2022-04-01 13:00 [PATCH 0/3] Fix register_test_foreach_arch Lancelot SIX
` (2 preceding siblings ...)
2022-04-01 13:00 ` [PATCH 3/3] gdb/selftest-arch: Make register_test_foreach_arch generate arch tests lazily Lancelot SIX
@ 2022-04-12 8:17 ` Lancelot SIX
2022-04-15 17:53 ` Tom Tromey
4 siblings, 0 replies; 12+ messages in thread
From: Lancelot SIX @ 2022-04-12 8:17 UTC (permalink / raw)
To: gdb-patches; +Cc: lsix
Hi,
Kindly pinging.
Best,
Lancelot.
On 01/04/2022 14:00, Lancelot SIX wrote:
> Hi,
>
> While working on a selftest issue, I noticed that there is a silent
> problem with register_test_foreach_arch:
>
> $ ./gdb/gdb \
> -data-directory ./gdb/data-directory \
> -quiet -batch -ex "maint selftest" 2>&1 \
> | grep -E "Ran [0-9]+ unit tests"
> Ran 145 unit tests, 0 failed
> $ GDB_REVERSE_INIT_FUNCTIONS=1 ./gdb/gdb \
> -data-directory ./gdb/data-directory \
> -quiet -batch -ex "maint selftest" 2>&1 \
> | grep -E "Ran [0-9]+ unit tests"
> Ran 82 unit tests, 0 failed
>
> Notice that when the GDB_REVERSE_INIT_FUNCTIONS env variable is set, the
> number of tests registered is different compared to when this env
> variable is not set.
>
> This is due to an issue with how register_test_foreach_arch is used and
> how it works. This function is used to register a generic selftest and
> run it against each architecture GDB supports. Such registration is
> typically done during GDB's initialization (in _initialize_* functions).
> At the point where this function is called, it will iterate over all the
> architectures known to GDB and create an instance of the selftest for
> it. The problem is that at that point, because GDB's initialization
> is not done yet, the best register_test_foreach_arch can do is register
> the test for each architecture initialized so far. All architectures
> which are initialized at a later point will not have the selftest.
>
> To fix this, this series proposes a lazy registration mechanism so
> register_test_foreach_arch can delay the moment it iterates over the
> known architectures to a point when GDB is fully initialized.
>
> The first patch of the series does some preparatory refactoring which
> will come in handy later. Patch 2 introduces the lazy test generation
> mechanism in the selftest framework, and patch 3 uses this mechanism to
> fix register_test_foreach_arch.
>
> Tested on x86_64-gnu-linux.
>
> All feedbacks are welcome.
>
> Best,
> Lancelot.
>
> Lancelot SIX (3):
> gdbsupport/selftest: Replace for_each_selftest with an iterator_range
> gdbsupport/selftest: Allow lazy registration
> gdb/selftest-arch: Make register_test_foreach_arch generate arch tests
> lazily
>
> gdb/maint.c | 13 ++++-----
> gdb/selftest-arch.c | 29 +++++++++++++------
> gdb/selftest-arch.h | 3 ++
> gdb/testsuite/gdb.gdb/unittest.exp | 25 ++++++++++++++--
> gdbsupport/selftest.cc | 46 +++++++++++++++++++++---------
> gdbsupport/selftest.h | 42 ++++++++++++++++++++++-----
> 6 files changed, 120 insertions(+), 38 deletions(-)
>
>
> base-commit: 5530c021ce01abf368a9cde26b22c4b34f320ee8
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] Fix register_test_foreach_arch
2022-04-01 13:00 [PATCH 0/3] Fix register_test_foreach_arch Lancelot SIX
` (3 preceding siblings ...)
2022-04-12 8:17 ` [PING] [PATCH 0/3] Fix register_test_foreach_arch Lancelot SIX
@ 2022-04-15 17:53 ` Tom Tromey
2022-04-19 8:18 ` Lancelot SIX
4 siblings, 1 reply; 12+ messages in thread
From: Tom Tromey @ 2022-04-15 17:53 UTC (permalink / raw)
To: Lancelot SIX via Gdb-patches; +Cc: Lancelot SIX, lsix
>>>>> "Lancelot" == Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org> writes:
Lancelot> The first patch of the series does some preparatory refactoring which
Lancelot> will come in handy later. Patch 2 introduces the lazy test generation
Lancelot> mechanism in the selftest framework, and patch 3 uses this mechanism to
Lancelot> fix register_test_foreach_arch.
I read through these. I had a question for #3 but otherwise it all
looks fine to me. Thank you.
Tom
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] Fix register_test_foreach_arch
2022-04-15 17:53 ` Tom Tromey
@ 2022-04-19 8:18 ` Lancelot SIX
0 siblings, 0 replies; 12+ messages in thread
From: Lancelot SIX @ 2022-04-19 8:18 UTC (permalink / raw)
To: Tom Tromey, Lancelot SIX via Gdb-patches; +Cc: lsix
On 15/04/2022 18:53, Tom Tromey wrote:
> [CAUTION: External Email]
>
>>>>>> "Lancelot" == Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Lancelot> The first patch of the series does some preparatory refactoring which
> Lancelot> will come in handy later. Patch 2 introduces the lazy test generation
> Lancelot> mechanism in the selftest framework, and patch 3 uses this mechanism to
> Lancelot> fix register_test_foreach_arch.
>
> I read through these. I had a question for #3 but otherwise it all
> looks fine to me. Thank you.
>
> Tom
Thanks,
I just pushed those 3 patches.
Best,
Lancelot.
^ permalink raw reply [flat|nested] 12+ messages in thread