Allow debug builds

[jmjatlanta]

PR #464 seems to have had problems during the merge. This PR fixes that.

Also fixes #492

[dimxy]

shouldn't we add --enable-debug param to zcutil/build-mac.sh to activate debug build?

[dimxy]

let's also put CXXFLAGS value in quotes (instead of apostrophes) in build-mac.sh to fix incorrect -I$PREFIX/include expansion (now expanded to -IREFIX/include). This would not affect building as there is another -I$PREFIX/include param so we can also remove the extra one

[dimxy]

I added --enable-debug to configure params and did a rebuild. Now I can access to all variables under lldb. This is wonderful! (previously most of vars were not accessible due to optimisation).
However my komodod failed on start with this:

Process 18196 launched: '/Users/dimxy/repo/komodo-jmj/src/komodod' (x86_64)
terminate called after throwing an instance of 'boost::wrapexcept<boost::lock_error>'
  what():  boost: mutex lock failed in pthread_mutex_lock: Invalid argument
Process 18196 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
    frame #0: 0x00007fff6b62233a libsystem_kernel.dylib`__pthread_kill + 10
libsystem_kernel.dylib`__pthread_kill:
->  0x7fff6b62233a <+10>: jae    0x7fff6b622344            ; <+20>
    0x7fff6b62233c <+12>: movq   %rax, %rdi
    0x7fff6b62233f <+15>: jmp    0x7fff6b61c629            ; cerror_nocancel
    0x7fff6b622344 <+20>: retq   
Target 0: (komodod) stopped.
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
  * frame #0: 0x00007fff6b62233a libsystem_kernel.dylib`__pthread_kill + 10
    frame #1: 0x00007fff6b6dee60 libsystem_pthread.dylib`pthread_kill + 430
    frame #2: 0x00007fff6b5a9808 libsystem_c.dylib`abort + 120
    frame #3: 0x0000000100da58d5 komodod`__gnu_cxx::__verbose_terminate_handler() at vterminate.cc:95:10
    frame #4: 0x0000000100a011b8 komodod`__cxxabiv1::__terminate(handler=<unavailable>)()) at eh_terminate.cc:47:15
    frame #5: 0x0000000100a011e3 komodod`std::terminate() at eh_terminate.cc:57:15
    frame #6: 0x0000000100a0ddcf komodod`::__cxa_throw(obj=<unavailable>, tinfo=<unavailable>, dest=<unavailable>)(void *)) at eh_throw.cc:95:18
    frame #7: 0x000000010000518b komodod`void boost::throw_exception<boost::lock_error>(e=0x00007ffeefbfcfc0) at throw_exception.hpp:70:44
    frame #8: 0x000000010056cf9e komodod`boost::mutex::lock(this=0x00000001016d8300) at mutex.hpp:64:39
    frame #9: 0x00000001005665bb komodod`::push_lock(c=0x000000010160df68, locklocation=0x00007ffeefbfd160, fTry=false) at sync.cpp:135:18
    frame #10: 0x0000000100566960 komodod`EnterCritical(pszName="cs", pszFile="addrman.h", nLine=504, cs=0x000000010160df68, fTry=false) at sync.cpp:166:14
    frame #11: 0x000000010057454b komodod`CMutexLock<AnnotatedMixin<boost::recursive_mutex> >::Enter(this=0x00007ffeefbfd280, pszName="cs", pszFile="addrman.h", nLine=504) at sync.h:127:22
    frame #12: 0x0000000100572cc4 komodod`CMutexLock<AnnotatedMixin<boost::recursive_mutex> >::CMutexLock(this=0x00007ffeefbfd280, mutexIn=0x000000010160df68, pszName="cs", pszFile="addrman.h", nLine=504, fTry=false) at sync.h:153:13
    frame #13: 0x00000001002c5bfe komodod`CAddrMan::Clear(this=0x000000010160df60) at addrman.h:504:9
    frame #14: 0x00000001002c5f03 komodod`CAddrMan::CAddrMan(this=0x000000010160df60) at addrman.h:527:14
    frame #15: 0x000000010030c7f2 komodod`::__static_initialization_and_destruction_0(__initialize_p=1, __priority=65535) at net.cpp:93:10
    frame #16: 0x000000010030d877 komodod`::_GLOBAL__sub_I_net.cpp() at net.cpp:2276:1
    frame #17: 0x00000001039231d3 dyld`ImageLoaderMachO::doModInitFunctions(ImageLoader::LinkContext const&) + 535
    frame #18: 0x00000001039235de dyld`ImageLoaderMachO::doInitialization(ImageLoader::LinkContext const&) + 40
    frame #19: 0x000000010391dffb dyld`ImageLoader::recursiveInitialization(ImageLoader::LinkContext const&, unsigned int, char const*, ImageLoader::InitializerTimingList&, ImageLoader::UninitedUpwards&) + 493
    frame #20: 0x000000010391c0b4 dyld`ImageLoader::processInitializers(ImageLoader::LinkContext const&, unsigned int, ImageLoader::InitializerTimingList&, ImageLoader::UninitedUpwards&) + 188
    frame #21: 0x000000010391c154 dyld`ImageLoader::runInitializers(ImageLoader::LinkContext const&, ImageLoader::InitializerTimingList&) + 82
    frame #22: 0x000000010390a6a8 dyld`dyld::initializeMainExecutable() + 199
    frame #23: 0x000000010390fbba dyld`dyld::_main(macho_header const*, unsigned long, int, char const**, char const**, char const**, unsigned long*) + 6667
    frame #24: 0x0000000103909227 dyld`dyldbootstrap::start(dyld3::MachOLoaded const*, int, char const**, dyld3::MachOLoaded const*, unsigned long*) + 453
    frame #25: 0x0000000103909025 dyld`_dyld_start + 37

