[PATCH] gdb: allow specifying multiple filters when running selftests

[PATCH] gdb: allow specifying multiple filters when running selftests

[Simon Marchi]

I found myself wanting to run a few specific selftests while developing.
I thought it would be nice to be able to provide multiple test names
when running `maintenant selftests`.  The arguments to that command is
currently interpreted as a single filter (not split by spaces), it now
becomes a list a filters, split by spaces.  A test is executed when it
matches at least one filter.

Here's an example of the result in GDB:

    (gdb) maintenance selftest xml
    Running selftest xml_escape_text.
    Running selftest xml_escape_text_append.
    Ran 2 unit tests, 0 failed
    (gdb) maintenance selftest xml unord
    Running selftest unordered_remove.
    Running selftest xml_escape_text.
    Running selftest xml_escape_text_append.
    Ran 3 unit tests, 0 failed
    (gdb) maintenance selftest xml unord foobar
    Running selftest unordered_remove.
    Running selftest xml_escape_text.
    Running selftest xml_escape_text_append.
    Ran 3 unit tests, 0 failed

Since the selftest machinery is also shared with gdbserver, I also
adapted gdbserver.  It accepts a `--selftest` switch, which accepts an
optional filter argument.  I made it so you can now pass `--selftest`
multiple time to add filters.

It's not so useful right now though: there's only a single selftest
right now in GDB and it's for an architecture I can't compile.  So I
tested by adding dummy tests, here's an example of the result:

    $ ./gdbserver --selftest=foo
    Running selftest foo.
    foo
    Running selftest foobar.
    foobar
    Ran 2 unit tests, 0 failed
    $ ./gdbserver --selftest=foo --selftest=bar
    Running selftest bar.
    bar
    Running selftest foo.
    foo
    Running selftest foobar.
    foobar
    Ran 3 unit tests, 0 failed

gdbsupport/ChangeLog:

	* selftest.h (run_tests): Change parameter to array_view.
	* selftest.c (run_tests): Change parameter to array_view and use
	it.

gdb/ChangeLog:

	* maint.c (maintenance_selftest): Split args and pass array_view
	to run_tests.

gdbserver/ChangeLog:

	* server.cc (captured_main): Accept multiple `--selftest=`
	options.  Pass all `--selftest=` arguments to run_tests.

Change-Id: I422bd49f08ea8095ae174c5d66a2dd502a59613a
---
 gdb/maint.c            |  3 ++-
 gdbserver/server.cc    | 14 +++++++++++---
 gdbsupport/selftest.cc | 17 ++++++++++++++---
 gdbsupport/selftest.h  |  8 +++++---
 4 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/gdb/maint.c b/gdb/maint.c
