3
\$\begingroup\$

What do you think about this?

#include <alloca.h>

#include <setjmp.h>

#include <cassert>

#include <functional>

template <::std::size_t N = 4096>
class coroutine
{
  jmp_buf env_in_;
  jmp_buf env_out_;

  bool running_{};

  char stack_[N];

public:
  coroutine() = default;

  auto running() const noexcept
  {
    return running_;
  }

  template <typename F, typename ...A>
  void run(F&& f, A&& ...a)
  {
    if (setjmp(env_in_))
    {
      return;
    }
    // else do nothing

    auto top(reinterpret_cast<char*>(&top));
    alloca(top - (stack_ + N));

    running_ = true;

    [this, f = ::std::forward<F>(f)](A&& ...a) __attribute__ ((noinline))
    {
      f(::std::ref(*this), ::std::forward<A>(a)...);

      running_ = false;

      yield();
    }(::std::forward<A>(a)...);
  }

  void yield() noexcept
  {
    if (setjmp(env_out_))
    {
      return;
    }
    else
    {
      longjmp(env_in_, 1);
    }
  }

  void resume()
  {
    assert(running_);
    if (setjmp(env_in_))
    {
      return;
    }
    else
    {
      longjmp(env_out_, 1);
    }
  }
};

Usage:

#include <iostream>

#include "coroutine.hpp"

int main()
{
  coroutine<> c;

  c.run([](decltype(c)& c)
    {
      for (int i{}; i != 3; ++i)
      {
        ::std::cout << i << ::std::endl;

        c.yield();
      }
    }
  );

  while (c.running())
  {
    c.resume();
  }

  return 0;
}

EDIT

Change

auto top(reinterpret_cast<char*>(&top));

to

char* top;
top = reinterpret_cast<char*>(&top);

Also check out an updated version of the code.

\$\endgroup\$
  • \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Mast Dec 29 '16 at 16:25
  • \$\begingroup\$ @Mast there was a bug in there due to an old gcc bug, check the answer to my question. This is why I fixed the code, not because of some other reason. The code as it is will not compile on newer compilers without the fix. \$\endgroup\$ – user1095108 Dec 29 '16 at 16:33
  • \$\begingroup\$ You shall not invalidate answers. So I'll roll back the edit again. Feel free to post a follow-up question linking back to this question with more up-to-date code. \$\endgroup\$ – Mast Dec 29 '16 at 16:44
  • 1
    \$\begingroup\$ @Mast there is a serious bug in there without the edit. I don't need a follow-up question. The code won't compile without the edit. \$\endgroup\$ – user1095108 Dec 29 '16 at 16:52
  • 1
    \$\begingroup\$ @Mast Not in case of serious bugs .(An exception to that would be if a user wrote a comment saying that your code is completely broken, and needs to be fixed before it can be reviewed.) This is exactly what happened. \$\endgroup\$ – user1095108 Dec 29 '16 at 17:27
3
\$\begingroup\$

I think you need to document your assumptions and/or intended use cases a little better. Right now, I don't see how anything based on setjmp/longjmp could actually implement a coroutine, since longjmp can only ever jump upward in the call-stack, never downward. (Really, it can't jump "downward" because there is no "downward": the only things below us on the call-stack are things that have already finished executing, and things which will execute in the future.)

Notice that "things which will execute in the future" includes "interrupt handlers". You might want to check whether your implementation plays well with Unix signal handlers — I suspect it does not.


alloca(top - (stack_ + N));

This is a red flag: you're calling alloca without assigning the result anywhere. A sufficiently smart compiler will just optimize away this useless call. (Yeah, I kinda get what you're trying to do... but the compiler is under no obligation to get it.) What you really need to do here is heap-allocate a buffer for your stack, and then somehow get the saved stack pointer in the jmp_buf to point to it.

And I only kinda see what you're doing here, anyway. Are you assuming that the coroutine object itself must be allocated on the stack? What happens if I make a global or static coroutine object?


[this, f = ::std::forward<F>(f)](A&& ...a) __attribute__ ((noinline))

The __attribute__((noinline)) here is another red flag. This time I don't get what you're trying to do. What would break if this lambda's operator() got inlined?

Also, style nit: I've never seen anyone put a space after the word __attribute__ before.


Other style nits:

auto running() const noexcept

auto is a strange way to spell bool. It's not saving you any bytes.

// else do nothing

The above comment is highly misleading, in that it's followed by 13 lines of really dense code — hardly "do nothing"! I'd delete that comment.

auto top(reinterpret_cast<char*>(&top));

I'm actually amazed that this works, given that the decltype of &top is not known at the time the RHS is parsed. ...Oh wait, yeah, it doesn't work. Both GCC and Clang reject this code.

for (int i{}; i != 3; ++i)

