From patchwork Sat Oct 6 01:19:35 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Marchi X-Patchwork-Id: 29664 Received: (qmail 77438 invoked by alias); 6 Oct 2018 01:19:44 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 77424 invoked by uid 89); 6 Oct 2018 01:19:43 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 spammy=Suggestions, H*MI:sk:2018091, UD:p.m, p.m X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 06 Oct 2018 01:19:39 +0000 Received: from [10.0.0.11] (unknown [192.222.164.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 5FBCA1E52B; Fri, 5 Oct 2018 21:19:36 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=simark.ca; s=mail; t=1538788776; bh=2+bQ/sfA7S9wG4Ui7iB5z9yVoWFPNTdcsWiNulJrwyI=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=BvMSbicAC8gyZ1VKZtv+VotO3mMUKDsNyY3gRdoqzl2/eM3Vbe4irgIRLCDT2SDxI F5hrhQr/2e5I/6klPrzyexoVPn0tA/TxdlytnX7W7eS0K18x1ZGQ7SGHh0KH4lBdEP QMBSpnME+B8qgWuaBBfpPAXSyxailF16cVglaG2w= Subject: Re: [PATCH] Add parameter to allow enabling/disabling selftests via configure To: Sergio Durigan Junior , GDB Patches Cc: Pedro Alves References: <20180814054221.13061-1-sergiodj@redhat.com> <20180917202242.28583-1-sergiodj@redhat.com> From: Simon Marchi Message-ID: Date: Fri, 5 Oct 2018 21:19:35 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0 MIME-Version: 1.0 In-Reply-To: <20180917202242.28583-1-sergiodj@redhat.com> 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 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 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 . + +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 +])