Hi Mark, On Fri, Oct 16, 2026 at 10:34 AM Mark Wielaard <[email protected]> wrote: > > 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).
Thanks, I've merged the patch with #ifdef USE_VG_ANNOTATIONS. Aaron > > > #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 >
