From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3887 invoked by alias); 20 Feb 2013 17:30:54 -0000 Received: (qmail 3878 invoked by uid 22791); 20 Feb 2013 17:30:54 -0000 X-SWARE-Spam-Status: No, hits=-7.9 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; Wed, 20 Feb 2013 17:30:45 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r1KHUcN4024941 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 20 Feb 2013 12:30:41 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r1KHUIuj004162; Wed, 20 Feb 2013 12:30:30 -0500 Message-ID: <5125082A.8040901@redhat.com> Date: Wed, 20 Feb 2013 17:30: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: Pedro Alves , 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> In-Reply-To: <20130220160808.GA19181@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/msg00538.txt.bz2 On 02/20/2013 04:08 PM, Jan Kratochvil wrote: > On Wed, 20 Feb 2013 16:56:04 +0100, Pedro Alves wrote: >> However, one unwritten rule (not just GDB's) that applies more broadly >> than configure things is to not overengineer things, do the minimal >> necessary, and worry only about unknown problems if they turn >> out to be real problems. > > In such case we could use gdb_assert / internal_error everywhere. I don't follow. Seems like a non sequitur. > This was what I was submitting before as any GDB problems get then > automatically bugreported with context via ABRT, probably also Apport etc. > I was told on gdb-patches warning calls are preferred in such case instead as > it does not break the user's debugging session. I'm not sure what you're talking about. Are you talking about the issue I pointed out in the original review of the broken nx detection, with the wait calls potentially catching an unexpected signal? That was something that can trigger in reality, not an imagined problem. It seems you're conflating different kinds of issues. > > 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. "Is all this complication necessary" is a natural question for a reviewer to ask. The answer may well be "yes", and that's fine. You asked for my opinion, and I gave it. Again, I did not intend to argue. > > So there should be either the way of (1) fatal error in the case of troubles > or (2) ignore (warn only) any troubles. > I do not see a clear direction of (1) or (2), initially I tried to go (1) and > now I try to go (2) but neither seems OK for you. The problem seems to be you're putting all kinds of "troubles" in the same bucket. -- commit 1c776cef3b1f203e0c6aa6347ad4129268c5ec8c Author: Pedro Alves Date: Wed Feb 20 17:00:07 2013 +0000 simpler valgrind. diff --git a/gdb/common/linux-ptrace.c b/gdb/common/linux-ptrace.c index 886be80..75c30e4 100644 --- a/gdb/common/linux-ptrace.c +++ b/gdb/common/linux-ptrace.c @@ -29,6 +29,10 @@ #include "gdb_assert.h" #include "gdb_wait.h" +#ifdef HAVE_VALGRIND_VALGRIND_H +# include +#endif + /* Find all possible reasons we could fail to attach PID and append these newline terminated reason strings to initialized BUFFER. '\0' termination of BUFFER must be done by the caller. */ @@ -76,6 +80,15 @@ linux_ptrace_test_ret_to_nx (void) long l; int status; +#if defined HAVE_VALGRIND_VALGRIND_H && defined RUNNING_ON_VALGRIND + /* Below we'll mmap a non-executable page, which under Valgrind + results in annoying warnings such as + "Bad permissions for mapped region at address 0xfoobar". + Just skip the whole test if running under Valgrind. */ + if (RUNNING_ON_VALGRIND) + return; +#endif + return_address = mmap (NULL, 2, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); if (return_address == MAP_FAILED) diff --git a/gdb/config.in b/gdb/config.in index 9e21325..47e2d6b 100644 --- a/gdb/config.in +++ b/gdb/config.in @@ -545,6 +545,9 @@ /* Define to 1 if you have the header file. */ #undef HAVE_UNISTD_H +/* Define to 1 if you have the header file. */ +#undef HAVE_VALGRIND_VALGRIND_H + /* Define to 1 if you have the `vfork' function. */ #undef HAVE_VFORK diff --git a/gdb/configure b/gdb/configure index c54709c..e23fa29 100755 --- a/gdb/configure +++ b/gdb/configure @@ -8893,7 +8893,8 @@ for ac_header in nlist.h machine/reg.h poll.h sys/poll.h proc_service.h \ sys/reg.h sys/debugreg.h sys/select.h sys/syscall.h \ sys/types.h sys/wait.h wait.h termios.h termio.h \ sgtty.h unistd.h elf_hp.h locale.h \ - dlfcn.h sys/socket.h sys/un.h linux/perf_event.h + dlfcn.h sys/socket.h sys/un.h linux/perf_event.h \ + valgrind/valgrind.h do : as_ac_Header=`$as_echo "ac_cv_header_$ac_header" | $as_tr_sh` ac_fn_c_check_header_mongrel "$LINENO" "$ac_header" "$as_ac_Header" "$ac_includes_default" diff --git a/gdb/configure.ac b/gdb/configure.ac index e501766..1060602 100644 --- a/gdb/configure.ac +++ b/gdb/configure.ac @@ -1082,7 +1082,8 @@ AC_CHECK_HEADERS([nlist.h machine/reg.h poll.h sys/poll.h proc_service.h \ sys/reg.h sys/debugreg.h sys/select.h sys/syscall.h \ sys/types.h sys/wait.h wait.h termios.h termio.h \ sgtty.h unistd.h elf_hp.h locale.h \ - dlfcn.h sys/socket.h sys/un.h linux/perf_event.h]) + dlfcn.h sys/socket.h sys/un.h linux/perf_event.h \ + valgrind/valgrind.h]) AC_CHECK_HEADERS(link.h, [], [], [#if HAVE_SYS_TYPES_H # include diff --git a/gdb/gdbserver/config.in b/gdb/gdbserver/config.in index b791b89..bcdb590 100644 --- a/gdb/gdbserver/config.in +++ b/gdb/gdbserver/config.in @@ -214,6 +214,9 @@ /* Define if UST is available */ #undef HAVE_UST +/* Define to 1 if you have the header file. */ +#undef HAVE_VALGRIND_VALGRIND_H + /* Define to 1 if you have the `vasprintf' function. */ #undef HAVE_VASPRINTF diff --git a/gdb/gdbserver/configure b/gdb/gdbserver/configure index 9ec9340..eec4696 100755 --- a/gdb/gdbserver/configure +++ b/gdb/gdbserver/configure @@ -4582,7 +4582,7 @@ $as_echo "$as_me: running $SHELL $ac_sub_configure $ac_sub_configure_args --cach cd "$ac_popdir" -for ac_header in sgtty.h termio.h termios.h sys/reg.h string.h proc_service.h sys/procfs.h thread_db.h linux/elf.h stdlib.h unistd.h errno.h fcntl.h signal.h sys/file.h malloc.h sys/ioctl.h netinet/in.h sys/socket.h netdb.h netinet/tcp.h arpa/inet.h sys/wait.h wait.h sys/un.h linux/perf_event.h +for ac_header in sgtty.h termio.h termios.h sys/reg.h string.h proc_service.h sys/procfs.h thread_db.h linux/elf.h stdlib.h unistd.h errno.h fcntl.h signal.h sys/file.h malloc.h sys/ioctl.h netinet/in.h sys/socket.h netdb.h netinet/tcp.h arpa/inet.h sys/wait.h wait.h sys/un.h linux/perf_event.h valgrind/valgrind.h do : as_ac_Header=`$as_echo "ac_cv_header_$ac_header" | $as_tr_sh` ac_fn_c_check_header_mongrel "$LINENO" "$ac_header" "$as_ac_Header" "$ac_includes_default" diff --git a/gdb/gdbserver/configure.ac b/gdb/gdbserver/configure.ac index 55fb461..a72a19c 100644 --- a/gdb/gdbserver/configure.ac +++ b/gdb/gdbserver/configure.ac @@ -68,7 +68,7 @@ AC_CHECK_HEADERS(sgtty.h termio.h termios.h sys/reg.h string.h dnl errno.h fcntl.h signal.h sys/file.h malloc.h dnl sys/ioctl.h netinet/in.h sys/socket.h netdb.h dnl netinet/tcp.h arpa/inet.h sys/wait.h wait.h sys/un.h dnl - linux/perf_event.h) + linux/perf_event.h valgrind/valgrind.h) AC_CHECK_FUNCS(pread pwrite pread64 readlink) AC_REPLACE_FUNCS(vasprintf vsnprintf)