Skip to content

Remove/cleanup usage of LAL_NDEBUG/NDEBUG in LAL

Karl Wette requested to merge ANU-CGA/lalsuite:remove-LAL_NDEBUG into master

Description

This MR addresses the use of LAL_NDEBUG/NDEBUG in LALSuite code. In most cases, use of these symbols to disable error messages/checking is removed; the performance cost of these checks is likely minimal, compared to the human cost of debugging obscure errors/segfaults (cf. #573 (closed)). Where legitimate use of such symbols remains, NDEBUG is used. (LAL_NDEBUG used to be controlled by the ./configure option --enable/--disable-debug, but since that option was deprecated in 2016 it has been permanently #undefined in LALConfig.h. It has now been removed entirely.)

LALMalloc.h

The most important use of LAL_NDEBUG/NDEBUG was in LALMalloc.h. This header defined two sets of standard memory allocation functions, LALMalloc() etc., and XLALMalloc() etc. These functions are used equivalently in many places in LALSuite, e.g. memory allocated by LALCalloc() may be freed by XLALFree(). An important difference is that the LALMalloc() functions are replaced with malloc() et al. when NDEBUG is defined, but the XLALMalloc() functions are not. This means that NDEBUG has to be defined consistently, not only when building LAL, but which building any code that links against LAL. The failure for this to happen in the Conda packages (!1929 (merged)) coupled with the mixing of LALMalloc() and XLALMalloc() functions in some codes meant that, in one LALPulsar executable, code allocated by LALCalloc() (which was pointing to calloc()) was passed to XLALFree(), generating an abort from LALCheckMemoryLeaks().

There is no reason to continue to maintain separate LAL and XLAL versions of the standard memory functions, particularly given these functions are widely assumed to be equivalent but may not be (e.g. when NDEBUG is defined). And allowing whether LALMalloc() or malloc() are used based on a macro in a header (e.g. NDEBUG) is dangerous, as one cannot enforce that choice consistently in all code that links against LAL.

This MR addresses these 2 points:

  • LALMalloc.h now just defines XLALMalloc() etc. as functions; LALMalloc() exist only as compatibility macros. (Within LALMalloc.c, the implementations of XLALMalloc() are those of the former LALMalloc() functions.) For each memory function there is a e.g. XLALMallocLong() version with file and line arguments, an XLALMallocShort() version without, and a XLALMalloc() macro which points to XLALMallocLong(..., __FILE__, __LINE__).
  • NDEBUG is no longer used in LALMalloc.h. Instead, if one really wants to disable the LAL memory functions completely (beyond not setting the memory-tracking bits in LAL_DEBUG_LEVEL) one can ./configure with --disable-memory-functions; this compiles LAL with -DLAL_MEMORY_FUNCTIONS_DISABLED which points XLALMalloc() etc. to malloc() etc. inside LALMalloc.c. Calls to malloc() through XLALMalloc() will incur an extra function call (since XLALMalloc() is no longer defined away) but this performance penalty is likely minimal. On the other hard, the use of LAL memory functions through LALMalloc.h is now guaranteed to be consistent across all code which links against a particular liblal.so, preventing the kinds of bugs found by #573 (closed).

Aside from the interface changes, the actual operation of the LAL memory functions (e.g. the memory tracking and overflow checking features) are unchanged.

Other uses of LAL_NDEBUG/NDEBUG

These changes have been split into a separate MR; see !1938 (merged)

Remaining uses of NDEBUG

The only remaining use of NDEBUG after this MR is to enable/disable the LALStatus macro ASSERT(), and code associated with using ASSERT(). Given that ASSERT() is meant to be the LALStatus equivalent of the standard C assert() it's reasonable to use the same macro. (As stated above, LAL_NDEBUG has been removed completely.)

API Changes and Justification

Backwards Compatible Changes

  • This change does not modify any class/function/struct/type definitions in a public C header file or any Python class/function definitions
  • This change adds new classes/functions/structs/types to a public C header file or Python module

Backwards Incompatible Changes

  • This change modifies an existing class/function/struct/type definition in a public C header file or Python module
  • This change removes an existing class/function/struct/type from a public C header file or Python module

This breaks the LALMalloc.h ABI due to the removal of the LALMalloc() et al. functions, which are now just macros.

In particular, XLALFree() is no longer a function, so code which needs a pointer to XLALFree() (e.g. as a destructor function) should instead use XLALFreeShort().

The advantage of preventing obscure bugs like #573 (closed) from arising is IMHO worth the cost of the ABI break.

Review Status

cc @adam-mercer @jolien-creighton

Edited by Karl Wette

Merge request reports