NDEBUG flag breaks binary compatibility, which can cause mysterious crashes

[kosak]

Code in immer/config.hpp causes IMMER_TAGGED_NODE to be set to 0 or 1 according to whether NDEBUG is defined. This in turn affects the definition of immer::detail::rbts::node; in particular whether or not impl_data_t contains a kind_t kind field.

This can cause problems if user code compiled with NDEBUG set is linked to other code compiled without NDEBUG, because the Immer data structures are not binary compatible between these two versions.

This situation can arise in a complicated build system where libraries are built by different build steps. Even though ideally one should try to have a coherent set of flags in all cases, I think people expect NDEBUG to be harmless, at most controlling whether assertions are enabled or perhaps turning on some log messages. In my opinion it would be surprising to learn that NDEBUG would break binary compatibility. In my company's case we had Rcpp building some of our code and it used an NDEBUG flag in its build steps.

In the example below, the code in main.cpp doesn't even call the code in util.cpp, but util.cpp poisons the binary, causing main.cpp to crash. Steps to reproduce:

// main.cpp
#include "immer/vector.hpp"

int main() {
  immer::vector<int> v0;
  for (size_t i = 0; i < 100; ++i) {
    v0 = v0.push_back(13);
  }
}

// util.cpp
#include "immer/vector.hpp"

void notcalled(const immer::vector<int> &v0) {
  (void)v0[0];
}

Command:

g++ -c -I. -DNDEBUG main.cpp
g++ -c -I. util.cpp
g++ -o doit util.o main.o 
./doit
doit: ./immer/detail/rbts/node.hpp:175: immer::detail::rbts::node<T, MemoryPolicy, B, BL>::node_t** immer::detail::rbts::node<T, MemoryPolicy, B, BL>::inner() [with T = int; MemoryPolicy = immer::memory_policy<immer::free_list_heap_policy<immer::cpp_heap>, immer::refcount_policy, immer::spinlock_policy>; unsigned int B = 5; unsigned int BL = 6; immer::detail::rbts::node<T, MemoryPolicy, B, BL>::node_t = immer::detail::rbts::node<int, immer::memory_policy<immer::free_list_heap_policy<immer::cpp_heap>, immer::refcount_policy, immer::spinlock_policy>, 5, 6>]: Assertion `kind() == kind_t::inner' failed.
Aborted (core dumped)

This can be a tricky thing to diagnose and debug. In my opinion, Immer ought to do one of two things. Either:

1. NDEBUG should not control IMMER_TAGGED_NODE (nor IMMER_ENABLE_DEBUG_SIZE_HEAP); if the programmer wants these behaviors they should have to set those flags explicitly and not get them via NDEBUG.

or:

2. These flags should change the namespace or classnames of all affected code (i.e. both the node data structure and any code referencing the node data structure). This would have the nice property that the two versions could coexist side-by-side and in the worst case an inconsistency would lead to a link error rather than a mysterious crash.

If you agree, I would be happy to contribute code for item 1 or 2, once I know what your preference is.

[arximboldi]

Hi @kosak!

Good find. I think you're right. I'm happy with option 1. When you make the PR, maybe you can add a CMake flag (-DImmer_DEBUG_TOOLS) or alike to enable those easily during development? Also remember to enable it in test.yml so in Debug builds that in CI it is still enabled.