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

XX025: && parameters in coroutines - very likely result in UB #13

Open
GorNishanov opened this issue Jun 10, 2017 · 8 comments
Open

XX025: && parameters in coroutines - very likely result in UB #13

GorNishanov opened this issue Jun 10, 2017 · 8 comments

Comments

@GorNishanov
Copy link
Owner

This coroutine follows a perfect forwarding pattern, but, results in UB

template <typename... Args>
generator<int> f(Args&&... args) {
  g(std::forward<Args>(args)...);
  co_return;
}

Should we change the rules for how && parameters are captured in coroutines?

@lewissbaker
Copy link

You can kind of work around it with something like this, which move-constructs copies of r-value reference parameters and passes l-value references through.

template<typename... Args>
generator<int> f(Args&&... args) {
  return [](Args... args) -> generator<int> {
    g(std::forward<Args>(args)...);
    co_return;
  }(std::forward<Args>(args)...);
}

@GorNishanov
Copy link
Owner Author

Cool workaround! Though, I think this is something that evolution group might have want to have some input on.

@lewissbaker
Copy link

lewissbaker commented Jun 10, 2017

I do worry that if we come up with a rule like "rvalue references are always move-copied" then we'll penalise use cases like std::expected or std::optional for which there would not be undefined behavior for rvalue references.

What if we let the promise object or coroutine_traits decide how parameters are captured? Eg by defining some nested typedef in std::experimental::coroutine_traits<RET, ARGS...>

Something like this:

template<typename T, typename... ARGS>
class coroutine_traits<generator<T>, ARGS...>
{
  using arg_capture = void(std::remove_rvalue_reference_t<ARGS>...);
};

This could possibly be a solution to allowing capture of 'this' by value too? If you can determine that call is to a member function from coroutine_traits.

@GorNishanov
Copy link
Owner Author

Possibly some constexpr member in the promise_type? That way we don't have to do the traits if we control the return type

@lewissbaker
Copy link

So were you thinking maybe something like this:

struct generator_promise<T>
{
  generator<T> get_return_object();
  suspend_always initial_suspend();
  suspend_always final_suspend();
  void unhandled_exception();

  // Should this be a non-static member function?
  template<typename ARG>
  static constexpr ARG parameter_transform(ARG&& arg) { return std::forward<ARG>(arg); }
};

Then within the coroutine a reference to the parameter is replaced with a reference to decltype(auto) varWithinCoroutine = promise_type::parameter_transform(var);

How would such a facility affect the ability for the compiler to elide copies if the parameter is not referenced after the first suspend-point? If provided, would the compiler still be required to call parameter_transform() even if not referenced after first suspend-point?

Would the parameter_transform() function be allowed to return a value of a different type than the parameter?
eg. could you add an overload like std::string parameter_transform(int x) { return std::to_string(x); }?

This could lead to particularly surprising (and difficult to understand) code. eg.

generator<int> f(int a, int b)
{
  static_assert(std::is_same_v<decltype(a), std::string>);
  auto ab = a + b; // <--- string concatenation here!
  co_return;
}

I can, however, see potential in the ability to, say, translate std::reference_wrapper<T> to T&.

For non-static member-function coroutines, would *this be passed to promise_type::parameter_transform() to capture implicit this?

@GorNishanov
Copy link
Owner Author

Good thinking. I had parameter_transform in early design for coroutine customization points that I later dropped to trying to make the design leaner and simpler. This is something that can be added in the future in a non-breaking fashion. I'd like to explore simpler alternatives for C++20.

@lewissbaker
Copy link

So something ugly like constexpr bool promise_type::capture_rvalue_reference_parameters_by_value = true;?

Or do we just recommend that generator coroutine functions (and other coroutines that are suspended before executing the body) take all parameters by value rather than trying to use perfect-forwarding?
Then have l-value references opt-in by passing std::reference_wrapper<T> values using std::ref/cref.

eg.

template<typename... ARGS>
generator<int> f(ARGS... args)
{
  g(std::move(args)...); // or possibly some other std::forward-like thing that unwraps std::reference_wrapper if required.
  co_return;
}

@GorNishanov GorNishanov changed the title 00004: GN001: && parameters in coroutines - very likely result in UB XX025: && parameters in coroutines - very likely result in UB Jun 18, 2017
@lewissbaker
Copy link

Another idea that came up during a discussion on #coroutines slack channel was to just prevent r-value references.

template<typename T, typename... ARGS>
struct coroutine_traits<generator<T>, ARGS...>
{
  static_assert(
    !std::disjunction<std::is_rvalue_reference_t<ARGS>...>::value,
    "Passing parameters by rvalue reference is not allowed for generator<T> coroutines");
  using promise_type = generator_promise<T>;
};

Possibly a bit of a sledgehammer, though.

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

2 participants