Kudos for using prefix increment. Everything else about this for-loop is pretty unusual, though. I strongly recommend looping over the range [0,3) like this instead:

for (int i = 0; i < 3; ++i)

API design suggestions:

  • Empty angle brackets suck. I'd recommend something more like

    template <size_t N> class coroutine_with_size { ... }
    
    using coroutine = coroutine_with_size<4096>;
    
  • It's unfortunate that your coroutine has to do all its own I/O; a better design would be like the one found in Javascript (and Python), where coroutines can yield values. Unfortunately, the ability to yield values causes a cascade of API changes. Look:

    c.run([](auto&& c) {
        for (int i = 0; i < 3; ++i) {
            c.yield(i);
        }
    });
    
    while (c.running())
    {
        int ii = c.resume();
        std::cout << ii << std::endl;
    }
    

Well, unless c is somehow "looking ahead" to determine the value it's going to return next time, c.running() doesn't really make sense anymore. We've got to have c.resume() do all the hard work, at which point we need a way to say "okay, no more data, please stop iterating". That is, we need a way for c.resume() to terminate execution but not return a value. This smells like an exception. So:

c.run([](auto&& c) {
    for (int i = 0; i < 3; ++i) {
        c.yield(i);
    }
});

while (true)
{
    try {
        int ii = c.resume();
        std::cout << ii << std::endl;
    } catch (const coroutine::done&) {
        break;
    }
}

But now what is c.run() actually doing? It can't be actually starting to run the provided lambda, because there's nowhere for us to stash the yielded result yet (we haven't gotten to ii's declaration). So all it's really doing now is constructing the coroutine object and then returning immediately. That smells like a constructor. So:

auto c = coroutine([](auto&& c) {
    for (int i = 0; i < 3; ++i) {
        c.yield(i);
    }
});

while (true)
{
    try {
        int ii = c.resume();
        std::cout << ii << std::endl;
    } catch (const coroutine::done&) {
        break;
    }
}

And unfortunately now we've got move-constructible coroutine objects, and your entire implementation strategy falls apart even more than it had been. Crap!


...Speaking of which, your current implementation provides defaulted copy and move constructors! You should definitely delete those:

coroutine(const coroutine&) = delete;
coroutine& operator=(const coroutine&) = delete;
// move operations get deleted implicitly due to the above;
// but I wouldn't object to deleting them explicitly as well

Getting back to the API-design example: the next step in a Javascript-like design would be allowing c.resume() to take input values, as well as yielding output values. Mind you, I'm not sure what the implementation for this would look like in C++. It might not be 100% possible. :)

auto c = coroutine([](auto&& c, int step) {
    for (int i = 0; i < 3; i += step) {
        step = c.yield(i);
    }
});

while (true)
{
    try {
        int ii = c.resume(1);
        std::cout << ii << std::endl;
    } catch (const coroutine::done&) {
        break;
    }
}

EDIT: Okay, I think it's not 100% possible — not without templating the coroutine class on the "inputs" and "outputs" (args-to-resume and args-to-yield) that it expects. I've done my own toy implementation here for comparison. Muddling through that implementation also helped me understand the undefined-behavior magic about which I complained at the top of this review.


At least in the trivial case you started with — a coroutine used only as a generator, and whose c.resume() takes no input values — it would make sense to provide a sort of coroutine_generator_iterator so that one could write simply

for (int ii : coroutine_generator_iterator(c)) {
    std::cout << ii << std::endl;
}

This coroutine_generator_iterator concept wouldn't be very useful for coroutines that were actually used as coroutines, e.g. passing control back and forth. But it's definitely a good idea if you're just trying to do Python-style generator functions in C++.

\$\endgroup\$
  • \$\begingroup\$ The top thing was due to a temporary gcc bug, I fixed the code a little. Anyway there's a better coroutine class here: github.com/user1095108/generic/blob/master/coroutine.hpp \$\endgroup\$ – user1095108 Dec 29 '16 at 16:13
  • \$\begingroup\$ As for generators, you can provide a reference to the coroutine, if you want some result returned. I thought generators were not orthogonal. I know that usually coroutines are used as such, but you many not want a result returned at all so, I felt this violated the orthogonality/minimalistic design principle. \$\endgroup\$ – user1095108 Dec 29 '16 at 16:18
  • \$\begingroup\$ Re "the top thing": no, that's not a compiler bug AFAIK. You can write char* top = reinterpret_cast<char*>(&top); to fix it. But you can't refer to top in an expression that requires top's type before top's type is known. Unless I'm mistaken, that's not a compiler bug; that's just how causality works in this universe. ;) \$\endgroup\$ – Quuxplusone Dec 29 '16 at 17:50
  • \$\begingroup\$ it used to compile, or I wouldn't post it here. \$\endgroup\$ – user1095108 Dec 29 '16 at 20:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.