Patchwork Add parameter to allow enabling/disabling selftests via configure

login
register
mail settings
Submitter Simon Marchi
Date Oct. 6, 2018, 1:19 a.m.
Message ID <d84a5691-166f-e252-fcb3-a247720e492e@simark.ca>
Download mbox | patch
Permalink /patch/29664/
State New
Headers show

Comments

Simon Marchi - Oct. 6, 2018, 1:19 a.m.
On 2018-09-17 4:22 p.m., Sergio Durigan Junior wrote:
> This is a follow-up of:
> 
>   https://sourceware.org/ml/gdb-patches/2018-08/msg00347.html
> 
> Instead of going throttle and always enabling our selftests (even in
> non-development builds), this patch is a bit more conservative and
> introduces a configure option ("--enable-unit-tests") that allows the
> user to choose whether she wants unit tests in the build or not.  Note
> that the current behaviour is retained: if no option is provided, GDB
> will have selftests included in a development build, and will *not*
> have selftests included in a non-development build.
> 
> The rationale for having this option is still the same: due to the
> many racy testcases and random failures we see when running the GDB
> testsuite, it is unfortunately not possible to perform a full test
> when one is building a downstream package.  As the Fedora GDB
> maintainer and one of the Debian GDB uploaders, I feel like this
> situation could be improved by, at least, executing our selftests
> after the package has been built.
> 
> This patch introduces no regressions to our build.

Hi Sergio,

I think having an configure switch with that default value makes sense.

I have two suggestions:

- Mention the switch explicitly in the error message (--{enable,disable}-unit-tests)
  so the user knows exactly what you are talking about.
- Share the code between gdb's and gdbserver's configure script

> +`--enable-unit-tests[=yes|no]'
> +     Enable (i.e., include) support for unit tests when compiling GDB
> +     and GDBServer.  Note that if this option is not passed, GDB will
> +     have selftests if it is a development build, and will *not* have
> +     selftests if it a non-development build.

Missing a word: "if it is a".

Here's a patch that goes on top of yours that does what I suggest (I had to try it
before suggesting it to make sure I don't ask for the impossible, so I might as well
share it).  With this (especially the code-sharing part), the patch LGTM (the
ChangeLog entry would need to be updated to account for the new changes).

Thanks,

Simon


From 8721a296c564b572c03e20031b33151fc3e9b7a8 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Fri, 5 Oct 2018 21:04:38 -0400
Subject: [PATCH] Suggestions

---
 gdb/README                 |  2 +-
 gdb/acinclude.m4           |  3 +++
 gdb/configure              |  6 ++++-
 gdb/configure.ac           | 21 ++----------------
 gdb/gdbserver/acinclude.m4 |  3 +++
 gdb/gdbserver/configure    |  8 +++++--
 gdb/gdbserver/configure.ac | 21 ++----------------
 gdb/selftest.m4            | 45 ++++++++++++++++++++++++++++++++++++++
 8 files changed, 67 insertions(+), 42 deletions(-)
 create mode 100644 gdb/selftest.m4
Sergio Durigan Junior - Oct. 10, 2018, 8:31 p.m.
On Friday, October 05 2018, Simon Marchi wrote:

> On 2018-09-17 4:22 p.m., Sergio Durigan Junior wrote:
>> This is a follow-up of:
>> 
>>   https://sourceware.org/ml/gdb-patches/2018-08/msg00347.html
>> 
>> Instead of going throttle and always enabling our selftests (even in
>> non-development builds), this patch is a bit more conservative and
>> introduces a configure option ("--enable-unit-tests") that allows the
>> user to choose whether she wants unit tests in the build or not.  Note
>> that the current behaviour is retained: if no option is provided, GDB
>> will have selftests included in a development build, and will *not*
>> have selftests included in a non-development build.
>> 
>> The rationale for having this option is still the same: due to the
>> many racy testcases and random failures we see when running the GDB
>> testsuite, it is unfortunately not possible to perform a full test
>> when one is building a downstream package.  As the Fedora GDB
>> maintainer and one of the Debian GDB uploaders, I feel like this
>> situation could be improved by, at least, executing our selftests
>> after the package has been built.
>> 
>> This patch introduces no regressions to our build.
>
> Hi Sergio,

Hi Simon,

Thanks for the review.

> I think having an configure switch with that default value makes sense.
>
> I have two suggestions:
>
> - Mention the switch explicitly in the error message (--{enable,disable}-unit-tests)
>   so the user knows exactly what you are talking about.
> - Share the code between gdb's and gdbserver's configure script
>
>> +`--enable-unit-tests[=yes|no]'
>> +     Enable (i.e., include) support for unit tests when compiling GDB
>> +     and GDBServer.  Note that if this option is not passed, GDB will
>> +     have selftests if it is a development build, and will *not* have
>> +     selftests if it a non-development build.
>
> Missing a word: "if it is a".
>
> Here's a patch that goes on top of yours that does what I suggest (I had to try it
> before suggesting it to make sure I don't ask for the impossible, so I might as well
> share it).  With this (especially the code-sharing part), the patch LGTM (the
> ChangeLog entry would need to be updated to account for the new changes).

