Skip to content

Commit

Permalink
Allow disabling use of __builtin_constant_p in internal macros
Browse files Browse the repository at this point in the history
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 catchorg#2925
  • Loading branch information
horenmar committed Oct 27, 2024
1 parent 7c2e1fb commit e260288
Show file tree
Hide file tree
Showing 5 changed files with 60 additions and 31 deletions.
2 changes: 2 additions & 0 deletions BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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",
)
Expand Down
1 change: 1 addition & 0 deletions CMake/CatchConfigOptions.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ set(_OverridableOptions
"WINDOWS_SEH"
"GETENV"
"EXPERIMENTAL_STATIC_ANALYSIS_SUPPORT"
"USE_BUILTIN_CONSTANT_P"
)

foreach(OptionName ${_OverridableOptions})
Expand Down
9 changes: 9 additions & 0 deletions docs/configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`).
Expand All @@ -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
Expand Down
9 changes: 9 additions & 0 deletions src/catch2/catch_user_config.hpp.in
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
70 changes: 39 additions & 31 deletions src/catch2/internal/catch_compiler_capabilities.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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\"")
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit e260288

Please sign in to comment.