[patch] Avoid false valgrind warnings on linux_ptrace_test_ret_to_nx

[patch] Avoid false valgrind warnings on linux_ptrace_test_ret_to_nx

[Jan Kratochvil]

Hi,

running "valgrind gdb ..." produces this false warning due to the Ubuntu
kernel bug detection in GDB:

==6371== Process terminating with default action of signal 11 (SIGSEGV)
==6371==  Bad permissions for mapped region at address 0x4032000
==6371==    at 0x4032000: ???
==6371==    by 0x603210: linux_ptrace_init_warnings (linux-ptrace.c:244)
==6371==    by 0x5EF295: linux_child_post_startup_inferior (linux-nat.c:602)
==6371==    by 0x5EDAE2: amd64_linux_child_post_startup_inferior (amd64-linux-nat.c:573)
==6371==    by 0x5E9ACE: inf_ptrace_create_inferior (inf-ptrace.c:147)
==6371==    by 0x5F0E35: linux_nat_create_inferior (linux-nat.c:1584)
==6371==    by 0x76A330: find_default_create_inferior (target.c:3086)
==6371==    by 0x7655C5: target_create_inferior (target.c:479)
==6371==    by 0x71A7BA: run_command_1 (infcmd.c:594)
==6371==    by 0x71A921: start_command (infcmd.c:649)
==6371==    by 0x63184A: do_cfunc (cli-decode.c:113)
==6371==    by 0x634930: cmd_func (cli-decode.c:1859)
==6371==    by 0x83D673: execute_command (top.c:484)
==6371==    by 0x739D70: catch_command_errors (exceptions.c:573)
==6371==    by 0x73D612: captured_main (main.c:1009)
==6371==    by 0x739C9E: catch_errors (exceptions.c:546)
==6371==    by 0x73D6CF: gdb_main (main.c:1042)
==6371==    by 0x48D6B5: main (gdb.c:34)
warning: linux_ptrace_test_ret_to_nx: PC 0x3806e309 is neither near return address 0x4032000 nor is the return instruction 0x602e33!

BTW that 0x3806e309 is in /usr/lib64/valgrind/memcheck-amd64-linux:

Dump of assembler code for function do_syscall_WRK:
   0x000000003806e2f0 <+0>:     mov    %rdi,%rax
   0x000000003806e2f3 <+3>:     mov    %rsi,%rdi
   0x000000003806e2f6 <+6>:     mov    %rdx,%rsi
   0x000000003806e2f9 <+9>:     mov    %rcx,%rdx
   0x000000003806e2fc <+12>:    mov    %r8,%r10
   0x000000003806e2ff <+15>:    mov    %r9,%r8
   0x000000003806e302 <+18>:    mov    0x8(%rsp),%r9
   0x000000003806e307 <+23>:    syscall
   ---------------------------->
   0x000000003806e309 <+25>:    retq

"Bad permissions for mapped region at address 0x4032000" is correct, this is
a test by GDB in GDB's linux_ptrace_test_ret_to_nx.

Therefore detect it and skip the test under valgrind.

RUNNING_ON_VALGRIND is described here - as found by Pedro:
	http://valgrind.org/docs/manual/manual-core-adv.html#manual-core-adv.clientreq

