Skip to content

Commit e260288

Browse files
committed
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
1 parent 7c2e1fb commit e260288

File tree

5 files changed

+60
-31
lines changed

5 files changed

+60
-31
lines changed

BUILD.bazel

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,8 @@ expand_template(
5656
"#cmakedefine CATCH_CONFIG_WCHAR": "",
5757
"#cmakedefine CATCH_CONFIG_WINDOWS_CRTDBG": "",
5858
"#cmakedefine CATCH_CONFIG_WINDOWS_SEH": "",
59+
"#cmakedefine CATCH_CONFIG_USE_BUILTIN_CONSTANT_P": "",
60+
"#cmakedefine CATCH_CONFIG_NO_USE_BUILTIN_CONSTANT_P": "",
5961
},
6062
template = "src/catch2/catch_user_config.hpp.in",
6163
)

CMake/CatchConfigOptions.cmake

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ set(_OverridableOptions
4444
"WINDOWS_SEH"
4545
"GETENV"
4646
"EXPERIMENTAL_STATIC_ANALYSIS_SUPPORT"
47+
"USE_BUILTIN_CONSTANT_P"
4748
)
4849

4950
foreach(OptionName ${_OverridableOptions})

docs/configuration.md

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,11 +158,14 @@ by using `_NO_` in the macro, e.g. `CATCH_CONFIG_NO_CPP17_UNCAUGHT_EXCEPTIONS`.
158158
CATCH_CONFIG_ANDROID_LOGWRITE // Use android's logging system for debug output
159159
CATCH_CONFIG_GLOBAL_NEXTAFTER // Use nextafter{,f,l} instead of std::nextafter
160160
CATCH_CONFIG_GETENV // System has a working `getenv`
161+
CATCH_CONFIG_USE_BUILTIN_CONSTANT_P // Use __builtin_constant_p to trigger warnings
161162

