Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Does not handle final_suspend properly in coroutine transformation #679

Open
geochip opened this issue Dec 13, 2024 · 0 comments
Open

Does not handle final_suspend properly in coroutine transformation #679

geochip opened this issue Dec 13, 2024 · 0 comments

Comments

@geochip
Copy link

geochip commented Dec 13, 2024

I tried to transform the following simple program, which uses C++20 coroutines:
https://github.com/feabhas/coroutines-blog/blob/main/src/char_demo.cpp

Code:

#include <coroutine>
#include <iostream>
#include <cassert>
#include <exception>

class Future;

struct Promise
{
    using value_type = const char*;
    const char* value{};

    Promise() = default;
    std::suspend_always initial_suspend() { return {}; }
    std::suspend_always final_suspend() noexcept { return {}; }
    void unhandled_exception() { std::rethrow_exception(std::current_exception()); }

    std::suspend_always yield_value(const char* value) {
        this->value = std::move(value);
        return {};
    }

    void return_void() {
        this->value = nullptr;
    }

    Future get_return_object();
};

class Future
{
public:
    using promise_type = Promise;

    explicit Future(std::coroutine_handle<Promise> handle)
        : handle (handle) 
    {}

    ~Future() {
        if (handle) { handle.destroy(); }
    }
    
    Promise::value_type next() {
        if (handle) {
            handle.resume();
            return handle.promise().value;
        }
        else {
            return {};
        }
    }

private:
    std::coroutine_handle<Promise> handle{};    
};

Future Promise::get_return_object()
{
    return Future{ std::coroutine_handle<Promise>::from_promise(*this) };
}

Future Generator()
{
    co_yield "Hello ";
    co_yield "world";
    co_yield "!";
}

int main()
{
	auto generator = Generator();
	while (const char* item = generator.next()) {
		std::cout << item;
	}
	std::cout << std::endl;

    return 0;
}

The generated result with "Show coroutine transformation" enabled:

/*************************************************************************************
 * NOTE: The coroutine transformation you've enabled is a hand coded transformation! *
 *       Most of it is _not_ present in the AST. What you see is an approximation.   *
 *************************************************************************************/
#include <coroutine>
#include <iostream>
#include <cassert>
#include <exception>

class Future;

struct Promise
{
  using value_type = const char *;
  const char * value;
  inline constexpr Promise() noexcept = default;
  inline std::suspend_always initial_suspend()
  {
    return {};
  }
  
  inline std::suspend_always final_suspend() noexcept
  {
    return {};
  }
  
  inline void unhandled_exception()
  {
    std::rethrow_exception(std::current_exception());
  }
  
  inline std::suspend_always yield_value(const char * value)
  {
    this->value = std::move(value);
    return {};
  }
  
  inline void return_void()
  {
    this->value = nullptr;
  }
  
  Future get_return_object();
  
};


class Future
{
  
  public: 
  using promise_type = Promise;
  inline explicit Future(std::coroutine_handle<Promise> handle)
  : handle{std::coroutine_handle<Promise>(handle)}
  {
  }
  
  inline ~Future() noexcept
  {
    if(this->handle.operator bool()) {
      this->handle.destroy();
    } 
    
  }
  
  inline Promise::value_type next()
  {
    if(this->handle.operator bool()) {
      this->handle.resume();
      return this->handle.promise().value;
    } else {
      return {};
    } 
    
  }
  
  
  private: 
  std::coroutine_handle<Promise> handle;
  public: 
};


Future Promise::get_return_object()
{
  return Future{std::coroutine_handle<Promise>::from_promise(*this)};
}


struct __GeneratorFrame
{
  void (*resume_fn)(__GeneratorFrame *);
  void (*destroy_fn)(__GeneratorFrame *);
  std::__coroutine_traits_impl<Future>::promise_type __promise;
  int __suspend_index;
  bool __initial_await_suspend_called;
  std::suspend_always __suspend_62_8;
  std::suspend_always __suspend_64_5;
  std::suspend_always __suspend_65_5;
  std::suspend_always __suspend_66_5;
  std::suspend_always __suspend_62_8_1;
};

Future Generator()
{
  /* Allocate the frame including the promise */
  /* Note: The actual parameter new is __builtin_coro_size */
  __GeneratorFrame * __f = reinterpret_cast<__GeneratorFrame *>(operator new(sizeof(__GeneratorFrame)));
  __f->__suspend_index = 0;
  __f->__initial_await_suspend_called = false;
  
  /* Construct the promise. */
  new (&__f->__promise)std::__coroutine_traits_impl<Future>::promise_type{};
  
  /* Forward declare the resume and destroy function. */
  void __GeneratorResume(__GeneratorFrame * __f);
  void __GeneratorDestroy(__GeneratorFrame * __f);
  
  /* Assign the resume and destroy function pointers. */
  __f->resume_fn = &__GeneratorResume;
  __f->destroy_fn = &__GeneratorDestroy;
  
  /* Call the made up function with the coroutine body for initial suspend.
     This function will be called subsequently by coroutine_handle<>::resume()
     which calls __builtin_coro_resume(__handle_) */
  __GeneratorResume(__f);
  
  
  return __f->__promise.get_return_object();
}

