Re: [patch] Avoid false valgrind warnings on linux_ptrace_test_ret_to_nx

Pedro Alves
2013-02-19T19:11:43Z

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