Re: [patch] Avoid false valgrind warnings on linux_ptrace_test_ret_to_nx

[Pedro Alves <palves@redhat.com>]

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