index b4890c34cab9..fd37acce5226 100644
--- a/gdb/maint.c
+++ b/gdb/maint.c
@@ -1041,7 +1041,8 @@ static void
 maintenance_selftest (const char *args, int from_tty)
 {
 #if GDB_SELF_TEST
-  selftests::run_tests (args);
+  gdb_argv argv (args);
+  selftests::run_tests (gdb::array_view<char *> (argv.get (), argv.count ()));
 #else
   printf_filtered (_("\
 Selftests have been disabled for this build.\n"));
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index aadcb9b5d30d..d45154d1f547 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -3610,7 +3610,7 @@ captured_main (int argc, char *argv[])
   int was_running;
   bool selftest = false;
 #if GDB_SELF_TEST
-  const char *selftest_filter = NULL;
+  std::vector<const char *> selftest_filters;
 #endif
 
   current_directory = getcwd (NULL, 0);
@@ -3747,8 +3747,16 @@ captured_main (int argc, char *argv[])
       else if (startswith (*next_arg, "--selftest="))
 	{
 	  selftest = true;
+
 #if GDB_SELF_TEST
-	  selftest_filter = *next_arg + strlen ("--selftest=");
+	  const char *filter = *next_arg + strlen ("--selftest=");
+	  if (*filter == '\0')
+	    {
+	      fprintf (stderr, _("Error: selftest filter is empty.\n"));
+	      exit (1);
+	    }
+
+	  selftest_filters.push_back (filter);
 #endif
 	}
       else
@@ -3825,7 +3833,7 @@ captured_main (int argc, char *argv[])
   if (selftest)
     {
 #if GDB_SELF_TEST
-      selftests::run_tests (selftest_filter);
+      selftests::run_tests (selftest_filters);
 #else
       printf (_("Selftests have been disabled for this build.\n"));
 #endif
diff --git a/gdbsupport/selftest.cc b/gdbsupport/selftest.cc
index 8ab63be31391..2adb7245571c 100644
--- a/gdbsupport/selftest.cc
+++ b/gdbsupport/selftest.cc
@@ -68,7 +68,7 @@ register_test (const std::string &name, self_test_function *function)
 /* See selftest.h.  */
 
 void
-run_tests (const char *filter)
+run_tests (gdb::array_view<const char *const> filters)
 {
   int ran = 0, failed = 0;
 
@@ -76,9 +76,20 @@ run_tests (const char *filter)
     {
       const std::string &name = pair.first;
       const std::unique_ptr<selftest> &test = pair.second;
+      bool run = false;
 
-      if (filter != NULL && *filter != '\0'
-	  && name.find (filter) == std::string::npos)
+      if (filters.empty ())
+	run = true;
+      else
+	{
+	  for (const char *filter : filters)
+	    {
+	      if (name.find (filter) != std::string::npos)
+		run = true;
+	    }
+	}
+
+      if (!run)
 	continue;
 
       try
diff --git a/gdbsupport/selftest.h b/gdbsupport/selftest.h
index 1c47fe1448ba..e0086132f7b9 100644
--- a/gdbsupport/selftest.h
+++ b/gdbsupport/selftest.h
@@ -19,6 +19,8 @@
 #ifndef COMMON_SELFTEST_H
 #define COMMON_SELFTEST_H
 
+#include "gdbsupport/array-view.h"
+
 /* A test is just a function that does some checks and throws an
    exception if something has gone wrong.  */
 
@@ -47,10 +49,10 @@ extern void register_test (const std::string &name,
 /* Run all the self tests.  This print a message describing the number
    of test and the number of failures.
 
-   If FILTER is not NULL and not empty, only tests with names containing FILTER
-   will be ran.  */
+   If FILTERS is not empty, only run tests with names containing one of the
+   element of FILTERS.  */
 
-extern void run_tests (const char *filter);
+extern void run_tests (gdb::array_view<const char *const> filters);
 
 /* Reset GDB or GDBserver's internal state.  */
 extern void reset ();
-- 
2.28.0

Re: [PATCH] gdb: allow specifying multiple filters when running selftests

[Kevin Buettner]

On Wed, 12 Aug 2020 18:12:45 -0400
Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> wrote:

> I found myself wanting to run a few specific selftests while developing.
> I thought it would be nice to be able to provide multiple test names
> when running `maintenant selftests`.  The arguments to that command is
> currently interpreted as a single filter (not split by spaces), it now
> becomes a list a filters, split by spaces.  A test is executed when it
> matches at least one filter.
> 
> Here's an example of the result in GDB:
> 
>     (gdb) maintenance selftest xml
>     Running selftest xml_escape_text.
>     Running selftest xml_escape_text_append.
>     Ran 2 unit tests, 0 failed
>     (gdb) maintenance selftest xml unord
>     Running selftest unordered_remove.
>     Running selftest xml_escape_text.
>     Running selftest xml_escape_text_append.
>     Ran 3 unit tests, 0 failed
>     (gdb) maintenance selftest xml unord foobar
>     Running selftest unordered_remove.
>     Running selftest xml_escape_text.
>     Running selftest xml_escape_text_append.
>     Ran 3 unit tests, 0 failed
> 
> Since the selftest machinery is also shared with gdbserver, I also
> adapted gdbserver.  It accepts a `--selftest` switch, which accepts an
> optional filter argument.  I made it so you can now pass `--selftest`
> multiple time to add filters.
> 
> It's not so useful right now though: there's only a single selftest
> right now in GDB and it's for an architecture I can't compile.  So I
> tested by adding dummy tests, here's an example of the result:
> 
>     $ ./gdbserver --selftest=foo
>     Running selftest foo.
>     foo
>     Running selftest foobar.
>     foobar
>     Ran 2 unit tests, 0 failed
>     $ ./gdbserver --selftest=foo --selftest=bar
>     Running selftest bar.
>     bar
>     Running selftest foo.
>     foo
>     Running selftest foobar.
>     foobar
>     Ran 3 unit tests, 0 failed
> 
> gdbsupport/ChangeLog:
> 
> 	* selftest.h (run_tests): Change parameter to array_view.
> 	* selftest.c (run_tests): Change parameter to array_view and use
> 	it.
> 
> gdb/ChangeLog:
> 
> 	* maint.c (maintenance_selftest): Split args and pass array_view
> 	to run_tests.
> 
> gdbserver/ChangeLog:
> 
> 	* server.cc (captured_main): Accept multiple `--selftest=`
> 	options.  Pass all `--selftest=` arguments to run_tests.

LGTM.

Kevin

Re: [PATCH] gdb: allow specifying multiple filters when running selftests

[Simon Marchi]

On 2020-08-12 9:46 p.m., Kevin Buettner wrote:
> LGTM.
> 
> Kevin

Thanks, I pushed it.

Simon

Re: [PATCH] gdb: allow specifying multiple filters when running selftests

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> +  selftests::run_tests (gdb::array_view<char *> (argv.get (), argv.count ()));

I think it would be good to add a method to gdb_argv that returns an
array view.

Tom

Re: [PATCH] gdb: allow specifying multiple filters when running selftests

[Simon Marchi]

On 2020-08-13 10:06 a.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> +  selftests::run_tests (gdb::array_view<char *> (argv.get (), argv.count ()));
> 
> I think it would be good to add a method to gdb_argv that returns an
> array view.

Hmm do you think it will happen often enough that we pass the parsed arguments straight to a
function taking an array view?

In any case, it's not difficult to do.  Did you mean a simple method like the patch below, or
a conversion operator that could be implemented like this, to be able to transparently pass
a gdb_argv as an array_view?

  using char_ptr_array_view = gdb::array_view<char *>;
  operator char_ptr_array_view()
  {
    return gdb::array_view<char *> (this->get (), this->count ());
  }

Personally I prefer things to be a bit more explicit, otherwise there's a bit too much hidden
magic.


From 79a9b29c1f08382ac790f2661fff55eade4d0174 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Thu, 13 Aug 2020 10:28:41 -0400
Subject: [PATCH] gdb: add gdb_argv::as_array_view method

Change-Id: I0645037613ed6549aabe60f14a36f3494513b177
---
 gdb/maint.c |  2 +-
 gdb/utils.c | 26 ++++++++++++++++++++++++++
 gdb/utils.h |  8 ++++++++
 3 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/gdb/maint.c b/gdb/maint.c
index fd37acce5226..3368769ad96f 100644
--- a/gdb/maint.c
+++ b/gdb/maint.c
@@ -1042,7 +1042,7 @@ maintenance_selftest (const char *args, int from_tty)
 {
 #if GDB_SELF_TEST
   gdb_argv argv (args);
-  selftests::run_tests (gdb::array_view<char *> (argv.get (), argv.count ()));
+  selftests::run_tests (argv.as_array_view ());
 #else
   printf_filtered (_("\
 Selftests have been disabled for this build.\n"));
diff --git a/gdb/utils.c b/gdb/utils.c
index 102db28787fb..fb1308ac9ae3 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -2991,6 +2991,31 @@ gdb_realpath_tests ()
   gdb_realpath_check_trailer ("", "");
 }

+/* Test the gdb_argv::as_array_view method.  */
+
+static void
+gdb_argv_as_array_view_test ()
+{
+  {
+    gdb_argv argv;
+
+    gdb::array_view<char *> view = argv.as_array_view ();
+
+    SELF_CHECK (view.data () == nullptr);
+    SELF_CHECK (view.size () == 0);
+  }
+  {
+    gdb_argv argv ("une bonne 50");
+
+    gdb::array_view<char *> view = argv.as_array_view ();
+
+    SELF_CHECK (view.size () == 3);
+    SELF_CHECK (strcmp (view[0], "une") == 0);
+    SELF_CHECK (strcmp (view[1], "bonne") == 0);
+    SELF_CHECK (strcmp (view[2], "50") == 0);
+  }
+}
+
 #endif /* GDB_SELF_TEST */

 /* Allocation function for the libiberty hash table which uses an
@@ -3489,5 +3514,6 @@ When set, debugging messages will be marked with seconds and microseconds."),

 #if GDB_SELF_TEST
   selftests::register_test ("gdb_realpath", gdb_realpath_tests);
+  selftests::register_test ("gdb_argv_array_view", gdb_argv_as_array_view_test);
 #endif
 }
diff --git a/gdb/utils.h b/gdb/utils.h
index 3434ff1caa25..9a235b963272 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -22,6 +22,7 @@
 #define UTILS_H

 #include "exceptions.h"
+#include "gdbsupport/array-view.h"
 #include "gdbsupport/scoped_restore.h"
 #include <chrono>

@@ -210,6 +211,13 @@ class gdb_argv
     return m_argv[arg];
   }

+  /* Return the arguments array as an array view.  */
+
+  gdb::array_view<char *> as_array_view ()
+  {
+    return gdb::array_view<char *> (this->get (), this->count ());
+  }
+
   /* The iterator type.  */

   typedef char **iterator;
-- 
2.28.0

Re: [PATCH] gdb: allow specifying multiple filters when running selftests

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> Hmm do you think it will happen often enough that we pass the
Simon> parsed arguments straight to a function taking an array view?

I am not sure it will happen often.

Simon> In any case, it's not difficult to do.  Did you mean a simple
Simon> method like the patch below, or a conversion operator that could
Simon> be implemented like this, to be able to transparently pass a
Simon> gdb_argv as an array_view?

...
Simon> Personally I prefer things to be a bit more explicit, otherwise there's a bit too much hidden
Simon> magic.

Me too.  I view implicit conversions as bad by default, needing special
justification to be used.

Tom

[PATCH] gdb: add gdb_argv::as_array_view method

[Simon Marchi]

On 2020-08-13 12:12 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> Hmm do you think it will happen often enough that we pass the
> Simon> parsed arguments straight to a function taking an array view?
> 
> I am not sure it will happen often.
> 
> Simon> In any case, it's not difficult to do.  Did you mean a simple
> Simon> method like the patch below, or a conversion operator that could
> Simon> be implemented like this, to be able to transparently pass a
> Simon> gdb_argv as an array_view?
> 
> ...
> Simon> Personally I prefer things to be a bit more explicit, otherwise there's a bit too much hidden
> Simon> magic.
> 
> Me too.  I view implicit conversions as bad by default, needing special
> justification to be used.

All right, here's the same patch with a commit log and ChangeLog entry.


From fec96cb9a8a0b50d85697cb0f6783bc5319a167b Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Thu, 13 Aug 2020 10:28:41 -0400
Subject: [PATCH] gdb: add gdb_argv::as_array_view method

Introduce the gdb_argv::as_array_view method, as a way to easily pass
the parsed arguments array to a function taking an array view.  There is
currently one caller where we can use this (which prompted the
suggestion to implement this method).

Add some selftests for the new method, which at the same time test a
little bit gdb_argv.  As far as I know, it's not tested currently.

gdb/ChangeLog:

	* utils.h (class gdb_argv) <as_array_view>: New method.
	* utils.c (gdb_argv_as_array_view_test): New.
	(_initialize_utils): Register selftest.
	* maint.c (maintenance_selftest): Use the new method.

Change-Id: I0645037613ed6549aabe60f14a36f3494513b177
---
 gdb/maint.c |  2 +-
 gdb/utils.c | 26 ++++++++++++++++++++++++++
 gdb/utils.h |  8 ++++++++
 3 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/gdb/maint.c b/gdb/maint.c
index fd37acce5226..3368769ad96f 100644
--- a/gdb/maint.c
+++ b/gdb/maint.c
@@ -1042,7 +1042,7 @@ maintenance_selftest (const char *args, int from_tty)
 {
 #if GDB_SELF_TEST
   gdb_argv argv (args);
-  selftests::run_tests (gdb::array_view<char *> (argv.get (), argv.count ()));
+  selftests::run_tests (argv.as_array_view ());
 #else
   printf_filtered (_("\
 Selftests have been disabled for this build.\n"));
diff --git a/gdb/utils.c b/gdb/utils.c
index 102db28787fb..fb1308ac9ae3 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -2991,6 +2991,31 @@ gdb_realpath_tests ()
   gdb_realpath_check_trailer ("", "");
 }

+/* Test the gdb_argv::as_array_view method.  */
+
+static void
+gdb_argv_as_array_view_test ()
+{
+  {
+    gdb_argv argv;
+
+    gdb::array_view<char *> view = argv.as_array_view ();
+
+    SELF_CHECK (view.data () == nullptr);
+    SELF_CHECK (view.size () == 0);
+  }
+  {
+    gdb_argv argv ("une bonne 50");
+
+    gdb::array_view<char *> view = argv.as_array_view ();
+
+    SELF_CHECK (view.size () == 3);
+    SELF_CHECK (strcmp (view[0], "une") == 0);
+    SELF_CHECK (strcmp (view[1], "bonne") == 0);
+    SELF_CHECK (strcmp (view[2], "50") == 0);
+  }
+}
+
 #endif /* GDB_SELF_TEST */

 /* Allocation function for the libiberty hash table which uses an
@@ -3489,5 +3514,6 @@ When set, debugging messages will be marked with seconds and microseconds."),

 #if GDB_SELF_TEST
   selftests::register_test ("gdb_realpath", gdb_realpath_tests);
+  selftests::register_test ("gdb_argv_array_view", gdb_argv_as_array_view_test);
 #endif
 }
diff --git a/gdb/utils.h b/gdb/utils.h
index 3434ff1caa25..9a235b963272 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -22,6 +22,7 @@
 #define UTILS_H

 #include "exceptions.h"
+#include "gdbsupport/array-view.h"
 #include "gdbsupport/scoped_restore.h"
 #include <chrono>

@@ -210,6 +211,13 @@ class gdb_argv
     return m_argv[arg];
   }

+  /* Return the arguments array as an array view.  */
+
+  gdb::array_view<char *> as_array_view ()
+  {
+    return gdb::array_view<char *> (this->get (), this->count ());
+  }
+
   /* The iterator type.  */

   typedef char **iterator;
-- 
2.28.0

Re: [PATCH] gdb: add gdb_argv::as_array_view method

[Tom Tromey]

Simon> All right, here's the same patch with a commit log and ChangeLog
Simon> entry.

Looks good, thank you.

Tom

Re: [PATCH] gdb: add gdb_argv::as_array_view method

[Simon Marchi]

On 2020-08-14 11:33 a.m., Tom Tromey wrote:
> Simon> All right, here's the same patch with a commit log and ChangeLog
> Simon> entry.
> 
> Looks good, thank you.
> 
> Tom
> 

Thanks, I pushed it.

Simon