Hi Aaron, On Wed, 2026-09-30 at 15:05 -0400, Aaron Merey wrote: > --enable-valgrind-annotations is intended to define macro > USE_VG_ANNOTATIONS but configure.ac is missing the needed AC_DEFINE.
Oops, which means there is no C level define. > USE_VG_ANNOTATIONS also guards an `#include <helgrind.h>` statement > which should instead be `#include <valgrind/helgrind.h>`. Yes, that is normally where helgrind.h is installed. > Fix this by adding an AC_DEFINE for USE_VG_ANNOTATIONS and using > `#include <valgrind/helgrind.h>`. That looks correct, just surprised this was broken and nobody noticed? > Also move this #include and the stub ANNOTATE_* macros from > libdw/libdw_alloc.c to lib/locks.h so they can be used outside of > libdw_alloc.c. A stub for VALGRIND_HG_DISABLE_CHECKING has been added > as well. Yes, that seems a better place. > The --with-valgrind include directory is now passed to CPPFLAGS instead > of CFLAGS since valgrind/helgrind.h may be included in C++ source files > as well. CPPFLAGS is now printed in the configure summary as well. Nice. Looks good, but one small nitpick below. > Signed-off-by: Aaron Merey <[email protected]> > --- > configure.ac | 9 ++++++--- > lib/locks.h | 8 ++++++++ > libdw/libdw_alloc.c | 6 ------ > 3 files changed, 14 insertions(+), 9 deletions(-) > > diff --git a/configure.ac b/configure.ac > index b0d1b48d..f6436ba7 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -426,13 +426,13 @@ AC_ARG_WITH([valgrind], > AS_HELP_STRING([--with-valgrind],[include directory for Valgrind headers]), > [with_valgrind_headers=$withval], [with_valgrind_headers=no]) > if test "x$with_valgrind_headers" != xno; then > - save_CFLAGS="$CFLAGS" > - CFLAGS="$CFLAGS -I$with_valgrind_headers" > + save_CPPFLAGS="$CPPFLAGS" > + CPPFLAGS="$CPPFLAGS -I$with_valgrind_headers" > AC_COMPILE_IFELSE([AC_LANG_SOURCE([[ > #include <valgrind/valgrind.h> > int main() { return 0; } > ]])], [ HAVE_VALGRIND_HEADERS="yes" > - CFLAGS="$save_CFLAGS -I$with_valgrind_headers" ], > + CPPFLAGS="$save_CPPFLAGS -I$with_valgrind_headers" ], > [ AC_MSG_ERROR([invalid valgrind include directory: > $with_valgrind_headers]) ]) > fi Save and set CPPFLAGS instead of CFLAGS. Good. > @@ -454,6 +454,8 @@ if test "$use_vg_annotations" = yes; then > ]])], [ AC_MSG_RESULT([yes]) ], > [ AC_MSG_ERROR([valgrind annotations requested but no headers > are available]) ]) > fi > + AC_DEFINE([USE_VG_ANNOTATIONS], [1], > + [Defined if valgrind annotations are enabled.]) > fi > AM_CONDITIONAL(USE_VG_ANNOTATIONS, test "$use_vg_annotations" = yes) Ack, add AC_DEFINE. > @@ -1093,6 +1095,7 @@ AC_MSG_NOTICE([ > > CFLAGS=${CFLAGS} > CXXFLAGS=${CXXFLAGS} > + CPPFLAGS=${CPPFLAGS} > > RECOMMENDED FEATURES (should all be yes) > gzip support : ${with_zlib} And also print CPPFLAGS at the end. > diff --git a/lib/locks.h b/lib/locks.h > index 9c17eca7..9b303092 100644 > --- a/lib/locks.h > +++ b/lib/locks.h > @@ -29,6 +29,14 @@ > #ifndef LOCKS_H > #define LOCKS_H 1 > > +#if USE_VG_ANNOTATIONS == 1 > +# include <valgrind/helgrind.h> > +#else > +# define ANNOTATE_HAPPENS_BEFORE(X) > +# define ANNOTATE_HAPPENS_AFTER(X) > +# define VALGRIND_HG_DISABLE_CHECKING(X, Y) > +#endif This is what was used in libdw_alloc, but #ifdef USE_VG_ANNOTATIONS would be more idiomatic (AC_DEFINE won't define the variable if the condition is false). > #ifdef USE_LOCKS > # include <pthread.h> > # include <assert.h> > diff --git a/libdw/libdw_alloc.c b/libdw/libdw_alloc.c > index 86c59908..d147d93d 100644 > --- a/libdw/libdw_alloc.c > +++ b/libdw/libdw_alloc.c > @@ -36,12 +36,6 @@ > #include "libdwP.h" > #include "system.h" > #include <stdatomic.h> > -#if USE_VG_ANNOTATIONS == 1 > -#include <helgrind.h> > -#else > -#define ANNOTATE_HAPPENS_BEFORE(X) > -#define ANNOTATE_HAPPENS_AFTER(X) > -#endif > > #define THREAD_ID_UNSET ((size_t) -1) > static __thread size_t thread_id = THREAD_ID_UNSET; And remove here. Looks good, Mark
