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

[Pedro Alves <palves at redhat dot com>]

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