[PATCH 0/3] Fix register_test_foreach_arch

[PATCH 0/3] Fix register_test_foreach_arch

[Lancelot SIX]

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

[PATCH 1/3] gdbsupport/selftest: Replace for_each_selftest with an iterator_range

[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

[PATCH 2/3] gdbsupport/selftest: Allow lazy registration

[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

[PATCH 3/3] gdb/selftest-arch: Make register_test_foreach_arch generate arch tests lazily

[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

[PING] [PATCH 0/3] Fix register_test_foreach_arch

[Lancelot SIX]

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

Re: [PATCH 3/3] gdb/selftest-arch: Make register_test_foreach_arch generate arch tests lazily

[Tom Tromey]

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

Re: [PATCH 0/3] Fix register_test_foreach_arch

[Tom Tromey]

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

Re: [PATCH 3/3] gdb/selftest-arch: Make register_test_foreach_arch generate arch tests lazily

[Lancelot SIX]

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.

Re: [PATCH 3/3] gdb/selftest-arch: Make register_test_foreach_arch generate arch tests lazily

[Tom Tromey]

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

Re: [PATCH 0/3] Fix register_test_foreach_arch

[Lancelot SIX]

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.

Re: [PATCH 3/3] gdb/selftest-arch: Make register_test_foreach_arch generate arch tests lazily

[Simon Marchi]

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

Re: [PATCH 3/3] gdb/selftest-arch: Make register_test_foreach_arch generate arch tests lazily

[Lancelot SIX]

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.