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

[Pedro Alves <palves@redhat.com>]

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)