catchorg / Catch2

A modern, C++-native, test framework for unit-tests, TDD and BDD - using C++14, C++17 and later (C++11 support is in v2.x branch, and C++03 on the Catch1.x branch)
https://discord.gg/4CWS9zD
Boost Software License 1.0
18.71k stars 3.06k forks source link

co_await expression inside REQUIRE may be run twice (GCC) #2591

Open alvinhochun opened 1 year ago

alvinhochun commented 1 year ago

Describe the bug

If a REQUIRE assertion contains a co_await expression, it may be executed twice.

Expected behavior

The expression shouldn't run twice.

Reproduction steps

(I'd hope to present a small self-contained example but I don't have one for something that involves coroutines, so...)

  1. Install MSYS2
  2. In MSYS2, install: pacman -S mingw-w64-ucrt-x86_64-toolchain mingw-w64-ucrt-x86_64-cmake mingw-w64-ucrt-x86_64-ninja git
  3. Open MSYS2 MinGW UCRT x64 command line
  4. Download the code from this commit: https://github.com/alvinhochun/cppwinrt/tree/20e5eb3a440a31913f294e5b4b3e759c830fe456
  5. Run:
    $ mkdir cppwinrt-build
    $ cd cppwinrt-build
    $ cmake ../cppwinrt -GNinja -DCMAKE_BUILD_TYPE=Debug \
    -DCMAKE_COLOR_DIAGNOSTICS=TRUE \
    -DDOWNLOAD_WINDOWSNUMERICS=TRUE \
    -DUSE_ANSI_COLOR=TRUE
    $ ninja test_old
    $ test/test_old  'async\, resume_on_signal'

Platform information:

Additional context

The problematic line is https://github.com/alvinhochun/cppwinrt/blob/20e5eb3a440a31913f294e5b4b3e759c830fe456/test/old_tests/UnitTests/async.cpp#L1572

REQUIRE(true == co_await resume_on_signal(signal, 1s)); // should eventually succeed

The preprocessed code looks like this:

       do { (void)__builtin_constant_p(
       true == co_await resume_on_signal(signal, 1s)
       ); Catch::AssertionHandler catchAssertionHandler( "REQUIRE"_catch_sr, ::Catch::SourceLineInfo( "D:/dev/mingw-winrt/cppwinrt/test/old_tests/UnitTests/async.cpp", static_cast<std::size_t>( 1572 ) ), "true == co_await resume_on_signal(signal, 1s)", Catch::ResultDisposition::Normal ); try {
        catchAssertionHandler.handleExpr( Catch::Decomposer() <= 
       true == co_await resume_on_signal(signal, 1s) 
       );
        } catch(...) { catchAssertionHandler.handleUnexpectedInflightException(); } catchAssertionHandler.complete(); } while( (void)0, (false) && static_cast<bool>( !!(
       true == co_await resume_on_signal(signal, 1s)
       ) ) )

From debugging the preprocessed code, it seems the co_await expression (line 2) inside __builtin_constant_p is being executed as well as the actual assertion (line 5). Is this looking like a compiler bug?

I believe this issue doesn't happen with Clang (mingw-w64).

horenmar commented 1 year ago

Yep, that's a compiler bug. __builtin_constant_p is an unevaluated context, so the code inside should not be run (it should not even be compiled into the binary).