While they suggest
	You are encouraged to copy the valgrind/*.h headers into your
	project's include directory, so your program doesn't have
	a compile-time dependency on Valgrind being installed.
I made a standard --with-X/--without-X external dependency as besides other
reasons the valgrind.h file is 280KB and its stripping also would not be
perfect as the macro depends on various arch-specific asm implementations.

No regressions on {x86_64,x86_64-m32,i686}-fedora19pre-linux-gnu.


Thanks,
Jan


gdb/
2013-02-19  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-19  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..a4d937d9
--- /dev/null
+++ b/gdb/common/acinclude.m4
@@ -0,0 +1,43 @@
+# Copyright 2012 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 false valgrind warning message skip])],,
+    [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 it has not been 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..e1bd5eb 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,11 @@ linux_ptrace_test_ret_to_nx (void)
   long l;
   int status;
 
+#ifdef HAVE_RUNNING_ON_VALGRIND
+  if (HAVE_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=""

Re: [patch] Avoid false valgrind warnings on linux_ptrace_test_ret_to_nx

[Pedro Alves]

On 02/19/2013 06:42 PM, Jan Kratochvil wrote:

>  ## ----------------------------------------- ##
>  ## 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..a4d937d9
> --- /dev/null
> +++ b/gdb/common/acinclude.m4
> @@ -0,0 +1,43 @@
> +# Copyright 2012 Free Software Foundation, Inc.

2013 missing.

> +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 false valgrind warning message skip])],,

I have trouble parsing this help string.  I suggest instead:

"do not include support for running-on-valgrind detection"

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

> +    [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 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/.


>  
> +#ifdef HAVE_RUNNING_ON_VALGRIND
> +  if (HAVE_RUNNING_ON_VALGRIND)
> +    return;
> +#endif
> +

I suggest adding a comment above this.  Like:

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

>    return_address = mmap (NULL, 2, PROT_READ | PROT_WRITE,
>  			 MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>    if (return_address == MAP_FAILED)

Otherwise this looks good to me.

-- 
Pedro Alves

Re: [patch] Avoid false valgrind warnings on linux_ptrace_test_ret_to_nx

[Jan Kratochvil]

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

Re: [patch] Avoid false valgrind warnings on linux_ptrace_test_ret_to_nx

[Pedro Alves]

On 02/20/2013 02:05 PM, Jan Kratochvil wrote:
> 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.

That reminds us that ideally new configury options are all
advertised in NEWS.

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

GDB is valgrind's inferior, and it is doing something fishy, hence
valgrind is printing the corresponding warning.  IMO, a"false" warning
is a warning that points at something that doesn't really exist,
like all those "variable might be used uninitialized" compiler
warnings when the variable is actually initialized in all the
code paths that use it.  But discussing this is pointless.

> 
> 
>>> +      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])

I was originally going to suggest something like that, but
didn't as I noticed the test doesn't look only for the header,
it also tries to use RUNNING_ON_VALGRIND.

I assumed there'd be a reason for the more complete test,
instead of a simpler and unconditional
AC_CHECK_HEADERS(valgrind/valgrind.h) and then just

#ifdef RUNNING_ON_VALGRIND
 if (RUNNING_ON_VALGRIND)
    return;
#endif

As in, you had found that older valgrind/valgrind.h's didn't
have that RUNNING_ON_VALGRIND macro.  Hence saying the more
general "support".  So as the test as written, it didn't
look 100% correct to point blame at the missing header.
But really I won't argue more about it.  We have bigger
fish to fry.  :-)

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

Thanks.  I have no further comments.

-- 
Pedro Alves

[patch+NEWS] Avoid false valgrind warnings on linux_ptrace_test_ret_to_nx

[Jan Kratochvil]

On Wed, 20 Feb 2013 16:06:21 +0100, Pedro Alves wrote:
> That reminds us that ideally new configury options are all
> advertised in NEWS.

Yes; forgot it.


> I assumed there'd be a reason for the more complete test,
> instead of a simpler and unconditional
> AC_CHECK_HEADERS(valgrind/valgrind.h) and then just
> 
> #ifdef RUNNING_ON_VALGRIND
>  if (RUNNING_ON_VALGRIND)
>     return;
> #endif
> 
> As in, you had found that older valgrind/valgrind.h's didn't
> have that RUNNING_ON_VALGRIND macro.

I haven't found such valgrind.  The reason is there could be a broken
valgrind.h include file installed.  As GDB built before not depending on such
file in the past and now it would break it would be a regression.

Or is somewhere a rule what should and what should not be tested by configure?


Thanks,
Jan


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

	* NEWS (--with-valgrind/--without-valgrind): New.
	* 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/NEWS b/gdb/NEWS
index 0877aa2..3e11229 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -79,6 +79,15 @@ Lynx 178 PowerPC		powerpc-*-lynx*178
   by default.  The --enable-libmcheck/--disable-libmcheck configure
   options allow the user to override that default.
 
+--with-valgrind/--without-valgrind
+  Include special support when GDB itself runs under valgrind.  Without this
+  support valgrind will report a warning on linux_ptrace_test_ret_to_nx which
+  is not a bug in GDB.  Include file <valgrind/valgring.h> needs to be
+  installed for this support.  Include file is autodetected by default.  Using
+  --with-valgrind aborts the configure script if the valgrind include file is
+  not installed.  --without-valgrind omits the support in GDB even if the
+  include file is installed.
+
 * New commands (for set/show, see "New options" below)
 
 catch signal 
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=""

Re: [patch+NEWS] Avoid false valgrind warnings on linux_ptrace_test_ret_to_nx

[Pedro Alves]

On 02/20/2013 03:23 PM, Jan Kratochvil wrote:

>> I assumed there'd be a reason for the more complete test,
>> instead of a simpler and unconditional
>> AC_CHECK_HEADERS(valgrind/valgrind.h) and then just
>>
>> #ifdef RUNNING_ON_VALGRIND
>>  if (RUNNING_ON_VALGRIND)
>>     return;
>> #endif
>>
>> As in, you had found that older valgrind/valgrind.h's didn't
>> have that RUNNING_ON_VALGRIND macro.
> 
> I haven't found such valgrind.  The reason is there could be a broken
> valgrind.h include file installed.  As GDB built before not depending on such
> file in the past and now it would break it would be a regression.
> 
> Or is somewhere a rule what should and what should not be tested by configure?

Hmm.  I don't see how I can reply to that with a one liner, so
apologies in advance for the long text.

The basic configure "rule" is test for features you need.
There's no way to be more strict and more than guidance.  Case by
case judgment needs to be applied.

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.  The applied example of that being the
general rule would be that we don't have autoconf checks for every
function and macro we use from all the headers we include.  From that
perspective, the simpler approach would be better, as we'd have
less code to maintain, and fewer configure knobs to document
and explain to users.

To break the build, either the header would be completely broken, and
would break every program that includes it (unlikely, used by several
programs in a distro), or, RUNNING_ON_VALGRIND would be broken (unlikely,
likewise), or the header would use some symbol that would conflict with
the other headers the linux-ptrace.c file includes before valgrind.h, which
is possible, but is not something that is checked by the new
autoconf check, so the most likely (IMO) scenario would be broken
anyway.

That said, I really don't intend to argue.

-- 
Pedro Alves

Re: [patch+NEWS] Avoid false valgrind warnings on linux_ptrace_test_ret_to_nx

[Jan Kratochvil]

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

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.

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.


Jan

Re: [patch+NEWS] Avoid false valgrind warnings on linux_ptrace_test_ret_to_nx

[Pedro Alves]

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 <palves@redhat.com>
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 <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;
 
+#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 <unistd.h> header file. */
 #undef HAVE_UNISTD_H
 
