[14/25,RFC] GDBserver self test

Message ID 1497256916-4958-15-git-send-email-yao.qi@linaro.org
State New, archived
Headers

Commit Message

Yao Qi June 12, 2017, 8:41 a.m. UTC
  This patch uses GDB self test in GDBserver.  The self tests are run if
GDBserver is started with option --self-test.

gdb/gdbserver:

2017-05-26  Yao Qi  <yao.qi@linaro.org>

	* configure.ac: AC_DEFINE GDB_SELF_TEST if $development.
	* configure, config.in: Re-generated.
	* server.c: Include sefltest.h and selftest.c.
	(captured_main): Handle option --self-test.
gdb:

2017-05-26  Yao Qi  <yao.qi@linaro.org>

	* selftest.c: Adjust it for GDBserver.

gdb/testsuite:

2017-05-26  Yao Qi  <yao.qi@linaro.org>

	* gdb.server/unittest.exp: New.
---
 gdb/gdbserver/config.in               |  3 +++
 gdb/gdbserver/configure               | 12 +++++++---
 gdb/gdbserver/configure.ac            |  5 +++++
 gdb/gdbserver/server.c                | 18 ++++++++++++++-
 gdb/selftest.c                        | 18 ++++++++++++++-
 gdb/testsuite/gdb.server/unittest.exp | 41 +++++++++++++++++++++++++++++++++++
 6 files changed, 92 insertions(+), 5 deletions(-)
 create mode 100644 gdb/testsuite/gdb.server/unittest.exp
  

Comments

Pedro Alves June 28, 2017, 5:08 p.m. UTC | #1
On 06/12/2017 09:41 AM, Yao Qi wrote:
> This patch uses GDB self test in GDBserver.  The self tests are run if
> GDBserver is started with option --self-test.

The name of the corresponding gdb command is "maint selftest",
i.e., no hyphen.  If wonder if we should be consistent.

> 
> gdb/gdbserver:
> 
> 2017-05-26  Yao Qi  <yao.qi@linaro.org>
> 
> 	* configure.ac: AC_DEFINE GDB_SELF_TEST if $development.
> 	* configure, config.in: Re-generated.
> 	* server.c: Include sefltest.h and selftest.c.
> 	(captured_main): Handle option --self-test.
> gdb:
> 
> 2017-05-26  Yao Qi  <yao.qi@linaro.org>
> 
> 	* selftest.c: Adjust it for GDBserver.
> 
> gdb/testsuite:
> 
> 2017-05-26  Yao Qi  <yao.qi@linaro.org>
> 
> 	* gdb.server/unittest.exp: New.

I like the idea of running unit tests in gdbserver
(and we briefly discussed it when we introduced selftest-arch.c
IIRC).

But I don't know whether you're proposing the patch as
is, or whether it's really just for comments on the idea.
I say this because there are several hacks in the patch
that ideally we'd avoid.

> +#include "../selftest.h"
> +#include "../selftest.c"

E.g., we should really move these to common/ and void
the #ifdefery within them.