/* This function invoked by coroutine_handle<>::resume() */
void __GeneratorResume(__GeneratorFrame * __f)
{
  try 
  {
    /* Create a switch to get to the correct resume point */
    switch(__f->__suspend_index) {
      case 0: break;
      case 1: goto __resume_Generator_1;
      case 2: goto __resume_Generator_2;
      case 3: goto __resume_Generator_3;
      case 4: goto __resume_Generator_4;
    }
    
    /* co_await insights.cpp:62 */
    __f->__suspend_62_8 = __f->__promise.initial_suspend();
    if(!__f->__suspend_62_8.await_ready()) {
      __f->__suspend_62_8.await_suspend(std::coroutine_handle<Promise>::from_address(static_cast<void *>(__f)).operator std::coroutine_handle<void>());
      __f->__suspend_index = 1;
      __f->__initial_await_suspend_called = true;
      return;
    } 
    
    __resume_Generator_1:
    __f->__suspend_62_8.await_resume();
    
    /* co_yield insights.cpp:64 */
    __f->__suspend_64_5 = __f->__promise.yield_value("Hello ");
    if(!__f->__suspend_64_5.await_ready()) {
      __f->__suspend_64_5.await_suspend(std::coroutine_handle<Promise>::from_address(static_cast<void *>(__f)).operator std::coroutine_handle<void>());
      __f->__suspend_index = 2;
      return;
    } 
    
    __resume_Generator_2:
    __f->__suspend_64_5.await_resume();
    
    /* co_yield insights.cpp:65 */
    __f->__suspend_65_5 = __f->__promise.yield_value("world");
    if(!__f->__suspend_65_5.await_ready()) {
      __f->__suspend_65_5.await_suspend(std::coroutine_handle<Promise>::from_address(static_cast<void *>(__f)).operator std::coroutine_handle<void>());
      __f->__suspend_index = 3;
      return;
    } 
    
    __resume_Generator_3:
    __f->__suspend_65_5.await_resume();
    
    /* co_yield insights.cpp:66 */
    __f->__suspend_66_5 = __f->__promise.yield_value("!");
    if(!__f->__suspend_66_5.await_ready()) {
      __f->__suspend_66_5.await_suspend(std::coroutine_handle<Promise>::from_address(static_cast<void *>(__f)).operator std::coroutine_handle<void>());
      __f->__suspend_index = 4;
      return;
    } 
    
    __resume_Generator_4:
    __f->__suspend_66_5.await_resume();
    goto __final_suspend;
  } catch(...) {
    if(!__f->__initial_await_suspend_called) {
      throw ;
    } 
    
    __f->__promise.unhandled_exception();
  }
  
  __final_suspend:
  
  /* co_await insights.cpp:62 */
  __f->__suspend_62_8_1 = __f->__promise.final_suspend();
  if(!__f->__suspend_62_8_1.await_ready()) {
    __f->__suspend_62_8_1.await_suspend(std::coroutine_handle<Promise>::from_address(static_cast<void *>(__f)).operator std::coroutine_handle<void>());
    return;
  } 
  
  __f->destroy_fn(__f);
}

/* This function invoked by coroutine_handle<>::destroy() */
void __GeneratorDestroy(__GeneratorFrame * __f)
{
  /* destroy all variables with dtors */
  __f->~__GeneratorFrame();
  /* Deallocating the coroutine frame */
  /* Note: The actual argument to delete is __builtin_coro_frame with the promise as parameter */
  operator delete(static_cast<void *>(__f));
}


int main()
{
  Future generator = Generator();
  while(item) {
    std::operator<<(std::cout, item);
  }
  
  std::cout.operator<<(std::endl);
  return 0;
}

Compiling this with

g++ -std=c++20 test.cpp

gives an error

test3.cpp: In function ‘int main()’:
test3.cpp:229:9: error: ‘item’ was not declared in this scope; did you mean ‘tm’?
  229 |   while(item) {
      |         ^~~~
      |         tm

but this incorrect handling of variable declaration in while loop parantheses doesn't seem to be related to the coroutines, so i created a separate issue with a more minimal example to reproduce this problem: #678

For now, i just manually replaced while(item) with the correct version while(const char *item = next()).
Then after compiling, instead of printing:

Hello world!

to the console, the generated program prints "Hello world!", but then it is stuck in an infinite loop printing the last result of co_yield (which is "!")
so the output is like this

Hello world!!!!!!!!!!!!!!!!!!!!!!!!!...

and continues to print "!" infinitely.

By comparing the generated assembly output of g++ with the cppinsights output, i have come up with the necessary changes to the program.

--- test2.cpp   2024-12-13 23:40:24.931842586 +0300
+++ test3.cpp   2024-12-13 23:47:19.387911094 +0300
@@ -141,6 +141,7 @@
       case 2: goto __resume_Generator_2;
       case 3: goto __resume_Generator_3;
       case 4: goto __resume_Generator_4;
+      case 5: goto __resume_Generator_5;
     }
 
     /* co_await insights.cpp:62 */
@@ -199,12 +200,15 @@
   __final_suspend:
 
   /* co_await insights.cpp:62 */
+  __f->__promise.return_void();
   __f->__suspend_62_8_1 = __f->__promise.final_suspend();
   if(!__f->__suspend_62_8_1.await_ready()) {
     __f->__suspend_62_8_1.await_suspend(std::coroutine_handle<Promise>::from_address(static_cast<void *>(__f)).operator std::coroutine_handle<void>());
+    __f->__suspend_index = 5;
     return;
   }
 
+  __resume_Generator_5:
   __f->destroy_fn(__f);
 }

The expected behavior would be to generate a correct program that behaves similar to the program with the applied diff from above, i.e. it prints "Hello world!" and exits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant