torognes / vsearch

Versatile open-source tool for microbiome analysis
Other
643 stars 123 forks source link

add a DEBUG compilation option #528

Closed frederic-mahe closed 10 months ago

frederic-mahe commented 11 months ago

I would like to start adding assertion tests to vsearch's code (<cassert>). I've done that for other projects using simple make files, but never for a project using automake. I would need someone (maybe @xflouris ?) to confirm point 1 (see below) and to review or do the required changes to perform point 2.

1) I am under the impression that, by default, assertions are skipped and do not appear in production binaries when generating make files with automake and compiling with normal options. If that is correct, then assertions currently have no impact on performances, and I can add as many as I want.

2) If I want to actually test the assertions at runtime, I think I need to modify the configure.ac and the Makefile.am. ChatGPT's suggested the following modifications.

configure.ac:

# Check for --enable-debug option
AC_ARG_ENABLE([debug],
  [AS_HELP_STRING([--enable-debug], [Enable debug build])],
  [enable_debug=$enableval],
  [enable_debug=no])

# Define AM_CONDITIONAL for debug
AM_CONDITIONAL([ENABLE_DEBUG], [test "x$enable_debug" = "xyes"])

Makefile.am:

# Conditionally set NDEBUG based on ENABLE_DEBUG
if ENABLE_DEBUG
AM_CPPFLAGS += -UNDEBUG
else
AM_CPPFLAGS += -DNDEBUG
endif

With the above setup, when you run ./configure, you can enable the debug build by using the --enable-debug option. For example:

./configure --enable-debug
torognes commented 10 months ago

I've now added the code above (with minor changes). The resulting binary will only include assertions if configure has been run with the --enable-debug option, otherwise (and by default) assertions are ignored.

Previously, before this change, assertions would have been included. But they were never used.

frederic-mahe commented 10 months ago

I've now added the code above (with minor changes). The resulting binary will only include assertions if configure has been run with the --enable-debug option, otherwise (and by default) assertions are ignored.

Thanks Torbjørn, however, I cannot get that to work on my machine. Assertions seem to be always present, and executed. Starting from a fresh vsearch install:

cd /tmp/
git clone https://github.com/torognes/vsearch.git
cd ./vsearch/

Manually modify vsearch.cc: add #include <cassert>, add assert(1 == 0); to main()

./autogen.sh
./configure CFLAGS="-O3" CXXFLAGS="-O3"
make -j
./bin/vsearch -h

Returns:

# vsearch: vsearch.cc:5721: int main(int, char**): Assertion `1 == 0' failed.

./configure --enable-debug is not used, but assertions are present in the binary and executed.

torognes commented 10 months ago

I am sorry, but I had forgotten to uncomment the statements that define or undefined the NDEBUG flag in Makefile.am. Fixed now in latest commit.

I also moved the inclusion of the cassert header file to vsearch.h. I like to put all inclusions there.

frederic-mahe commented 10 months ago

Great! It now works as expected. For future reference, here is my current debugging pipeline:

cd /tmp/

DEBUG_FLAGS="-Og -ggdb"

git clone https://github.com/torognes/vsearch.git

cd ./vsearch/
./autogen.sh
./configure --enable-debug CFLAGS="${DEBUG_FLAGS}" CXXFLAGS="${DEBUG_FLAGS}"
make -j

In Emacs, launch gdb:

gdb -i=mi /tmp/vsearch/bin/vsearch
gdb-many-windows

Weirdly, binary size decreases when adding debugging options:

But that's a mystery for another day.