Add selftests run filtering
Commit Message
With the growing number of selftests, I think it would be useful to be
able to run only a subset of the tests. This patch associates a name to
each registered selftest. It then allows doing something like:
(gdb) maintenance selftest aarch64
Running self-tests.
Running selftest aarch64-analyze-prologue.
Running selftest aarch64-process-record.
Ran 2 unit tests, 0 failed
which will only run the tests that contain "aarch64" in their name. To
help validate that the tests you want to run were actually ran, it also
prints a message with the test name before running each test.
Right now, the arch-dependent tests are registered as a single test of
the selftests. To be able to filter those too, I made them completely
separate. run_tests and run_tests_with_arch now return their results,
and maintenance_selftest takes care of printing the combined results.
There aren't many selftests in gdbserver yet, but it wasn't too hard to
make it accept a filter too. It can be given with --selftest=FILTER.
gdb/ChangeLog:
* common/selftest.h (struct selftests_results): New struct.
(register_test): Add name parameter.
(run_tests): Add filter parameter.
* common/selftest.c (tests): Change type to std::map.
(register_test): Add name parameter and use it.
(run_tests): Add filter parameter and use it. Add prints.
Adjust to vector -> map change. Return results.
* aarch64-tdep.c (_initialize_aarch64_tdep): Add names when
registering selftests.
* arm-tdep.c (_initialize_arm_tdep): Likewise.
* disasm-selftests.c (_initialize_disasm_selftests): Likewise.
* dwarf2-frame.c (_initialize_dwarf2_frame): Likewise.
* dwarf2loc.c (_initialize_dwarf2loc): Likewise.
* findvar.c (_initialize_findvar): Likewise.
* gdbarch-selftests.c (_initialize_gdbarch_selftests): Likewise.
* maint.c: Include "selftest-arch.h".
(maintenance_selftest): Call run_tests_with_arch. Print
results.
* regcache.c (_initialize_regcache): Add names when registering
selftests.
* rust-exp.y (_initialize_rust_exp): Likewise.
* selftest-arch.c: Include <map>.
(gdbarch_tests): Change type to std::map.
(register_test_foreach_arch): Add name parameter and use it.
(run_tests_with_arch): Add filter parameter and use it. Add
prints. Adjust to vector -> map change. Return results.
(_initialize_selftests_foreach_arch): Remove.
* selftest-arch.h (register_test_foreach_arch): Add name
parameter.
(run_tests_with_arch): New declaration.
* utils-selftests.c (_initialize_utils_selftests): Add names
when registering selftests.
* utils.c (_initialize_utils): Likewise.
* unittests/array-view-selftests.c
(_initialize_array_view_selftests): Likewise.
* unittests/environ-selftests.c (_initialize_environ_selftests):
Likewise.
* unittests/function-view-selftests.c
(_initialize_function_view_selftests): Likewise.
* unittests/offset-type-selftests.c
(_initialize_offset_type_selftests): Likewise.
* unittests/optional-selftests.c
(_initialize_optional_selftests): Likewise.
* unittests/scoped_restore-selftests.c
(_initialize_scoped_restore_selftests): Likewise.
gdb/gdbserver/ChangeLog:
* server.c (captured_main): Accept argument for --selftest.
Update run_tests call, print test results.
* linux-x86-tdesc-selftest.c (initialize_low_tdesc): Add names
when registering selftests.
---
gdb/aarch64-tdep.c | 6 +++--
gdb/arm-tdep.c | 2 +-
gdb/common/selftest.c | 32 ++++++++++++++---------
gdb/common/selftest.h | 19 +++++++++++---
gdb/disasm-selftests.c | 6 +++--
gdb/dwarf2-frame.c | 3 ++-
gdb/dwarf2loc.c | 2 +-
gdb/findvar.c | 4 ++-
gdb/gdbarch-selftests.c | 3 ++-
gdb/gdbserver/linux-x86-tdesc-selftest.c | 4 +--
gdb/gdbserver/server.c | 15 ++++++++++-
gdb/maint.c | 8 +++++-
gdb/regcache.c | 3 ++-
gdb/rust-exp.y | 2 +-
gdb/selftest-arch.c | 44 +++++++++++++++++---------------
gdb/selftest-arch.h | 10 +++++++-
gdb/unittests/array-view-selftests.c | 3 ++-
gdb/unittests/environ-selftests.c | 3 ++-
gdb/unittests/function-view-selftests.c | 3 ++-
gdb/unittests/offset-type-selftests.c | 2 +-
gdb/unittests/optional-selftests.c | 2 +-
gdb/unittests/scoped_restore-selftests.c | 3 ++-
gdb/utils-selftests.c | 2 +-
gdb/utils.c | 2 +-
24 files changed, 122 insertions(+), 61 deletions(-)
Comments
Sounds useful.
Patch looks fine to me. Nits and comments below.
I wonder whether we'll want to be able to do select tests
with e.g., a regexp instead of exact matching. If we do, then
the std::vector->std::map change would seem pointless.
Do you plan on adding some command to list the existing tests?
On 09/05/2017 12:50 PM, Simon Marchi wrote:
> static void
> maintenance_selftest (char *args, int from_tty)
> {
> - selftests::run_tests ();
> + selftests::selftests_results results_noarch = selftests::run_tests (args);
> + selftests::selftests_results results_arch = selftests::run_tests_with_arch (args);
Line too long? You could also shorten "selftests_results"
to e.g., "test_results", since the type is already in the
"selftests" namespace.
> + printf_filtered (_("Ran %d unit tests, %d failed\n"),
> + results_noarch.ran + results_arch.ran,
> + results_noarch.failed + results_arch.failed);
Wonder whether it'd make sense to implement operator+= for
selftests_results so that you'd write:
selftests::selftests_results results = selftests::run_tests (args);
results += selftests::run_tests_with_arch (args);
// etc.
printf_filtered (_("Ran %d unit tests, %d failed\n"),
results.ran, results.failed);
> _initialize_findvar (void)
> {
> #if GDB_SELF_TEST
> - selftests::register_test (selftests::findvar_tests::copy_integer_to_size_test);
> + selftests::register_test (
> + "copy_integer_to_size",
> + selftests::findvar_tests::copy_integer_to_size_test);
> #endif
In GNU style it's much more common to break the line
before "(", not after.
Thanks,
Pedro Alves
On 2017-09-06 17:25, Pedro Alves wrote:
> Sounds useful.
>
> Patch looks fine to me. Nits and comments below.
>
> I wonder whether we'll want to be able to do select tests
> with e.g., a regexp instead of exact matching. If we do, then
> the std::vector->std::map change would seem pointless.
Not sure I understand. I am not using exact matching now either, so
using a map is probably already pointless. I am not sure what lead me
there, maybe I started with the intent of using exact matching. I'll
change it to a vector of a new struct type.
> Do you plan on adding some command to list the existing tests?
Yeah, why not. "maintenance info selftests"?
> On 09/05/2017 12:50 PM, Simon Marchi wrote:
>> static void
>> maintenance_selftest (char *args, int from_tty)
>> {
>> - selftests::run_tests ();
>> + selftests::selftests_results results_noarch = selftests::run_tests
>> (args);
>> + selftests::selftests_results results_arch =
>> selftests::run_tests_with_arch (args);
>
> Line too long? You could also shorten "selftests_results"
> to e.g., "test_results", since the type is already in the
> "selftests" namespace.
Done.
>> + printf_filtered (_("Ran %d unit tests, %d failed\n"),
>> + results_noarch.ran + results_arch.ran,
>> + results_noarch.failed + results_arch.failed);
>
> Wonder whether it'd make sense to implement operator+= for
> selftests_results so that you'd write:
>
> selftests::selftests_results results = selftests::run_tests (args);
> results += selftests::run_tests_with_arch (args);
> // etc.
>
> printf_filtered (_("Ran %d unit tests, %d failed\n"),
> results.ran, results.failed);
I thought it was a bit overkill for our needs, but on the other side
there's no good argument against it and it's cleaner. I'll do that.
>> _initialize_findvar (void)
>> {
>> #if GDB_SELF_TEST
>> - selftests::register_test
>> (selftests::findvar_tests::copy_integer_to_size_test);
>> + selftests::register_test (
>> + "copy_integer_to_size",
>> + selftests::findvar_tests::copy_integer_to_size_test);
>> #endif
>
> In GNU style it's much more common to break the line
> before "(", not after.
Ah ok thanks, I always have a doubt in that situation.
Simon
On 2017-09-06 20:37, Simon Marchi wrote:
> On 2017-09-06 17:25, Pedro Alves wrote:
>> Sounds useful.
>>
>> Patch looks fine to me. Nits and comments below.
>>
>> I wonder whether we'll want to be able to do select tests
>> with e.g., a regexp instead of exact matching. If we do, then
>> the std::vector->std::map change would seem pointless.
>
> Not sure I understand. I am not using exact matching now either, so
> using a map is probably already pointless. I am not sure what lead me
> there, maybe I started with the intent of using exact matching. I'll
> change it to a vector of a new struct type.
Ah, now I remember. It was so we could easily check that there isn't
already a test with this name, when registering a new test. It's won't
break anything if we happened to have two tests with the same name, so I
can just remove this check.
On 2017-09-06 20:41, Simon Marchi wrote:
> On 2017-09-06 20:37, Simon Marchi wrote:
>> On 2017-09-06 17:25, Pedro Alves wrote:
>>> Sounds useful.
>>>
>>> Patch looks fine to me. Nits and comments below.
>>>
>>> I wonder whether we'll want to be able to do select tests
>>> with e.g., a regexp instead of exact matching. If we do, then
>>> the std::vector->std::map change would seem pointless.
>>
>> Not sure I understand. I am not using exact matching now either, so
>> using a map is probably already pointless. I am not sure what lead me
>> there, maybe I started with the intent of using exact matching. I'll
>> change it to a vector of a new struct type.
>
> Ah, now I remember. It was so we could easily check that there isn't
> already a test with this name, when registering a new test. It's
> won't break anything if we happened to have two tests with the same
> name, so I can just remove this check.
Oh, and also so it would automatically iterate in alphabetical order, I
thought it was prettier :).
On 09/06/2017 07:43 PM, Simon Marchi wrote:
> On 2017-09-06 20:41, Simon Marchi wrote:
>> On 2017-09-06 20:37, Simon Marchi wrote:
>>> On 2017-09-06 17:25, Pedro Alves wrote:
>>>> Sounds useful.
>>>>
>>>> Patch looks fine to me. Nits and comments below.
>>>>
>>>> I wonder whether we'll want to be able to do select tests
>>>> with e.g., a regexp instead of exact matching. If we do, then
>>>> the std::vector->std::map change would seem pointless.
>>>
>>> Not sure I understand. I am not using exact matching now either, so
>>> using a map is probably already pointless. I am not sure what lead me
>>> there, maybe I started with the intent of using exact matching. I'll
>>> change it to a vector of a new struct type.
>>
>> Ah, now I remember. It was so we could easily check that there isn't
>> already a test with this name, when registering a new test. It's
>> won't break anything if we happened to have two tests with the same
>> name, so I can just remove this check.
>
> Oh, and also so it would automatically iterate in alphabetical order, I
> thought it was prettier :).
Ah, making sure order is stable makes sense. Might be worth it
of a comment.
Thanks,
Pedro Alves
@@ -3068,8 +3068,10 @@ When on, AArch64 specific debugging is enabled."),
&setdebuglist, &showdebuglist);
#if GDB_SELF_TEST
- selftests::register_test (selftests::aarch64_analyze_prologue_test);
- selftests::register_test (selftests::aarch64_process_record_test);
+ selftests::register_test ("aarch64-analyze-prologue",
+ selftests::aarch64_analyze_prologue_test);
+ selftests::register_test ("aarch64-process-record",
+ selftests::aarch64_process_record_test);
#endif
}
@@ -9727,7 +9727,7 @@ vfp - VFP co-processor."),
&setdebuglist, &showdebuglist);
#if GDB_SELF_TEST
- selftests::register_test (selftests::arm_record_test);
+ selftests::register_test ("arm-record", selftests::arm_record_test);
#endif
}
@@ -20,39 +20,48 @@
#include "common-exceptions.h"
#include "common-debug.h"
#include "selftest.h"
-#include <vector>
+#include <map>
namespace selftests
{
/* All the tests that have been registered. */
-static std::vector<self_test_function *> tests;
+static std::map<std::string, self_test_function *> tests;
/* See selftest.h. */
void
-register_test (self_test_function *function)
+register_test (const std::string &name, self_test_function *function)
{
- tests.push_back (function);
+ /* Make sure we don't already have a test with this name. */
+ gdb_assert (tests.find (name) == tests.end ());
+
+ tests[name] = function;
}
/* See selftest.h. */
-void
-run_tests (void)
+selftests_results
+run_tests (const char *filter)
{
- int failed = 0;
+ selftests_results results;
- for (int i = 0; i < tests.size (); ++i)
+ for (const auto &test : tests)
{
+ if (filter != NULL && *filter != '\0'
+ && test.first.find (filter) == std::string::npos)
+ continue;
+
TRY
{
- tests[i] ();
+ debug_printf (_("Running selftest %s.\n"), test.first.c_str ());
+ results.ran++;
+ test.second ();
}
CATCH (ex, RETURN_MASK_ERROR)
{
- ++failed;
+ results.failed++;
debug_printf ("Self test failed: %s\n", ex.message);
}
END_CATCH
@@ -60,7 +69,6 @@ run_tests (void)
reset ();
}
- debug_printf ("Ran %lu unit tests, %d failed\n",
- (long) tests.size (), failed);
+ return results;
}
} // namespace selftests
@@ -26,15 +26,26 @@ typedef void self_test_function (void);
namespace selftests
{
+struct selftests_results
+{
+ /* Number of test cases executed. */
+ int ran = 0;
+
+ /* Number of test cases that failed. */
+ int failed = 0;
+};
/* Register a new self-test. */
-extern void register_test (self_test_function *function);
+extern void register_test (const std::string &name,
+ self_test_function *function);
+
+/* Run all the architecture-agnostic self tests.
-/* 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. */
-extern void run_tests (void);
+extern selftests_results run_tests (const char *filter);
/* Reset GDB or GDBserver's internal state. */
extern void reset ();
@@ -214,7 +214,9 @@ void
_initialize_disasm_selftests (void)
{
#if GDB_SELF_TEST
- selftests::register_test_foreach_arch (selftests::print_one_insn_test);
- selftests::register_test_foreach_arch (selftests::memory_error_test);
+ selftests::register_test_foreach_arch ("print_one_insn",
+ selftests::print_one_insn_test);
+ selftests::register_test_foreach_arch ("memory_error",
+ selftests::memory_error_test);
#endif
}
@@ -2406,6 +2406,7 @@ _initialize_dwarf2_frame (void)
dwarf2_frame_objfile_data = register_objfile_data ();
#if GDB_SELF_TEST
- selftests::register_test_foreach_arch (selftests::execute_cfa_program_test);
+ selftests::register_test_foreach_arch ("execute_cfa_program",
+ selftests::execute_cfa_program_test);
#endif
}
@@ -4687,6 +4687,6 @@ _initialize_dwarf2loc (void)
&setdebuglist, &showdebuglist);
#if GDB_SELF_TEST
- selftests::register_test (selftests::copy_bitwise_tests);
+ selftests::register_test ("copy_bitwise", selftests::copy_bitwise_tests);
#endif
}
@@ -1095,6 +1095,8 @@ void
_initialize_findvar (void)
{
#if GDB_SELF_TEST
- selftests::register_test (selftests::findvar_tests::copy_integer_to_size_test);
+ selftests::register_test (
+ "copy_integer_to_size",
+ selftests::findvar_tests::copy_integer_to_size_test);
#endif
}
@@ -151,6 +151,7 @@ void
_initialize_gdbarch_selftests (void)
{
#if GDB_SELF_TEST
- selftests::register_test_foreach_arch (selftests::register_to_value_test);
+ selftests::register_test_foreach_arch ("register_to_value",
+ selftests::register_to_value_test);
#endif
}
@@ -164,7 +164,7 @@ initialize_low_tdesc ()
init_registers_i386_avx_avx512_linux ();
init_registers_i386_avx_mpx_avx512_pku_linux ();
- selftests::register_test (selftests::tdesc::i386_tdesc_test);
+ selftests::register_test ("i386-tdesc", selftests::tdesc::i386_tdesc_test);
#ifdef __x86_64__
init_registers_x32_linux ();
@@ -178,6 +178,6 @@ initialize_low_tdesc ()
init_registers_amd64_avx_avx512_linux ();
init_registers_amd64_avx_mpx_avx512_pku_linux ();
- selftests::register_test (selftests::tdesc::amd64_tdesc_test);
+ selftests::register_test ("amd64-tdesc", selftests::tdesc::amd64_tdesc_test);
#endif
}
@@ -3587,6 +3587,7 @@ captured_main (int argc, char *argv[])
volatile int attach = 0;
int was_running;
bool selftest = false;
+ const char *selftest_filter = NULL;
while (*next_arg != NULL && **next_arg == '-')
{
@@ -3707,6 +3708,11 @@ captured_main (int argc, char *argv[])
run_once = 1;
else if (strcmp (*next_arg, "--selftest") == 0)
selftest = true;
+ else if (startswith (*next_arg, "--selftest="))
+ {
+ selftest = true;
+ selftest_filter = *next_arg + strlen ("--selftest=");
+ }
else
{
fprintf (stderr, "Unknown argument: %s\n", *next_arg);
@@ -3783,7 +3789,14 @@ captured_main (int argc, char *argv[])
if (selftest)
{
- selftests::run_tests ();
+ debug_printf (_("Running self-tests.\n"));
+
+ selftests::selftests_results results
+ = selftests::run_tests (selftest_filter);
+
+ debug_printf (_("Ran %d unit tests, %d failed\n"),
+ results.ran, results.failed);
+
throw_quit ("Quit");
}
@@ -39,6 +39,7 @@
#include "top.h"
#include "maint.h"
#include "selftest.h"
+#include "selftest-arch.h"
#include "cli/cli-decode.h"
#include "cli/cli-utils.h"
@@ -959,7 +960,12 @@ show_per_command_cmd (char *args, int from_tty)
static void
maintenance_selftest (char *args, int from_tty)
{
- selftests::run_tests ();
+ selftests::selftests_results results_noarch = selftests::run_tests (args);
+ selftests::selftests_results results_arch = selftests::run_tests_with_arch (args);
+
+ printf_filtered (_("Ran %d unit tests, %d failed\n"),
+ results_noarch.ran + results_arch.ran,
+ results_noarch.failed + results_arch.failed);
}
@@ -1775,7 +1775,8 @@ Print the internal register configuration including each register's\n\
remote register number and buffer offset in the g/G packets.\n\
Takes an optional file parameter."),
&maintenanceprintlist);
+
#if GDB_SELF_TEST
- selftests::register_test (selftests::current_regcache_test);
+ selftests::register_test ("current_regcache", selftests::current_regcache_test);
#endif
}
@@ -2781,6 +2781,6 @@ _initialize_rust_exp (void)
gdb_assert (code == 0);
#if GDB_SELF_TEST
- selftests::register_test (rust_lex_tests);
+ selftests::register_test ("rust-lex", rust_lex_tests);
#endif
}
@@ -22,15 +22,20 @@
#include "selftest.h"
#include "selftest-arch.h"
#include "arch-utils.h"
+#include <map>
namespace selftests {
-static std::vector<self_test_foreach_arch_function *> gdbarch_tests;
+static std::map<std::string, self_test_foreach_arch_function *> gdbarch_tests;
void
-register_test_foreach_arch (self_test_foreach_arch_function *function)
+register_test_foreach_arch (const std::string &name,
+ self_test_foreach_arch_function *function)
{
- gdbarch_tests.push_back (function);
+ /* Make sure we don't already have a test with this name. */
+ gdb_assert (gdbarch_tests.find (name) == gdbarch_tests.end ());
+
+ gdbarch_tests[name] = function;
}
void
@@ -41,13 +46,17 @@ reset ()
reinit_frame_cache ();
}
-static void
-tests_with_arch ()
+selftests_results
+run_tests_with_arch (const char *filter)
{
- int failed = 0;
+ selftests_results results;
- for (const auto &f : gdbarch_tests)
+ for (const auto &test : gdbarch_tests)
{
+ if (filter != NULL && *filter != '\0'
+ && test.first.find (filter) == std::string::npos)
+ continue;
+
const char **arches = gdbarch_printable_names ();
for (int i = 0; arches[i] != NULL; i++)
@@ -80,11 +89,15 @@ tests_with_arch ()
struct gdbarch *gdbarch = gdbarch_find_by_info (info);
SELF_CHECK (gdbarch != NULL);
- f (gdbarch);
+
+ printf_unfiltered (_("Running selftest %s for arch %s.\n"),
+ test.first.c_str (), arches[i]);
+ results.ran++;
+ test.second (gdbarch);
}
CATCH (ex, RETURN_MASK_ERROR)
{
- ++failed;
+ results.failed++;
exception_fprintf (gdb_stderr, ex,
_("Self test failed: arch %s: "), arches[i]);
}
@@ -94,19 +107,8 @@ tests_with_arch ()
}
}
- SELF_CHECK (failed == 0);
+ return results;
}
} // namespace selftests
#endif /* GDB_SELF_TEST */
-
-/* Suppress warning from -Wmissing-prototypes. */
-extern initialize_file_ftype _initialize_selftests_foreach_arch;
-
-void
-_initialize_selftests_foreach_arch ()
-{
-#if GDB_SELF_TEST
- selftests::register_test (selftests::tests_with_arch);
-#endif
-}
@@ -24,7 +24,15 @@ typedef void self_test_foreach_arch_function (struct gdbarch *);
namespace selftests
{
extern void
- register_test_foreach_arch (self_test_foreach_arch_function *function);
+ register_test_foreach_arch (const std::string &name,
+ self_test_foreach_arch_function *function);
+
+/* Run all the architecture-dependent self tests.
+
+ If FILTER is not NULL and not empty, only tests with names containing FILTER
+ will be ran. */
+
+extern selftests_results run_tests_with_arch (const char *filter);
}
#endif /* SELFTEST_ARCH_H */
@@ -491,5 +491,6 @@ run_tests ()
void
_initialize_array_view_selftests ()
{
- selftests::register_test (selftests::array_view_tests::run_tests);
+ selftests::register_test ("array_view",
+ selftests::array_view_tests::run_tests);
}
@@ -301,5 +301,6 @@ run_tests ()
void
_initialize_environ_selftests ()
{
- selftests::register_test (selftests::gdb_environ_tests::run_tests);
+ selftests::register_test ("gdb_environ",
+ selftests::gdb_environ_tests::run_tests);
}
@@ -174,5 +174,6 @@ run_tests ()
void
_initialize_function_view_selftests ()
{
- selftests::register_test (selftests::function_view::run_tests);
+ selftests::register_test ("function_view",
+ selftests::function_view::run_tests);
}
@@ -174,5 +174,5 @@ run_tests ()
void
_initialize_offset_type_selftests ()
{
- selftests::register_test (selftests::offset_type::run_tests);
+ selftests::register_test ("offset_type", selftests::offset_type::run_tests);
}
@@ -90,5 +90,5 @@ run_tests ()
void
_initialize_optional_selftests ()
{
- selftests::register_test (selftests::optional::run_tests);
+ selftests::register_test ("optional", selftests::optional::run_tests);
}
@@ -106,5 +106,6 @@ run_tests ()
void
_initialize_scoped_restore_selftests ()
{
- selftests::register_test (selftests::scoped_restore_tests::run_tests);
+ selftests::register_test ("scoped_restore",
+ selftests::scoped_restore_tests::run_tests);
}
@@ -55,6 +55,6 @@ void
_initialize_utils_selftests (void)
{
#if GDB_SELF_TEST
- selftests::register_test (selftests::common_utils_tests);
+ selftests::register_test ("common-utils", selftests::common_utils_tests);
#endif
}
@@ -3307,6 +3307,6 @@ _initialize_utils (void)
add_internal_problem_command (&demangler_warning_problem);
#if GDB_SELF_TEST
- selftests::register_test (gdb_realpath_tests);
+ selftests::register_test ("gdb_realpath", gdb_realpath_tests);
#endif
}