[Coverity] Move from a user-supplied l-value reference

[AlexeySachkov]

This is reported as CID 535449, you can find Coverity issues at https://scan.coverity.com/projects/intel-llvm?tab=overview

llvm/sycl/include/sycl/handler.hpp

Lines 1463 to 1476 in 3d70234

	template <typename FuncT>
	std::enable_if_t<detail::check_fn_signature<std::remove_reference_t<FuncT>,
	void()>::value ||
	detail::check_fn_signature<std::remove_reference_t<FuncT>,
	void(interop_handle)>::value>
	host_task_impl(FuncT &&Func) {
	throwIfActionIsCreated();
	// Need to copy these rather than move so that we can check associated
	// accessors during finalize
	setArgsToAssociatedAccessors();
	SetHostTask(std::move(Func));
	}

We accept Func argument as a universal reference, meaning that an end user can supply us with an l-value reference:

llvm/sycl/include/sycl/handler.hpp

Lines 1709 to 1711 in 3d70234

	host_task(FuncT &&Func) {
	host_task_impl(Func);
	}

Which we will then move, thus creating a potentially unexpected side effect. In the example below the second cout results in nulltpr.

#include <sycl/sycl.hpp>

struct Functor {
  Functor() : ptr(new int(42)) {}
  Functor(const Functor &other) : ptr(new int) {
    *ptr = *other.ptr;
  }

  Functor(Functor &&other) {
    ptr = other.ptr;
    other.ptr = nullptr;
  }

  void operator()() const {}

  ~Functor() {
    if (ptr)
      delete ptr;
  }

  int *ptr;
};

int main() {

  sycl::queue q;

  Functor f;

  std::cout << "f.ptr = " << f.ptr << std::endl;

  q.submit([&](sycl::handler &cgh) {
    cgh.host_task(f);
  });

  std::cout << "f.ptr = " << f.ptr << std::endl;

  return 0;
}

The spec does not seem to be clear about whether or not it is valid for the implementation to leave the input object in an unspecified state.
Related discussions:

• Add sycl_khr_free_function_commands extension KhronosGroup/SYCL-Docs#644 (comment)
• Add sycl_khr_free_function_commands extension KhronosGroup/SYCL-Docs#922 (comment)

Tagging @aelovikov-intel for awareness.

Without the spec clarification we should probably avoid doing std::moves, switch to std::forward and provide a library entry point which accepts a regular reference (because today we only provide one for the universal reference).

[aelovikov-intel]

The spec does not seem to be clear about whether or not it is valid for the implementation to leave the input object in an unspecified state.

I think perfect forwarding in the API implies that std::forward will be used on that parameter. E.g., std::apply has:

Given the exposition-only function apply-impl defined as follows:
...
The effect is equivalent to:
return apply-impl(std::forward<F>(f), ...);

As such, we should just change move to forward.

As for

provide a library entry point which accepts a regular reference (because today we only provide one for the universal reference)

can you clarify why?