From e26028880790addff43cc5546de1e39214d86a04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Ho=C5=99e=C5=88ovsk=C3=BD?= Date: Sun, 27 Oct 2024 17:08:47 +0100 Subject: [PATCH] Allow disabling use of __builtin_constant_p in internal macros Turns out that even in GCC, the expression in `__builtin_cosntant_p` can end up evaluated and side-effects executed. To allow users to work around this bug, I added a configuration option to disable its use in internal macros. Related to #2925 --- BUILD.bazel | 2 + CMake/CatchConfigOptions.cmake | 1 + docs/configuration.md | 9 +++ src/catch2/catch_user_config.hpp.in | 9 +++ .../internal/catch_compiler_capabilities.hpp | 70 +++++++++++-------- 5 files changed, 60 insertions(+), 31 deletions(-) diff --git a/BUILD.bazel b/BUILD.bazel index c51bf57e..d96c7fa7 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -56,6 +56,8 @@ expand_template( "#cmakedefine CATCH_CONFIG_WCHAR": "", "#cmakedefine CATCH_CONFIG_WINDOWS_CRTDBG": "", "#cmakedefine CATCH_CONFIG_WINDOWS_SEH": "", + "#cmakedefine CATCH_CONFIG_USE_BUILTIN_CONSTANT_P": "", + "#cmakedefine CATCH_CONFIG_NO_USE_BUILTIN_CONSTANT_P": "", }, template = "src/catch2/catch_user_config.hpp.in", ) diff --git a/CMake/CatchConfigOptions.cmake b/CMake/CatchConfigOptions.cmake index 6eae220d..a2f2870b 100644 --- a/CMake/CatchConfigOptions.cmake +++ b/CMake/CatchConfigOptions.cmake @@ -44,6 +44,7 @@ set(_OverridableOptions "WINDOWS_SEH" "GETENV" "EXPERIMENTAL_STATIC_ANALYSIS_SUPPORT" + "USE_BUILTIN_CONSTANT_P" ) foreach(OptionName ${_OverridableOptions}) diff --git a/docs/configuration.md b/docs/configuration.md index 8a3ddfab..f268ba50 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -158,11 +158,14 @@ by using `_NO_` in the macro, e.g. `CATCH_CONFIG_NO_CPP17_UNCAUGHT_EXCEPTIONS`. CATCH_CONFIG_ANDROID_LOGWRITE // Use android's logging system for debug output CATCH_CONFIG_GLOBAL_NEXTAFTER // Use nextafter{,f,l} instead of std::nextafter CATCH_CONFIG_GETENV // System has a working `getenv` + CATCH_CONFIG_USE_BUILTIN_CONSTANT_P // Use __builtin_constant_p to trigger warnings > [`CATCH_CONFIG_ANDROID_LOGWRITE`](https://github.com/catchorg/Catch2/issues/1743) and [`CATCH_CONFIG_GLOBAL_NEXTAFTER`](https://github.com/catchorg/Catch2/pull/1739) were introduced in Catch2 2.10.0 > `CATCH_CONFIG_GETENV` was [introduced](https://github.com/catchorg/Catch2/pull/2562) in Catch2 3.2.0 +> `CATCH_CONFIG_USE_BUILTIN_CONSTANT_P` was introduced in Catch2 vX.Y.Z + Currently Catch enables `CATCH_CONFIG_WINDOWS_SEH` only when compiled with MSVC, because some versions of MinGW do not have the necessary Win32 API support. `CATCH_CONFIG_POSIX_SIGNALS` is on by default, except when Catch is compiled under `Cygwin`, where it is disabled by default (but can be force-enabled by defining `CATCH_CONFIG_POSIX_SIGNALS`). @@ -183,6 +186,12 @@ With the exception of `CATCH_CONFIG_EXPERIMENTAL_REDIRECT`, these toggles can be disabled by using `_NO_` form of the toggle, e.g. `CATCH_CONFIG_NO_WINDOWS_SEH`. +`CATCH_CONFIG_USE_BUILTIN_CONSTANT_P` is ON by default for Clang and GCC +(but as far as possible, not for other compilers masquerading for these +two). However, it can cause bugs where the enclosed code is evaluated, even +though it should not be, e.g. in [#2925](https://github.com/catchorg/Catch2/issues/2925). + + ### `CATCH_CONFIG_FAST_COMPILE` This compile-time flag speeds up compilation of assertion macros by ~20%, by disabling the generation of assertion-local try-catch blocks for diff --git a/src/catch2/catch_user_config.hpp.in b/src/catch2/catch_user_config.hpp.in index 10d61937..3acda688 100644 --- a/src/catch2/catch_user_config.hpp.in +++ b/src/catch2/catch_user_config.hpp.in @@ -178,6 +178,15 @@ #endif +#cmakedefine CATCH_CONFIG_USE_BUILTIN_CONSTANT_P +#cmakedefine CATCH_CONFIG_NO_USE_BUILTIN_CONSTANT_P + +#if defined( CATCH_CONFIG_USE_BUILTIN_CONSTANT_P ) && \ + defined( CATCH_CONFIG_NO_USE_BUILTIN_CONSTANT_P ) +# error Cannot force USE_BUILTIN_CONSTANT_P to both ON and OFF +#endif + + // ------ // Simple toggle defines // their value is never used and they cannot be overridden diff --git a/src/catch2/internal/catch_compiler_capabilities.hpp b/src/catch2/internal/catch_compiler_capabilities.hpp index 7f09da51..d544b00c 100644 --- a/src/catch2/internal/catch_compiler_capabilities.hpp +++ b/src/catch2/internal/catch_compiler_capabilities.hpp @@ -62,7 +62,7 @@ # define CATCH_INTERNAL_SUPPRESS_SHADOW_WARNINGS \ _Pragma( "GCC diagnostic ignored \"-Wshadow\"" ) -# define CATCH_INTERNAL_IGNORE_BUT_WARN(...) (void)__builtin_constant_p(__VA_ARGS__) +# define CATCH_INTERNAL_CONFIG_USE_BUILTIN_CONSTANT_P #endif @@ -86,35 +86,13 @@ // clang-cl defines _MSC_VER as well as __clang__, which could cause the // start/stop internal suppression macros to be double defined. #if defined(__clang__) && !defined(_MSC_VER) - +# define CATCH_INTERNAL_CONFIG_USE_BUILTIN_CONSTANT_P # define CATCH_INTERNAL_START_WARNINGS_SUPPRESSION _Pragma( "clang diagnostic push" ) # define CATCH_INTERNAL_STOP_WARNINGS_SUPPRESSION _Pragma( "clang diagnostic pop" ) - #endif // __clang__ && !_MSC_VER #if defined(__clang__) -// As of this writing, IBM XL's implementation of __builtin_constant_p has a bug -// which results in calls to destructors being emitted for each temporary, -// without a matching initialization. In practice, this can result in something -// like `std::string::~string` being called on an uninitialized value. -// -// For example, this code will likely segfault under IBM XL: -// ``` -// REQUIRE(std::string("12") + "34" == "1234") -// ``` -// -// Similarly, NVHPC's implementation of `__builtin_constant_p` has a bug which -// results in calls to the immediately evaluated lambda expressions to be -// reported as unevaluated lambdas. -// https://developer.nvidia.com/nvidia_bug/3321845. -// -// Therefore, `CATCH_INTERNAL_IGNORE_BUT_WARN` is not implemented. -# if !defined(__ibmxl__) && !defined(__CUDACC__) && !defined( __NVCOMPILER ) -# define CATCH_INTERNAL_IGNORE_BUT_WARN(...) (void)__builtin_constant_p(__VA_ARGS__) /* NOLINT(cppcoreguidelines-pro-type-vararg, hicpp-vararg) */ -# endif - - # define CATCH_INTERNAL_SUPPRESS_GLOBALS_WARNINGS \ _Pragma( "clang diagnostic ignored \"-Wexit-time-destructors\"" ) \ _Pragma( "clang diagnostic ignored \"-Wglobal-constructors\"") @@ -139,6 +117,27 @@ #endif // __clang__ +// As of this writing, IBM XL's implementation of __builtin_constant_p has a bug +// which results in calls to destructors being emitted for each temporary, +// without a matching initialization. In practice, this can result in something +// like `std::string::~string` being called on an uninitialized value. +// +// For example, this code will likely segfault under IBM XL: +// ``` +// REQUIRE(std::string("12") + "34" == "1234") +// ``` +// +// Similarly, NVHPC's implementation of `__builtin_constant_p` has a bug which +// results in calls to the immediately evaluated lambda expressions to be +// reported as unevaluated lambdas. +// https://developer.nvidia.com/nvidia_bug/3321845. +// +// Therefore, `CATCH_INTERNAL_IGNORE_BUT_WARN` is not implemented. +#if defined( __ibmxl__ ) || defined( __CUDACC__ ) || defined( __NVCOMPILER ) +# define CATCH_INTERNAL_CONFIG_NO_USE_BUILTIN_CONSTANT_P +#endif + + //////////////////////////////////////////////////////////////////////////////// // We know some environments not to support full POSIX signals @@ -362,6 +361,22 @@ #endif +// The goal of this macro is to avoid evaluation of the arguments, but +// still have the compiler warn on problems inside... +#if defined( CATCH_INTERNAL_CONFIG_USE_BUILTIN_CONSTANT_P ) && \ + !defined( CATCH_INTERNAL_CONFIG_NO_USE_BUILTIN_CONSTANT_P ) && !defined(CATCH_CONFIG_USE_BUILTIN_CONSTANT_P) +#define CATCH_CONFIG_USE_BUILTIN_CONSTANT_P +#endif + +#if defined( CATCH_CONFIG_USE_BUILTIN_CONSTANT_P ) && \ + !defined( CATCH_CONFIG_NO_USE_BUILTIN_CONSTANT_P ) +# define CATCH_INTERNAL_IGNORE_BUT_WARN( ... ) \ + (void)__builtin_constant_p( __VA_ARGS__ ) /* NOLINT(cppcoreguidelines-pro-type-vararg, \ + hicpp-vararg) */ +#else +# define CATCH_INTERNAL_IGNORE_BUT_WARN( ... ) +#endif + // Even if we do not think the compiler has that warning, we still have // to provide a macro that can be used by the code. #if !defined(CATCH_INTERNAL_START_WARNINGS_SUPPRESSION) @@ -398,13 +413,6 @@ # define CATCH_INTERNAL_SUPPRESS_SHADOW_WARNINGS #endif - -// The goal of this macro is to avoid evaluation of the arguments, but -// still have the compiler warn on problems inside... -#if !defined(CATCH_INTERNAL_IGNORE_BUT_WARN) -# define CATCH_INTERNAL_IGNORE_BUT_WARN(...) -#endif - #if defined(__APPLE__) && defined(__apple_build_version__) && (__clang_major__ < 10) # undef CATCH_INTERNAL_SUPPRESS_UNUSED_TEMPLATE_WARNINGS #elif defined(__clang__) && (__clang_major__ < 5)