> +global server_spawn_id
> +
> +set gdbserver [find_gdbserver]
> +set gdbserver_command "$gdbserver --self-test"
> +
> +set server_spawn_id [remote_spawn target $gdbserver_command]
> +
> +gdb_expect {
> +    -i $server_spawn_id
> +    -re "Ran $decimal unit tests, 0 failed" {
> +	pass "unit tests"
> +    }
> +    -re "Ran $decimal unit tests, $decimal failed" {
> +	fail "unit tests"
> +    }

Shouldn't this have a "default" case that fails instead
of only handling one specific failure mode?

Thanks,
Pedro Alves
  
Yao Qi June 29, 2017, 9:08 a.m. UTC | #2
Pedro Alves <palves@redhat.com> writes:

> The name of the corresponding gdb command is "maint selftest",
> i.e., no hyphen.  If wonder if we should be consistent.
>

I chose "--self-test" because gcc uses "-fself-test".  I am fine to use
"--selftest" to align with GDB command "maint selftest".

>> 
>> gdb/gdbserver:
>> 
>> 2017-05-26  Yao Qi  <yao.qi@linaro.org>
>> 
>> 	* configure.ac: AC_DEFINE GDB_SELF_TEST if $development.
>> 	* configure, config.in: Re-generated.
>> 	* server.c: Include sefltest.h and selftest.c.
>> 	(captured_main): Handle option --self-test.
>> gdb:
>> 
>> 2017-05-26  Yao Qi  <yao.qi@linaro.org>
>> 
>> 	* selftest.c: Adjust it for GDBserver.
>> 
>> gdb/testsuite:
>> 
>> 2017-05-26  Yao Qi  <yao.qi@linaro.org>
>> 
>> 	* gdb.server/unittest.exp: New.
>
> I like the idea of running unit tests in gdbserver
> (and we briefly discussed it when we introduced selftest-arch.c
> IIRC).

Yes, we discussed this _idea_ before, but we didn't know how to do the
unit tests in GDBserver.  I remembered that some one said GDBserver unit
tests can be triggered by a special packet sent by GDB.  When I wrote
this patch, it was still an open question to me.  I chose a new command
line option to trigger the tests, post this patch, and see if people
like this way.

>
> But I don't know whether you're proposing the patch as
> is, or whether it's really just for comments on the idea.

The latter.  The whole patch series is about collecting comments on the
flexible target description, and GDBserver unit tests are used to make
sure my patches don't break target descriptions on GDBserver.  It is of
hacks, and I'll fix them in v3, definitely.

> I say this because there are several hacks in the patch
> that ideally we'd avoid.
>
>> +#include "../selftest.h"
>> +#include "../selftest.c"
>
> E.g., we should really move these to common/ and void
> the #ifdefery within them.
>

Yes, I'll move them to common/.

>> +global server_spawn_id
>> +
>> +set gdbserver [find_gdbserver]
>> +set gdbserver_command "$gdbserver --self-test"
>> +
>> +set server_spawn_id [remote_spawn target $gdbserver_command]
>> +
>> +gdb_expect {
>> +    -i $server_spawn_id
>> +    -re "Ran $decimal unit tests, 0 failed" {
>> +	pass "unit tests"
>> +    }
>> +    -re "Ran $decimal unit tests, $decimal failed" {
>> +	fail "unit tests"
>> +    }
>
> Shouldn't this have a "default" case that fails instead
> of only handling one specific failure mode?

OK, I'll fix it too.
  

Patch

diff --git a/gdb/gdbserver/config.in b/gdb/gdbserver/config.in
index 34a7443..5dacbac 100644
--- a/gdb/gdbserver/config.in
+++ b/gdb/gdbserver/config.in
@@ -8,6 +8,9 @@ 
 /* Define to 1 if using `alloca.c'. */
 #undef C_ALLOCA
 
+/* Define if self-testing features should be enabled */
+#undef GDB_SELF_TEST
+
 /* Define to 1 if you have `alloca', as a function or macro. */
 #undef HAVE_ALLOCA
 
diff --git a/gdb/gdbserver/configure b/gdb/gdbserver/configure
index b314c41..8d439ec 100755
--- a/gdb/gdbserver/configure
+++ b/gdb/gdbserver/configure
@@ -5813,6 +5813,12 @@  fi
   fi
 
 
+if $development; then
+
+$as_echo "#define GDB_SELF_TEST 1" >>confdefs.h
+
+fi
+
  case ${build_alias} in
   "") build_noncanonical=${build} ;;
   *) build_noncanonical=${build_alias} ;;
@@ -7498,9 +7504,9 @@  _ACEOF
 fi
 
 
-# See if <sys/user.h> supports the %fs_base and %gs_base amd64 segment
-# registers.  Older amd64 Linux's don't have the fs_base and gs_base
-# members of `struct user_regs_struct'.
+# See if <sys/user.h> supports the %fs_base and %gs_bas amd64 segment registers.
+# Older amd64 Linux's don't have the fs_base and gs_base members of
+# `struct user_regs_struct'.
 ac_fn_c_check_member "$LINENO" "struct user_regs_struct" "fs_base" "ac_cv_member_struct_user_regs_struct_fs_base" "#include <sys/user.h>
 "
 if test "x$ac_cv_member_struct_user_regs_struct_fs_base" = x""yes; then :
diff --git a/gdb/gdbserver/configure.ac b/gdb/gdbserver/configure.ac
index 4ea7913..36e21c5 100644
--- a/gdb/gdbserver/configure.ac
+++ b/gdb/gdbserver/configure.ac
@@ -56,6 +56,11 @@  else
 fi
 GDB_AC_LIBMCHECK(${libmcheck_default})
 
