Re: [patch] Avoid false valgrind warnings on linux_ptrace_test_ret_to_nx

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

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=""