diff mbox series

[1/2] gcc: make Valgrind errors fatal during bootstrap

Message ID 2408cb8d5bd12befb566b0e96056df6bd62a33a8.1727923173.git.sam@gentoo.org
State New
Headers show
Series Valgrind checking improvements | expand

Commit Message

Sam James Oct. 3, 2024, 2:39 a.m. UTC 
Valgrind doesn't error out by default which means bootstrap issues like
in PR116945 can easily be missed: pass --exit-errorcode=1 to handle this.

While here, also set --trace-children=yes to cover child processes
of tools invoked during the build.

Note that this only handles tools invoke during the build, it doesn't
cover everything that --enable-checking=valgrind does.

gcc/ChangeLog:
	PR other/116945
	PR other/116947

	* configure: Regenerate.
	* configure.ac (valgrind_cmd): Pass additional options.
---
 gcc/configure    | 2 +-
 gcc/configure.ac | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Jeff Law Oct. 4, 2024, 2:08 p.m. UTC | #1 
On 10/2/24 8:39 PM, Sam James wrote:
> Valgrind doesn't error out by default which means bootstrap issues like
> in PR116945 can easily be missed: pass --exit-errorcode=1 to handle this.
> 
> While here, also set --trace-children=yes to cover child processes
> of tools invoked during the build.
> 
> Note that this only handles tools invoke during the build, it doesn't
> cover everything that --enable-checking=valgrind does.
> 
> gcc/ChangeLog:
> 	PR other/116945
> 	PR other/116947
> 
> 	* configure: Regenerate.
> 	* configure.ac (valgrind_cmd): Pass additional options.
But is this going to cause all bootstraps with Ada to fail?  That's how 
I read 116945 which was closed as WONTFIX.  Or am I mis-interpreting 
that BZ and its interaction with this patch?

jeff
Sam James Oct. 5, 2024, 5:07 a.m. UTC | #2 
Jeff Law <jeffreyalaw@gmail.com> writes:

> On 10/2/24 8:39 PM, Sam James wrote:
>> Valgrind doesn't error out by default which means bootstrap issues like
>> in PR116945 can easily be missed: pass --exit-errorcode=1 to handle this.
>> While here, also set --trace-children=yes to cover child processes
>> of tools invoked during the build.
>> Note that this only handles tools invoke during the build, it
>> doesn't
>> cover everything that --enable-checking=valgrind does.
>> gcc/ChangeLog:
>> 	PR other/116945
>> 	PR other/116947
>> 	* configure: Regenerate.
>> 	* configure.ac (valgrind_cmd): Pass additional options.
> But is this going to cause all bootstraps with Ada to fail?  That's
> how I read 116945 which was closed as WONTFIX.  Or am I
> mis-interpreting that BZ and its interaction with this patch?

No, you're right, I consider this on pause unless/until we figure out
that bug -- I'm speaking with mjw about some ideas.

>
> jeff

thanks,
sam
Mark Wielaard Oct. 5, 2024, 10:55 a.m. UTC | #3 
Hi (adding Philippe to CC who knows a bit more about Valgrind vs Ada),

On Sat, Oct 05, 2024 at 06:07:27AM +0100, Sam James wrote:
> Jeff Law <jeffreyalaw@gmail.com> writes:
> 
> > On 10/2/24 8:39 PM, Sam James wrote:
> >> Valgrind doesn't error out by default which means bootstrap issues like
> >> in PR116945 can easily be missed: pass --exit-errorcode=1 to handle this.
> >> While here, also set --trace-children=yes to cover child processes
> >> of tools invoked during the build.
> >> Note that this only handles tools invoke during the build, it
> >> doesn't
> >> cover everything that --enable-checking=valgrind does.
> >> gcc/ChangeLog:
> >> 	PR other/116945
> >> 	PR other/116947
> >> 	* configure: Regenerate.
> >> 	* configure.ac (valgrind_cmd): Pass additional options.
> > But is this going to cause all bootstraps with Ada to fail?  That's
> > how I read 116945 which was closed as WONTFIX.  Or am I
> > mis-interpreting that BZ and its interaction with this patch?
> 
> No, you're right, I consider this on pause unless/until we figure out
> that bug -- I'm speaking with mjw about some ideas.

The problem as explained in the bug
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116945 is that we don't
understand what code is generated for the guard that checks whether
the value is undefined. I assume the 'Valid guard relies at least on
(partially) defined bits (but maybe it really is not deterministic?)

Depending on what the generated code actually looks like we can either
try to get memcheck more accurate, add valgrind annotations like in
ggc (but do we have an Ada variant of the valgrind.h macros?) or add a
suppression (where another wrinkle is that Valgrind doesn't seem to
demangle Ada symbols, https://bugs.kde.org/show_bug.cgi?id=445235).

Cheers,

Mark
diff mbox series

Patch

diff --git a/gcc/configure b/gcc/configure
index 5acc42c1e4d9..0a999e609c92 100755
--- a/gcc/configure
+++ b/gcc/configure
@@ -7751,7 +7751,7 @@  fi
     as_fn_error $? "*** Cannot find valgrind" "$LINENO" 5
   fi
   valgrind_path_defines=-DVALGRIND_PATH='\"'$valgrind_path'\"'
-  valgrind_command="$valgrind_path -q"
+  valgrind_command="$valgrind_path --trace-children=yes --error-exitcode=1 -q"
 
 $as_echo "#define ENABLE_VALGRIND_CHECKING 1" >>confdefs.h
 
diff --git a/gcc/configure.ac b/gcc/configure.ac
index 23f4884eff9e..9c52061c981c 100644
--- a/gcc/configure.ac
+++ b/gcc/configure.ac
@@ -781,7 +781,7 @@  if test x$ac_valgrind_checking != x ; then
     AC_MSG_ERROR([*** Cannot find valgrind])
   fi
   valgrind_path_defines=-DVALGRIND_PATH='\"'$valgrind_path'\"'
-  valgrind_command="$valgrind_path -q"
+  valgrind_command="$valgrind_path --trace-children=yes --error-exitcode=1 -q"
   AC_DEFINE(ENABLE_VALGRIND_CHECKING, 1,
 [Define if you want to run subprograms and generated programs
    through valgrind (a memory checker).  This is extremely expensive.])