From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26506 invoked by alias); 25 Feb 2013 18:10:29 -0000 Received: (qmail 26488 invoked by uid 22791); 25 Feb 2013 18:10:26 -0000 X-SWARE-Spam-Status: No, hits=-8.0 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 25 Feb 2013 18:10:18 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r1PIAFnu018261 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 25 Feb 2013 13:10:15 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r1PIACt9001370; Mon, 25 Feb 2013 13:10:13 -0500 Message-ID: <512BA904.8080604@redhat.com> Date: Mon, 25 Feb 2013 18:10:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130110 Thunderbird/17.0.2 MIME-Version: 1.0 To: Jan Kratochvil CC: Philippe Waroquiers , gdb-patches@sourceware.org Subject: Re: [patch+NEWS] Avoid false valgrind warnings on linux_ptrace_test_ret_to_nx References: <20130220140520.GA10822@host2.jankratochvil.net> <5124E66D.3060606@redhat.com> <20130220152331.GA16617@host2.jankratochvil.net> <5124F214.7020506@redhat.com> <20130220160808.GA19181@host2.jankratochvil.net> <5125082A.8040901@redhat.com> <20130225143242.GB25286@host2.jankratochvil.net> In-Reply-To: <20130225143242.GB25286@host2.jankratochvil.net> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2013-02/txt/msg00633.txt.bz2 On 02/25/2013 02:32 PM, Jan Kratochvil wrote: > On Wed, 20 Feb 2013 18:30:18 +0100, Pedro Alves wrote: >> On 02/20/2013 04:08 PM, Jan Kratochvil wrote: >>> OTOH here you ask for the opposite, unchecked inclusion of include file >>> which is very complicated >>> with compiler-dependent parts and arch-specific code, >>> which all seem to me to possibly cause compilation failures. >> >> Now I'm at a total loss. I have no idea what you're considering >> very complicated, and what needs to be compiler dependent. > >> All I was thinking was something like the patch below. > > It primarily misses the --with-valgrind feature: to error out when user does > want the (valgrind) support but system does not have the prerequisite files. > > As the packaging system may omit some of the prerequisites (such as when PKG > splits out its PKG-devel subpackage) one needs to ensure such build will fail. > We do not want to quietly build new GDB missing some of the expected features. > --with-valgrind makes this easy. Otherwise the packager needs to place in > the .spec file: > grep HAVE_VALGRIND_VALGRIND_H gdb/config.h > Moreover multiple times as config.h gets sometimes rebuilt during some build > scenarios. > > For example Fedora gdb.spec already has such a hack because there is no > corresponding --with-*/--enable-* option for this one: > ! grep '_RELOCATABLE.*1' gdb/config.h > > It may be clear I do not want to introduce more such greps there. Thanks. That's a much better rationale than handling imagined broken headers. > > > So with the --with-valgrind requirement we end up discussing whether these 8 > lines of code should or should not be there: > > + AC_MSG_CHECKING([for RUNNING_ON_VALGRIND macro]) > + AC_CACHE_VAL(gdb_cv_check_running_on_valgrind, [ > + AC_LINK_IFELSE( > + [AC_LANG_PROGRAM([[#include ]], > + [[return RUNNING_ON_VALGRIND;]])], > + [gdb_cv_check_running_on_valgrind=yes], > + [gdb_cv_check_running_on_valgrind=no])]) > + AC_MSG_RESULT($gdb_cv_check_running_on_valgrind) > > I believe there is a real risk there can exist old system(s) on non-x86* arch > having valgrind/valgrind.h available but failing to build RUNNING_ON_VALGRIND. > > Sure people have different opinions what is useful and what is just a bloat. > Although when I already wrote the code I do not find such a serious problem to > put there such 8 lines to sanity check the compatibility. It was the fact that the desire for the option wasn't fully explained and the "is there a rule for configure checks" question that prompted me to go more general, as I didn't want to set precedent for adding checks for unknown problems without reason. It's better to wait for the problems to appear, so at least our future selves wanting to change this code know what sort of problems to expect. Going back to this particular case, as long as we have to have the --with-foo switch, I agree those 8 lines are no real issue. I find the non-x86* argument somewhat compelling. I do still think the mention of valgrind/valgrind.h here: [--with-valgrind was requested but was not found]) isn't 100% correct, and will be misleading -- your non-x86* example failing to build RUNNING_ON_VALGRIND would show that error, though the header was indeed found. If you don't like the other suggestion for "support" and you still want to mention the header, then adding a "working" would be fine with me: [--with-valgrind was requested but working was not found]) Anyway, that's a very minor detail. I went through the patch again, and it stills looks good to me otherwise. Thanks, this email I'm replying to has the sort of info that was obvious to you, but was not to me and maybe others (either present or future selves doing archaeology), and is good and useful info to have explicit on patch submissions (*). (*) - and I wish in commit logs - following Joel's lead, I've been personally making an effort to push self-contained commit logs with full change description myself. -- Pedro Alves