Fair enough.  I applied your patch, updated the ChangeLog, and pushed
it.

8ecfd7bd4acd69213c06fac6de9af38299123547

Thanks,

Patch

diff --git a/gdb/README b/gdb/README
index 4639d76c4e83..8a91aab2a4c6 100644
--- a/gdb/README
+++ b/gdb/README
@@ -549,7 +549,7 @@  more obscure GDB `configure' options are not listed here.
      Enable (i.e., include) support for unit tests when compiling GDB
      and GDBServer.  Note that if this option is not passed, GDB will
      have selftests if it is a development build, and will *not* have
-     selftests if it a non-development build.
+     selftests if it is a non-development build.

 `configure' accepts other options, for compatibility with configuring
 other GNU tools recursively.
diff --git a/gdb/acinclude.m4 b/gdb/acinclude.m4
index 52ba3f9ed6e1..d21dd76fab9c 100644
--- a/gdb/acinclude.m4
+++ b/gdb/acinclude.m4
@@ -18,6 +18,9 @@  sinclude(warning.m4)
 # AM_GDB_UBSAN
 sinclude(sanitize.m4)

+# This gets GDB_AC_SELFTEST.
+sinclude(selftest.m4)
+
 dnl gdb/configure.in uses BFD_NEED_DECLARATION, so get its definition.
 sinclude(../bfd/bfd.m4)

diff --git a/gdb/configure b/gdb/configure
index 7a9295459692..47319bf36a3e 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -17841,6 +17841,7 @@  ac_config_links="$ac_config_links $ac_config_links_1"
 $as_echo "#define GDB_DEFAULT_HOST_CHARSET \"UTF-8\"" >>confdefs.h


+
 # Check whether we will enable the inclusion of unit tests when
 # compiling GDB.
 #
@@ -17852,7 +17853,7 @@  if test "${enable_unit_tests+set}" = set; then :
   enableval=$enable_unit_tests; case "${enableval}" in
   yes)  enable_unittests=true  ;;
   no)   enable_unittests=false ;;
-  *)    as_fn_error $? "bad value ${enableval} for enable unit tests option" "$LINENO" 5 ;;
+  *)    as_fn_error $? "bad value ${enableval} for --{enable,disable}-unit-tests option" "$LINENO" 5 ;;
 esac
 else
   enable_unittests=$development
@@ -17863,11 +17864,14 @@  if $enable_unittests; then

 $as_echo "#define GDB_SELF_TEST 1" >>confdefs.h

+
   CONFIG_OBS="$CONFIG_OBS \$(SUBDIR_UNITTESTS_OBS) common/selftest.o selftest-arch.o"
   CONFIG_SRCS="$CONFIG_SRCS \$(SUBDIR_UNITTESTS_SRCS) common/selftest.c selftest-arch.c"
+
 fi


+
   gdb_ac_transform=`echo "$program_transform_name" | sed -e 's/\\$\\$/\\$/g'`
   GDB_TRANSFORM_NAME=`echo gdb | sed -e "$gdb_ac_transform"`
   if test "x$GDB_TRANSFORM_NAME" = x; then
diff --git a/gdb/configure.ac b/gdb/configure.ac
index 987bf01df86c..b2343a90e5fa 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -2247,27 +2247,10 @@  dnl  At the moment, we just assume it's UTF-8.
 AC_DEFINE(GDB_DEFAULT_HOST_CHARSET, "UTF-8",
           [Define to be a string naming the default host character set.])

-# Check whether we will enable the inclusion of unit tests when
-# compiling GDB.
-#
-# The default value of this option changes depending whether we're on
-# development mode (in which case it's "true") or not (in which case
-# it's "false").
-AC_ARG_ENABLE(unit-tests,
-AS_HELP_STRING([--enable-unit-tests],
-[Enable the inclusion of unit tests when compiling GDB]),
-[case "${enableval}" in
-  yes)  enable_unittests=true  ;;
-  no)   enable_unittests=false ;;
-  *)    AC_MSG_ERROR(bad value ${enableval} for enable unit tests option) ;;
-esac], [enable_unittests=$development])
-
-if $enable_unittests; then
-  AC_DEFINE(GDB_SELF_TEST, 1,
-            [Define if self-testing features should be enabled])
+GDB_AC_SELFTEST([
   CONFIG_OBS="$CONFIG_OBS \$(SUBDIR_UNITTESTS_OBS) common/selftest.o selftest-arch.o"
   CONFIG_SRCS="$CONFIG_SRCS \$(SUBDIR_UNITTESTS_SRCS) common/selftest.c selftest-arch.c"
-fi
+])

 GDB_AC_TRANSFORM([gdb], [GDB_TRANSFORM_NAME])
 GDB_AC_TRANSFORM([gcore], [GCORE_TRANSFORM_NAME])