This occurs because of calling mutex in CAddrMan::Clear() from the constructor CAddrMan::CAddrMan() for static addrman. To fix this I suggest removing Clear() call from the CAddrMan::CAddrMan() and call it inside AppInit2(): dimxy@b4fdd3c

[jmjatlanta]

shouldn't we add --enable-debug param to zcutil/build-mac.sh to activate debug build?

Good question. The enable-debug param does two things (on all platforms, not just mac). It adds -DDEBUG which will enable assert, and adds the -g3 and -O0 compiler flags.

So the questions become:

• When do we not want to have -DDEBUG? I have seen a comment in the code that says asserts should be left on always. I treat all comments with suspicion, so unsure if that is valid or not. IMO relying on asserts beyond the development phase is bad practice (use exceptions instead IMO).
• Would we ever want to have -DDEBUG and different optimization values? I believe the answer is yes (due to the above). If so, we should probably not use enable-debug, or make it control less than it does today.

My opinion: We should examine all asserts and switch them to exceptions if they should exists beyond the development phase. With the asserts fixed, enable-debug will work as I believe it was intended.

[jmjatlanta]

let's also put CXXFLAGS value in quotes (instead of apostrophes) in build-mac.sh to fix incorrect -I$PREFIX/include expansion (now expanded to -IREFIX/include). This would not affect building as there is another -I$PREFIX/include param so we can also remove the extra one

Fixed the single/double quote issue. I am testing that now. It is in there twice due to the difference between CPPFLAGS and CXXFLAGS. If we remove it from one, there is a small chance that either C or C++ programs will be compiled without the flags we desire. While most scripts combine CPPFLAGS and CXXFLAGS, I would guess some do not. So I recommend leaving them both in there. If you wish to only put them in one, let me know and I'll happily do so.

[jmjatlanta]

This occurs because of calling mutex in CAddrMan::Clear() from the constructor CAddrMan::CAddrMan() for static addrman.

I wonder why this was not caught earlier. Thanks. The constructor does not need the lock, so calling a private function that does not lock. What do you think? See my fix in 8618e6e

[dimxy]

shouldn't we add --enable-debug param to zcutil/build-mac.sh to activate debug build?

Good question. The enable-debug param does two things (on all platforms, not just mac). It adds -DDEBUG which will enable assert, and adds the -g3 and -O0 compiler flags.

