Add parameter to allow enabling/disabling selftests via configure
Commit Message
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
Comments
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,
@@ -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.
@@ -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)
@@ -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
@@ -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])
@@ -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.
@@ -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} ;;
@@ -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
new file mode 100644
@@ -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
+])