Re: [patch] Avoid false valgrind warnings on linux_ptrace_test_ret_to_nx

Jan Kratochvil
2013-02-20T14:06:30Z

On Tue, 19 Feb 2013 20:11:26 +0100, Pedro Alves wrote:
> On 02/19/2013 06:42 PM, Jan Kratochvil wrote:
> > + [AS_HELP_STRING([--without-valgrind],
> > + [do not include false valgrind warning message skip])],,
>
> I have trouble parsing this help string. I suggest instead:
>
> "do not include support for running-on-valgrind detection"

The problem is this message does not say what is it good for from the user
point of view. But used it as I do not mind which message is there,
particularly as I agree the message of mine was very cryptic.

> (Nit, the mmap warning is not really false, as the code is
> doing what the warning says. It's only that it's annoying.)

From the user point of view valgrind should report problems in the inferior.
This test is not a problem in GDB.

> > + AC_ERROR([--with-valgrind was requested but it has not been found])
>
> A reader is left confused over what's "it"? A possible simple/small
> fix is just s/it/support/ or s/it/necessary support/.

OK, so why to be so abstract:
[--with-valgrind was requested but <valgrind/valgrind.h> was not found])

> /* 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. */

Added there, with closing '"'.

On Tue, 19 Feb 2013 22:10:06 +0100, Philippe Waroquiers wrote:
> On Tue, 2013-02-19 at 19:42 +0100, Jan Kratochvil wrote:
>
> > +#ifdef HAVE_RUNNING_ON_VALGRIND
> > + if (HAVE_RUNNING_ON_VALGRIND)
> Isn't it rather
> if (RUNNING_ON_VALGRIND)
> which is needed ?
> > + return;
> > +#endif

Oops.

Thanks,
Jan

gdb/
2013-02-20 Jan Kratochvil <jan.kratochvil@redhat.com>

* acinclude.m4: Include common/acinclude.m4.
* common/acinclude.m4: New file.
* common/linux-ptrace.c <HAVE_RUNNING_ON_VALGRIND>: Include
valgrind/valgrind.h.
(linux_ptrace_test_ret_to_nx) <HAVE_RUNNING_ON_VALGRIND>: Return.
* config.in: Regenerate.
* configure: Regenerate.
* configure.ac: Call GDB_AC_CHECK_VALGRIND.

gdb/gdbserver/
2013-02-20 Jan Kratochvil <jan.kratochvil@redhat.com>

* acinclude.m4: Include ../common/acinclude.m4.
* config.in: Regenerate.
* configure: Regenerate.
* configure.ac: Call GDB_AC_CHECK_VALGRIND.

diff --git a/gdb/acinclude.m4 b/gdb/acinclude.m4
index 25caddd..12da851 100644
--- a/gdb/acinclude.m4
+++ b/gdb/acinclude.m4
@@ -49,6 +49,8 @@ sinclude([../config/codeset.m4])

sinclude([../config/zlib.m4])

+sinclude([common/acinclude.m4])
+
## ----------------------------------------- ##
## ANSIfy the C compiler whenever possible. ##
## From Franc,ois Pinard ##
diff --git a/gdb/common/acinclude.m4 b/gdb/common/acinclude.m4
new file mode 100644
index 0000000..fcb1c71
--- /dev/null
+++ b/gdb/common/acinclude.m4
@@ -0,0 +1,44 @@
+# Copyright 2013 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+dnl GDB_AC_CHECK_VALGRIND
+dnl Check for HAVE_RUNNING_ON_VALGRIND.
+dnl If such symbol is defined <valgrind/valgrind.h> is also available.
+AC_DEFUN([GDB_AC_CHECK_VALGRIND], [
+ AC_ARG_WITH([valgrind],
+ [AS_HELP_STRING([--without-valgrind],
+ [do not include support for running-on-valgrind detection])],,
+ [with_valgrind=check])
+ if test "$with_valgrind" != no; then
+ AC_MSG_CHECKING([for RUNNING_ON_VALGRIND macro])
+ AC_CACHE_VAL(gdb_cv_check_running_on_valgrind, [
+ AC_LINK_IFELSE(
+ [AC_LANG_PROGRAM([[#include <valgrind/valgrind.h>]],
+ [[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)
+ if test "$gdb_cv_check_running_on_valgrind" = yes; then
+ with_valgrind=yes
+ elif test "$with_valgrind" = yes; then
+ AC_ERROR(
+ [--with-valgrind was requested but <valgrind/valgrind.h> was not found])
+ fi
+ fi
+ if test "$with_valgrind" = yes; then
+ AC_DEFINE(HAVE_RUNNING_ON_VALGRIND, 1,
+ [Define if you have <valgrind/valgrind.h> and RUNNING_ON_VALGRIND.])
+ fi
+])
diff --git a/gdb/common/linux-ptrace.c b/gdb/common/linux-ptrace.c
index 886be80..97a314b 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_RUNNING_ON_VALGRIND
+# include <valgrind/valgrind.h>
+#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;

+ /* 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. */
+#ifdef HAVE_RUNNING_ON_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/configure.ac b/gdb/configure.ac
index e501766..a7bac76 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -1166,6 +1166,7 @@ AC_CHECK_FUNCS([canonicalize_file_name realpath getrusage getuid getgid \
ttrace wborder wresize setlocale iconvlist libiconvlist btowc \
setrlimit getrlimit posix_madvise waitpid lstat])
AM_LANGINFO_CODESET
+GDB_AC_CHECK_VALGRIND

# Check the return and argument types of ptrace. No canned test for
# this, so roll our own.
diff --git a/gdb/gdbserver/acinclude.m4 b/gdb/gdbserver/acinclude.m4
index 0e0bdc8..c5a93b7 100644
--- a/gdb/gdbserver/acinclude.m4
+++ b/gdb/gdbserver/acinclude.m4
@@ -12,6 +12,8 @@ sinclude(../../config/acx.m4)
m4_include(../../config/depstand.m4)
m4_include(../../config/lead-dot.m4)

+sinclude([../common/acinclude.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.ac b/gdb/gdbserver/configure.ac
index 55fb461..d492e8b 100644
--- a/gdb/gdbserver/configure.ac
+++ b/gdb/gdbserver/configure.ac
@@ -71,6 +71,7 @@ AC_CHECK_HEADERS(sgtty.h termio.h termios.h sys/reg.h string.h dnl
linux/perf_event.h)
AC_CHECK_FUNCS(pread pwrite pread64 readlink)
AC_REPLACE_FUNCS(vasprintf vsnprintf)
+GDB_AC_CHECK_VALGRIND

# Check for UST
ustlibs=""