Re: [patch] Avoid false valgrind warnings on linux_ptrace_test_ret_to_nx

[Pedro Alves <palves@redhat.com>]

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