162163
> [`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
163164
164165
> `CATCH_CONFIG_GETENV` was [introduced](https://github.com/catchorg/Catch2/pull/2562) in Catch2 3.2.0
165166
167+
> `CATCH_CONFIG_USE_BUILTIN_CONSTANT_P` was introduced in Catch2 vX.Y.Z
168+
166169
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.
167170

168171
`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`,
183186
these toggles can be disabled by using `_NO_` form of the toggle,
184187
e.g. `CATCH_CONFIG_NO_WINDOWS_SEH`.
185188

189+
`CATCH_CONFIG_USE_BUILTIN_CONSTANT_P` is ON by default for Clang and GCC
190+
(but as far as possible, not for other compilers masquerading for these
191+
two). However, it can cause bugs where the enclosed code is evaluated, even
192+
though it should not be, e.g. in [#2925](https://github.com/catchorg/Catch2/issues/2925).
193+
194+
186195
### `CATCH_CONFIG_FAST_COMPILE`
187196
This compile-time flag speeds up compilation of assertion macros by ~20%,
188197
by disabling the generation of assertion-local try-catch blocks for

src/catch2/catch_user_config.hpp.in

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,15 @@
178178
#endif
179179

180180

181+
#cmakedefine CATCH_CONFIG_USE_BUILTIN_CONSTANT_P
182+
#cmakedefine CATCH_CONFIG_NO_USE_BUILTIN_CONSTANT_P
183+
184+
#if defined( CATCH_CONFIG_USE_BUILTIN_CONSTANT_P ) && \
185+
defined( CATCH_CONFIG_NO_USE_BUILTIN_CONSTANT_P )
186+
# error Cannot force USE_BUILTIN_CONSTANT_P to both ON and OFF
187+
#endif
188+
189+
181190
// ------
182191
// Simple toggle defines
183192
// their value is never used and they cannot be overridden

src/catch2/internal/catch_compiler_capabilities.hpp

Lines changed: 39 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@
6262
# define CATCH_INTERNAL_SUPPRESS_SHADOW_WARNINGS \
6363
_Pragma( "GCC diagnostic ignored \"-Wshadow\"" )
6464

65-
# define CATCH_INTERNAL_IGNORE_BUT_WARN(...) (void)__builtin_constant_p(__VA_ARGS__)
65+
# define CATCH_INTERNAL_CONFIG_USE_BUILTIN_CONSTANT_P
6666

6767
#endif
6868

@@ -86,35 +86,13 @@
8686
// clang-cl defines _MSC_VER as well as __clang__, which could cause the
8787
// start/stop internal suppression macros to be double defined.
8888
#if defined(__clang__) && !defined(_MSC_VER)
89-
89+
# define CATCH_INTERNAL_CONFIG_USE_BUILTIN_CONSTANT_P
9090
# define CATCH_INTERNAL_START_WARNINGS_SUPPRESSION _Pragma( "clang diagnostic push" )
9191
# define CATCH_INTERNAL_STOP_WARNINGS_SUPPRESSION _Pragma( "clang diagnostic pop" )
92-
9392
#endif // __clang__ && !_MSC_VER
9493

9594
#if defined(__clang__)
9695

97-
// As of this writing, IBM XL's implementation of __builtin_constant_p has a bug
98-
// which results in calls to destructors being emitted for each temporary,
99-
// without a matching initialization. In practice, this can result in something
100-
// like `std::string::~string` being called on an uninitialized value.
101-
//
102-
// For example, this code will likely segfault under IBM XL:
103-
// ```
104-
// REQUIRE(std::string("12") + "34" == "1234")
105-
// ```
106-
//
107-
// Similarly, NVHPC's implementation of `__builtin_constant_p` has a bug which
108-
// results in calls to the immediately evaluated lambda expressions to be
109-
// reported as unevaluated lambdas.
110-
// https://developer.nvidia.com/nvidia_bug/3321845.
111-
//
112-
// Therefore, `CATCH_INTERNAL_IGNORE_BUT_WARN` is not implemented.
113-
# if !defined(__ibmxl__) && !defined(__CUDACC__) && !defined( __NVCOMPILER )
114-
# define CATCH_INTERNAL_IGNORE_BUT_WARN(...) (void)__builtin_constant_p(__VA_ARGS__) /* NOLINT(cppcoreguidelines-pro-type-vararg, hicpp-vararg) */
115-
# endif
116-
117-
11896
# define CATCH_INTERNAL_SUPPRESS_GLOBALS_WARNINGS \
11997
_Pragma( "clang diagnostic ignored \"-Wexit-time-destructors\"" ) \
12098
_Pragma( "clang diagnostic ignored \"-Wglobal-constructors\"")
@@ -139,6 +117,27 @@
139117

140118
#endif // __clang__
141119

120+
// As of this writing, IBM XL's implementation of __builtin_constant_p has a bug
121+
// which results in calls to destructors being emitted for each temporary,
122+
// without a matching initialization. In practice, this can result in something
123+
// like `std::string::~string` being called on an uninitialized value.
124+
//
125+
// For example, this code will likely segfault under IBM XL:
126+
// ```
127+
// REQUIRE(std::string("12") + "34" == "1234")
128+
// ```
129+
//
130+
// Similarly, NVHPC's implementation of `__builtin_constant_p` has a bug which
131+
// results in calls to the immediately evaluated lambda expressions to be
132+
// reported as unevaluated lambdas.
133+
// https://developer.nvidia.com/nvidia_bug/3321845.
134+
//
135+
// Therefore, `CATCH_INTERNAL_IGNORE_BUT_WARN` is not implemented.
136+
#if defined( __ibmxl__ ) || defined( __CUDACC__ ) || defined( __NVCOMPILER )
137+
# define CATCH_INTERNAL_CONFIG_NO_USE_BUILTIN_CONSTANT_P
138+
#endif
139+
140+
142141

143142
////////////////////////////////////////////////////////////////////////////////
144143
// We know some environments not to support full POSIX signals
@@ -362,6 +361,22 @@
362361
#endif
363362

364363

364+
// The goal of this macro is to avoid evaluation of the arguments, but
365+
// still have the compiler warn on problems inside...
366+
#if defined( CATCH_INTERNAL_CONFIG_USE_BUILTIN_CONSTANT_P ) && \
367+
!defined( CATCH_INTERNAL_CONFIG_NO_USE_BUILTIN_CONSTANT_P ) && !defined(CATCH_CONFIG_USE_BUILTIN_CONSTANT_P)
368+
#define CATCH_CONFIG_USE_BUILTIN_CONSTANT_P
369+
#endif
370+
371+
#if defined( CATCH_CONFIG_USE_BUILTIN_CONSTANT_P ) && \
372+
!defined( CATCH_CONFIG_NO_USE_BUILTIN_CONSTANT_P )
373+
# define CATCH_INTERNAL_IGNORE_BUT_WARN( ... ) \
374+
(void)__builtin_constant_p( __VA_ARGS__ ) /* NOLINT(cppcoreguidelines-pro-type-vararg, \
375+
hicpp-vararg) */
376+
#else
377+
# define CATCH_INTERNAL_IGNORE_BUT_WARN( ... )
378+
#endif
379+
365380
// Even if we do not think the compiler has that warning, we still have
366381
// to provide a macro that can be used by the code.
367382
#if !defined(CATCH_INTERNAL_START_WARNINGS_SUPPRESSION)
@@ -398,13 +413,6 @@
398413
# define CATCH_INTERNAL_SUPPRESS_SHADOW_WARNINGS
399414
#endif
400415

401-
402-
// The goal of this macro is to avoid evaluation of the arguments, but
403-
// still have the compiler warn on problems inside...
404-
#if !defined(CATCH_INTERNAL_IGNORE_BUT_WARN)
405-
# define CATCH_INTERNAL_IGNORE_BUT_WARN(...)
406-
#endif
407-
408416
#if defined(__APPLE__) && defined(__apple_build_version__) && (__clang_major__ < 10)
409417
# undef CATCH_INTERNAL_SUPPRESS_UNUSED_TEMPLATE_WARNINGS
410418
#elif defined(__clang__) && (__clang_major__ < 5)

0 commit comments

Comments
 (0)