From 4af7a8071c09fe61f460feb0c18dd23a2e24b57e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Valverde?= Date: Sat, 16 Sep 2023 11:59:18 +0100 Subject: [PATCH] CMake: Rework assertions and add dependency on NDEBUG Separate enabling assertions from WS_DEBUG. Use NDEBUG if ENABLE_ASSERT is not defined, to conform to CMake conventions for the build type. Misc assertion header enhancements. This partially reverts 25d4a099f7c5578d1d43f66f8e9ea567f698cc4b. --- CMakeLists.txt | 1 + CMakeOptions.txt | 5 ++-- docbook/wsdg_src/wsdg_sources.adoc | 15 +++++++----- wsutil/version_info.c | 6 ----- wsutil/ws_assert.h | 39 ++++++++++++++++-------------- 5 files changed, 34 insertions(+), 32 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index e1e63e1fab..043a4999c4 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -517,6 +517,7 @@ set_property(DIRECTORY "G_DISABLE_SINGLE_INCLUDES" $<$,$>:WS_DEBUG> $<$,$>,$>:WS_DEBUG_UTF_8> + $<$:ENABLE_ASSERT> ) if(WIN32) diff --git a/CMakeOptions.txt b/CMakeOptions.txt index af09742bb8..2358031d4f 100644 --- a/CMakeOptions.txt +++ b/CMakeOptions.txt @@ -50,11 +50,12 @@ option(BUILD_mmdbresolve "Build MaxMind DB resolver" ON) option(BUILD_fuzzshark "Build fuzzshark" OFF) option(ENABLE_WERROR "Treat warnings as errors" ON) -# Debugging is enabled for "Debug" and "RelWithDebInfo" build types. +# Debugging is enabled for "Debug" build type. option(ENABLE_DEBUG "Enable debugging for all build configurations" OFF) -# UTF-8 debugging is enabled for "Debug" and "RelWithDebInfo" build types. option(ENABLE_DEBUG_UTF_8 "Enable UTF-8 sanity checks (requires ENABLE_DEBUG)" ON) option(ENABLE_DEBUG_A2W "Enable line directive from .cnf file" OFF) +# Assertions are enabled for "Debug" and "RelWithDebInfo" build types. +option(ENABLE_ASSERT "Enable assertions for all build configurations" OFF) option(ENABLE_CCACHE "Speed up compiling and linking using ccache if possible" OFF) option(DISABLE_FRAME_LARGER_THAN_WARNING "Disable warning if the size of a function frame is large" OFF) diff --git a/docbook/wsdg_src/wsdg_sources.adoc b/docbook/wsdg_src/wsdg_sources.adoc index 22f497cea6..9e2a3c101c 100644 --- a/docbook/wsdg_src/wsdg_sources.adoc +++ b/docbook/wsdg_src/wsdg_sources.adoc @@ -235,29 +235,32 @@ CMake can compile Wireshark for several different build types: |`RelWithDebInfo` |`-O2 -g` |Default, build with default optimizations and generate debug symbols. -Does not enable extra debugging code or debug level logs +Enables assertions and disables debug level logs |`Debug` |`-O0 -g -DWS_DEBUG -DWS_DEBUG_UTF_8` -|For debugging, no optimization. Enables extra debugging code +|For development, no optimization. Enables assertions +and debug level logs |`Release` |`-O3 -DNDEBUG` -|Optimized for speed, no debug symbols or debug level logs +|Optimized for speed, no debug symbols or debug level logs or assertions |`MinSizeRel` |`-Os -DNDEBUG` -|Optimized for size, no debug symbols or debug level logs +|Optimized for size, no debug symbols or debug level logs or assertions |=== The default is `RelWithDebInfo`, which provides a good compromise of some optimization (`-O2`) along with including debug symbols (`-g`) for release builds. For normal development coding you probably want to be using `Debug` build type or set -DENABLE_DEBUG=On, to enable full -<> and all assertions. +<> and debug code. CMake will automatically add the -DNDEBUG option to certain build -types but this macro is not used very much in Wireshark code, if at all. +types. This macro is used to disable assertions but it can be overruled +using ENABLE_ASSERT, which can be used to unconditionally enable assertions +if defined. To change the build type, set the CMake variable `CMAKE_BUILD_TYPE`, e.g.: diff --git a/wsutil/version_info.c b/wsutil/version_info.c index e22f247d06..0294358aca 100644 --- a/wsutil/version_info.c +++ b/wsutil/version_info.c @@ -212,12 +212,6 @@ get_compiled_version_info(gather_feature_func gather_compile) #ifdef WS_DEBUG g_string_append(str, ", debug build"); -#else - g_string_append(str, ", release build"); -#endif - -#ifdef WS_DEBUG_UTF_8 - g_string_append(str, " (+utf8)"); #endif g_string_append(str, "."); diff --git a/wsutil/ws_assert.h b/wsutil/ws_assert.h index b7727cbf4d..f4880fd8b3 100644 --- a/wsutil/ws_assert.h +++ b/wsutil/ws_assert.h @@ -17,10 +17,12 @@ #include #include -#ifdef WS_DEBUG -#define _ASSERT_ENABLED true +#if defined(ENABLE_ASSERT) +#define WS_ASSERT_ENABLED 1 +#elif defined(NDEBUG) +#define WS_ASSERT_ENABLED 0 #else -#define _ASSERT_ENABLED false +#define WS_ASSERT_ENABLED 1 #endif #ifdef __cplusplus @@ -28,32 +30,32 @@ extern "C" { #endif /* __cplusplus */ /* - * We don't want to execute the expression with !WS_DEBUG because + * We don't want to execute the expression without assertions because * it might be time and space costly and the goal here is to optimize for - * !WS_DEBUG. However removing it completely is not good enough + * that case. However removing it completely is not good enough * because it might generate many unused variable warnings. So we use * if (false) and let the compiler optimize away the dead execution branch. */ -#define _ASSERT_IF_ACTIVE(active, expr) \ +#define ws_assert_if_active(active, expr) \ do { \ if ((active) && !(expr)) \ ws_error("assertion failed: %s", #expr); \ } while (0) /* - * ws_abort_if_fail() is not conditional on WS_DEBUG. + * ws_abort_if_fail() is not conditional on having assertions enabled. * Usually used to appease a static analyzer. */ #define ws_abort_if_fail(expr) \ - _ASSERT_IF_ACTIVE(true, expr) + ws_assert_if_active(true, expr) /* * ws_assert() cannot produce side effects, otherwise code will - * behave differently because of WS_DEBUG, and probably introduce - * some difficult to track bugs. + * behave differently because of having assertions enabled/disabled, and + * probably introduce some difficult to track bugs. */ #define ws_assert(expr) \ - _ASSERT_IF_ACTIVE(_ASSERT_ENABLED, expr) + ws_assert_if_active(WS_ASSERT_ENABLED, expr) #define ws_assert_streq(s1, s2) \ @@ -62,7 +64,7 @@ extern "C" { #define ws_assert_utf8(str, len) \ do { \ const char *__assert_endptr; \ - if (_ASSERT_ENABLED && \ + if (WS_ASSERT_ENABLED && \ !g_utf8_validate(str, len, &__assert_endptr)) { \ ws_log_utf8_full(LOG_DOMAIN_UTF_8, LOG_LEVEL_ERROR, \ __FILE__, __LINE__, __func__, \ @@ -71,7 +73,8 @@ extern "C" { } while (0) /* - * We don't want to disable ws_assert_not_reached() with WS_DEBUG. + * We don't want to disable ws_assert_not_reached() with (optional) assertions + * disabled. * That would blast compiler warnings everywhere for no benefit, not * even a miniscule performance gain. Reaching this function is always * a programming error and will unconditionally abort execution. @@ -99,21 +102,21 @@ extern "C" { */ #define ws_warn_badarg(str) \ - ws_log_full(LOG_DOMAIN_EINVAL, LOG_LEVEL_INFO, \ + ws_log_full(LOG_DOMAIN_EINVAL, LOG_LEVEL_WARNING, \ __FILE__, __LINE__, __func__, \ - "bad argument: %s", str) + "invalid argument: %s", str) #define ws_return_str_if(expr, scope) \ do { \ - if (expr) { \ + if (WS_ASSERT_ENABLED && (expr)) { \ ws_warn_badarg(#expr); \ - return wmem_strdup(scope, "(invalid argument)"); \ + return wmem_strdup_printf(scope, "(invalid argument: %s)", #expr); \ } \ } while (0) #define ws_return_val_if(expr, val) \ do { \ - if (expr) { \ + if (WS_ASSERT_ENABLED && (expr)) { \ ws_warn_badarg(#expr); \ return (val); \ } \