+/* Define to 1 if you have the <valgrind/valgrind.h> 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 <sys/types.h>
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 <valgrind/valgrind.h> 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)
 

Re: [patch+NEWS] Avoid false valgrind warnings on linux_ptrace_test_ret_to_nx

[Jan Kratochvil]

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.


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 <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)

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.


Jan

Re: [patch+NEWS] Avoid false valgrind warnings on linux_ptrace_test_ret_to_nx

[Pedro Alves]

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 <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)
> 
> 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 <valgrind/valgrind.h> 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 <valgrind/valgrind.h> 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

Re: [patch+NEWS] Avoid false valgrind warnings on linux_ptrace_test_ret_to_nx

[Jan Kratochvil]

On Mon, 25 Feb 2013 19:10:12 +0100, Pedro Alves wrote:
> 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.

This contradicts your earlier mail for the same feature:
	Re: [patch+7.5] Fix assertion fail on PaX gentoo (PR 14564)
	http://sourceware.org/ml/gdb-patches/2012-09/msg00108.html
	Message-ID: <504DF20E.102@redhat.com>

	The linux_test_for_tracefork family of functions seems to warn instead
	of asserting if something unexpected comes out of ptrace/waitpid.
	Might be a good idea for this function too?

You wrote there first:

# It seems like one should be able to make them trigger
# by sending a signal to gdb's process group, or "killall gdb" at
# the wrong time?  E.g., a SIGTERM, a SIGINT, or a SIGSTOP? 

For this specific part I have to disagree, this does not solve the
warning-vs-assert problem, even if SIG* reception does not get handled
correctly still bogus warning gets reported.

If someone sends SIGTERM/SIGINT then it will kill also GDB process itself so
it is not an issue.  SIGSTOP should not be a problem as waitpid does not use
WSTOPPED there.  Just guessing, I have not tried either, though.

And by turning it assert -> warning makes the situation only worse (whether it
is worse or not is IIUC the subject of disagreement).  On assert it gets
automatically bugreport by ABRT and a fix gets possibly automatically
delivered in several days in a system update.  Contrary to it a warning annoys
people for a long time before someone finally bugreports it by hand, usually
omitting various details needed for a fix so it gets all expensive for user
+ developer to investigate the problem first.

So while it is clear what is better for normal modern systems like Fedora
there remains a question what is the direction of upstream GDB.  According to
your 2012 direction errors should be reported "softly".  In your current mail
you prefer rather build failure (instread of omitting the feature unsupported
by the build machine).

While I have discussed here only this specific issue of these two mails this
was always an issue with GDB, what should be an internal error vs. warning
only in some unexpected cases.



> I find the non-x86* argument somewhat compelling.  I do still
> think the mention of valgrind/valgrind.h here:
> 
>   [--with-valgrind was requested but <valgrind/valgrind.h> 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 <valgrind/valgrind.h> was not found])

OK.


> (*) - 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.

I normally look up the mail thread around it.  It would be great to have some
automated tracking of commit vs. its mail (and therefore its mail thread).
The longer commit logs are fine but they do not replace the mail thread.


Jan

Re: [patch+NEWS] Avoid false valgrind warnings on linux_ptrace_test_ret_to_nx

[Andreas Schwab]

Jan Kratochvil <jan.kratochvil@redhat.com> writes:

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

Wouldn't it be enough for gdb to ship with an appropriate suppression
file?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [patch+NEWS] Avoid false valgrind warnings on linux_ptrace_test_ret_to_nx

[Jan Kratochvil]

On Wed, 20 Feb 2013 21:15:07 +0100, Andreas Schwab wrote:
> Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> 
> > +  /* 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
> 
> Wouldn't it be enough for gdb to ship with an appropriate suppression
> file?

Besides some inconveniences of using suppression files primarily there would
still remain the false warning printed by GDB itself:
	warning: linux_ptrace_test_ret_to_nx: PC 0x3806e309 is neither near return address 0x4032000 nor is the return instruction 0x602e33!


Jan

Re: [patch] Avoid false valgrind warnings on linux_ptrace_test_ret_to_nx

[Philippe Waroquiers]

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
> +