Hi, On Fri, 2024-08-02 at 19:38 -0400, Aaron Merey wrote: > From: Heather McIntyre <h...@rice.edu> > > * tests/eu_search_cfi.c: New file. > * tests/eu_search_die.c: New file. > * tests/eu_search_lines.c: New file. > * tests/eu_search_macros.c: New file. > * tests/run-eu-search-tests.sh: New test. > * tests/Makefile.am: Add USE_LOCKS condition for -pthread. > (check_PROGRAMS): Add eu_search_cfi, eu_search_die, > eu_search_lines, and eu_search_macros. > (TESTS): Add run-eu-search-tests.sh > (eu_search_cfi_LDADD): New variable. > (eu_search_die_LDADD): New variable. > (eu_search_lines_LDADD): New variable. > (eu_search_macros_LDADD): New variable. > (eu_search_cfi_LDFLAGS): New variable. > Add -pthread if USE_LOCKS is not defined. > (eu_search_die_LDFLAGS): Likewise. > (eu_search_lines_LDFLAGS): Likewise. > (eu_search_macros_LDFLAGS): Likewise. > > Signed-off-by: Heather S. McIntyre <h...@rice.edu> > Signed-off-by: Aaron Merey <ame...@redhat.com> > Signed-off-by: Mark Wielaard <m...@klomp.org> > > --- > No changes between v2 and v3. > > The elfutils testsuite (including the tests added in this patch) passes > for me on the sourceware buildbots. However the buildbots do not configure > elfutils with --enable-thread-safety. I think we should include this > configure option when the buildbots run elfutils tests.
yes, once these new tests go in we should add a buildbot build that uses --enable-thread-safety. I am sorry to ask for even more work, but could you split this into 4 separate tests/patches with separate run wrappers? That makes things easier to review. It makes it possible to run them in parallel. And provides easier to interpret test results. Also could you fixup the indentation of the tests so it uses the formatting as in other code, specifically the wide indentation makes things slightly less readable. The run-eu-search-tests.sh wrapper is slightly "wrong". The way it runs the tests under valgrind --tool=helgrind doesn't use/set the test environment (specifically LD_LIBRARY_PATH) so it is actually testing the tests against the installed libelf and libdw instead of the just build libraries. Also it could just run the tests without valgrind being available. The tests do various useful sanity tests themselves that are useful even without extra valgrind checking. Or if valgrind is available and the user configured using --enable- valgrind then running the tests under valgrind memcheck would still be useful. I suggest adding a new configure option --enable-helgrind and handle that like --enable-valgrind in the tests/Makefile.am. So it would do something like: if USE_HELGRIND valgrind_cmd=valgrind -q --tool=helgrind --error-exitcode=1 endif So all tests are run under helgrind (it wouldn't do much for the non- threaded testcases, but it would be consistent and makes it so individual new tests don't need any special tricks to be tested under helgrind). Thanks, Mark