+if $development; then
+  AC_DEFINE(GDB_SELF_TEST, 1,
+            [Define if self-testing features should be enabled])
+fi
+
 ACX_NONCANONICAL_TARGET
 ACX_NONCANONICAL_HOST
 
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 69fcab1..428a9db 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -3487,6 +3487,9 @@  detach_or_kill_for_exit_cleanup (void *ignore)
   END_CATCH
 }
 
+#include "../selftest.h"
+#include "../selftest.c"
+
 /* Main function.  This is called by the real "main" function,
    wrapped in a TRY_CATCH that handles any uncaught exceptions.  */
 
@@ -3501,6 +3504,7 @@  captured_main (int argc, char *argv[])
   volatile int multi_mode = 0;
   volatile int attach = 0;
   int was_running;
+  bool selftest = false;
 
   while (*next_arg != NULL && **next_arg == '-')
     {
@@ -3608,6 +3612,11 @@  captured_main (int argc, char *argv[])
 	disable_randomization = 0;
       else if (strcmp (*next_arg, "--once") == 0)
 	run_once = 1;
+      else if (strcmp (*next_arg, "--self-test") == 0)
+	{
+	  selftest = true;
+	  break;
+	}
       else
 	{
 	  fprintf (stderr, "Unknown argument: %s\n", *next_arg);
@@ -3623,7 +3632,8 @@  captured_main (int argc, char *argv[])
       port = *next_arg;
       next_arg++;
     }
-  if (port == NULL || (!attach && !multi_mode && *next_arg == NULL))
+  if ((port == NULL || (!attach && !multi_mode && *next_arg == NULL))
+       && !selftest)
     {
       gdbserver_usage (stderr);
       exit (1);
@@ -3676,6 +3686,12 @@  captured_main (int argc, char *argv[])
   own_buf = (char *) xmalloc (PBUFSIZ + 1);
   mem_buf = (unsigned char *) xmalloc (PBUFSIZ);
 
+  if (selftest)
+    {
+      run_self_tests ();
+      throw_quit ("Quit");
+    }
+
   if (pid == 0 && *next_arg != NULL)
     {
       int i, n;
diff --git a/gdb/selftest.c b/gdb/selftest.c
index 14b76f6..c947749 100644
--- a/gdb/selftest.c
+++ b/gdb/selftest.c
@@ -15,8 +15,15 @@ 
 
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
-
+#include "config.h"
+#ifdef GDBSERVER
+#define QUIT do {} while (0)
+#else
 #include "defs.h"
+#endif
+#include "common-defs.h"
+#include "common-exceptions.h"
+#include "common-debug.h"
 #include "selftest.h"
 #include <vector>
 
@@ -50,15 +57,24 @@  run_self_tests (void)
       CATCH (ex, RETURN_MASK_ERROR)
 	{
 	  ++failed;
+	  #ifndef GDBSERVER
 	  exception_fprintf (gdb_stderr, ex, _("Self test failed: "));
+	  #endif
 	}
       END_CATCH
 
+#ifndef GDBSERVER
       /* Clear GDB internal state.  */
       registers_changed ();
       reinit_frame_cache ();
+#endif
     }
 
+  #ifdef GDBSERVER
+  debug_printf ("Ran %lu unit tests, %d failed\n",
+		(long) tests.size (), failed);
+  #else
   printf_filtered (_("Ran %lu unit tests, %d failed\n"),
 		   (long) tests.size (), failed);
+  #endif
 }
diff --git a/gdb/testsuite/gdb.server/unittest.exp b/gdb/testsuite/gdb.server/unittest.exp
new file mode 100644
index 0000000..6dc4b6e
--- /dev/null
+++ b/gdb/testsuite/gdb.server/unittest.exp
@@ -0,0 +1,41 @@ 
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2017 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+load_lib gdbserver-support.exp
+
+standard_testfile
+
+if { [skip_gdbserver_tests] } {
+    return 0
+}
+
+global server_spawn_id
+
+set gdbserver [find_gdbserver]
+set gdbserver_command "$gdbserver --self-test"
+
+set server_spawn_id [remote_spawn target $gdbserver_command]
+
+gdb_expect {
+    -i $server_spawn_id
+    -re "Ran $decimal unit tests, 0 failed" {
+	pass "unit tests"
+    }
+    -re "Ran $decimal unit tests, $decimal failed" {
+	fail "unit tests"
+    }
+}