diff --git a/gdb/gdbserver/acinclude.m4 b/gdb/gdbserver/acinclude.m4
index c75d783f57bb..fc3e344a611a 100644
--- a/gdb/gdbserver/acinclude.m4
+++ b/gdb/gdbserver/acinclude.m4
@@ -31,6 +31,9 @@  m4_include(../ptrace.m4)

 m4_include(../ax_cxx_compile_stdcxx.m4)

+dnl For GDB_AC_SELFTEST.
+m4_include(../selftest.m4)
+
 dnl Check for existence of a type $1 in libthread_db.h
 dnl Based on BFD_HAVE_SYS_PROCFS_TYPE in bfd/bfd.m4.

diff --git a/gdb/gdbserver/configure b/gdb/gdbserver/configure
index 3df3111259fd..1ddbd6b27e0a 100755
--- a/gdb/gdbserver/configure
+++ b/gdb/gdbserver/configure
@@ -5892,6 +5892,7 @@  fi
   fi


+
 # Check whether we will enable the inclusion of unit tests when
 # compiling GDB.
 #
@@ -5903,7 +5904,7 @@  if test "${enable_unit_tests+set}" = set; then :
   enableval=$enable_unit_tests; case "${enableval}" in
   yes)  enable_unittests=true  ;;
   no)   enable_unittests=false ;;
-  *)    as_fn_error $? "bad value ${enableval} for enable unit tests option" "$LINENO" 5 ;;
+  *)    as_fn_error $? "bad value ${enableval} for --{enable,disable}-unit-tests option" "$LINENO" 5 ;;
 esac
 else
   enable_unittests=$development
@@ -5911,12 +5912,15 @@  fi


 if $enable_unittests; then
-  srv_selftest_objs="common/selftest.o"

 $as_echo "#define GDB_SELF_TEST 1" >>confdefs.h

+
+  srv_selftest_objs="common/selftest.o"
+
 fi

+
  case ${build_alias} in
   "") build_noncanonical=${build} ;;
   *) build_noncanonical=${build_alias} ;;
diff --git a/gdb/gdbserver/configure.ac b/gdb/gdbserver/configure.ac
index f1e7086662af..f1dfa4546cce 100644
--- a/gdb/gdbserver/configure.ac
+++ b/gdb/gdbserver/configure.ac
@@ -54,26 +54,9 @@  else
 fi
 GDB_AC_LIBMCHECK(${libmcheck_default})

-# Check whether we will enable the inclusion of unit tests when
-# compiling GDB.
-#
-# The default value of this option changes depending whether we're on
-# development mode (in which case it's "true") or not (in which case
-# it's "false").
-AC_ARG_ENABLE(unit-tests,
-AS_HELP_STRING([--enable-unit-tests],
-[Enable the inclusion of unit tests when compiling GDB]),
-[case "${enableval}" in
-  yes)  enable_unittests=true  ;;
-  no)   enable_unittests=false ;;
-  *)    AC_MSG_ERROR(bad value ${enableval} for enable unit tests option) ;;
-esac], [enable_unittests=$development])
-
-if $enable_unittests; then
+GDB_AC_SELFTEST([
   srv_selftest_objs="common/selftest.o"
-  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/selftest.m4 b/gdb/selftest.m4
new file mode 100644
index 000000000000..acf4050bde85
--- /dev/null
+++ b/gdb/selftest.m4
@@ -0,0 +1,45 @@ 
+dnl Copyright (C) 2018 Free Software Foundation, Inc.
+dnl
+dnl This file is part of GDB.
+dnl
+dnl This program is free software; you can redistribute it and/or modify
+dnl it under the terms of the GNU General Public License as published by
+dnl the Free Software Foundation; either version 3 of the License, or
+dnl (at your option) any later version.
+dnl
+dnl This program is distributed in the hope that it will be useful,
+dnl but WITHOUT ANY WARRANTY; without even the implied warranty of
+dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+dnl GNU General Public License for more details.
+dnl
+dnl You should have received a copy of the GNU General Public License
+dnl along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+dnl GDB_AC_SELFTEST(ACTION-IF-ENABLED)
+dnl
+dnl Enable the unit/self tests if needed.  If they are enabled, AC_DEFINE
+dnl the GDB_SELF_TEST macro, and execute ACTION-IF-ENABLED.
+
+AC_DEFUN([GDB_AC_SELFTEST],[
+# Check whether we will enable the inclusion of unit tests when
+# compiling GDB.
+#
+# The default value of this option changes depending whether we're on
+# development mode (in which case it's "true") or not (in which case
+# it's "false").
+AC_ARG_ENABLE(unit-tests,
+AS_HELP_STRING([--enable-unit-tests],
+[Enable the inclusion of unit tests when compiling GDB]),
+[case "${enableval}" in
+  yes)  enable_unittests=true  ;;
+  no)   enable_unittests=false ;;
+  *)    AC_MSG_ERROR(
+[bad value ${enableval} for --{enable,disable}-unit-tests option]) ;;
+esac], [enable_unittests=$development])
+
+if $enable_unittests; then
+  AC_DEFINE(GDB_SELF_TEST, 1,
+            [Define if self-testing features should be enabled])
+  $1
+fi
+])