So the questions become:

• When do we not want to have -DDEBUG? I have seen a comment in the code that says asserts should be left on always. I treat all comments with suspicion, so unsure if that is valid or not. IMO relying on asserts beyond the development phase is bad practice (use exceptions instead IMO).
• Would we ever want to have -DDEBUG and different optimization values? I believe the answer is yes (due to the above). If so, we should probably not use enable-debug, or make it control less than it does today.

My opinion: We should examine all asserts and switch them to exceptions if they should exists beyond the development phase. With the asserts fixed, enable-debug will work as I believe it was intended.

I think --enable-debug is needed first of all to switch off optimisation which hides variables in the debugger

if test "x$enable_debug" = xyes; then
    CPPFLAGS="$CPPFLAGS -DDEBUG -DDEBUG_LOCKORDER"
    if test "x$GCC" = xyes; then
        CFLAGS="$CFLAGS -g3 -O0"
    fi

    if test "x$GXX" = xyes; then
        CXXFLAGS="$CXXFLAGS -g3 -O0"
    fi
fi

I can also see that --enable-debug seems to not affect the NDEBUG def (disabling asserts) and I do not see we use the DEBUG def somewhere

[jmjatlanta]

I think --enable-debug is needed first of all to switch off optimisation which hides variables in the debugger

That is done in the individual platform configuration files. See depends/hosts/linux.mk. OOPS: forgot about mac. Fixing that now.

I can also see that --enable-debug seems to not affect the NDEBUG def (disabling asserts) and I do not see we use the DEBUG def somewhere

I was confusing -DDEBUG with -DNDEBUG. Thanks for fixing my head.

[dimxy]

That is done in the individual platform configuration files. See depends/hosts/linux.mk. OOPS: forgot about mac. Fixing that now.

does this mean executables will be always built with no optimisation and with most debug info (-g3)? I thought it would be good if we have optimisation on and only some debug info when enable-debug=no and no opt and full debug info if yes

[jmjatlanta]

does this mean executables will be always built with no optimisation and with most debug info (-g3)? I thought it would be good if we have optimisation on and only some debug info when enable-debug=no and no opt and full debug info if yes

As it stands with the latest commit 20e90c6 default optimizations are set at 3 on Linux, 1 on Mac. I am not sure why mac is different. It may benefit us performance-wise to up that.

According to the .mk files, debugging information is not included by default.

In the latest commit I have modified things so that if you run the script in zcutil on Linux or MacOS with --enable-debugging, optimization is turned off, and debugging information is turned on. Previously, I was using DEBUG=1 ./zcutil/build[-platform].sh to do that. But I think standardizing on ./zcutil/build[-platform].sh --enable-debugging makes things clearer for everyone.

As for including debug information in the executables, we can certainly do that. We can include debugging information globally (modify the .mk), or only certain libraries/executables (modify the appropriate Makefile.am).

1. Do you like the idea of standardizing on --enable-debugging?
2. Should debugging information be included in optimized executables? If so, to what degree?

[dimxy]

• Do you like the idea of standardizing on --enable-debugging?

yes I really like this

• Should debugging information be included in optimized executables? If so, to what degree?

I think we need some debug info in release executables (to diagnose crashes) and I could see the basic option was '-g' (and seems the same is in the bitcoin sources https://github.com/bitcoin/bitcoin/blob/807169e10b4a18324356ed6ee4d69587b96a7c70/configure.ac#L357)

[dimxy]

I tried to build with enable-debugging and in the output it is printed:

Options used to compile and link:
  with wallet   = yes
  with proton   = no
  with zmq      = yes
  with test     = yes
  debug enabled = no   <-------
  werror        = no

I think is because in the configure.ac the 'enable-debug' option is checked and you changed it to 'enable-debugging'

[jmjatlanta]

I think is because in the configure.ac the 'enable-debug' option is checked and you changed it to 'enable-debugging'

Fixed. --enable-debug is now the standard across the project. The latest commit fixes the inaccurate output.