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

unique_region_t move constructor and assigment operator doesn't compile #713

Open
ralwing opened this issue Feb 13, 2025 · 1 comment
Open

Comments

@ralwing
Copy link

ralwing commented Feb 13, 2025

The code:

#include <cuda/api.hpp>
#include <type_traits>
#ifndef _MSC_VER
// MSVC on Windows has trouble locating cudaProfiler.h somehow
#include <cuda/nvtx.hpp>
#endif
#include <cuda/rtc.hpp>
#include <cuda/fatbin.hpp>

#include <cstdlib>
#include <iostream>

struct TrivialStruct
{
  float x, y, z;
};

class RegionHolder
{
public:
  RegionHolder()
  : cuda_device_{cuda::device::current::get()},
    mem_(cuda::memory::device::make_unique_region(cuda_device_, 10 * sizeof(TrivialStruct)))
  {
  }
  RegionHolder(const RegionHolder & other) = delete;
  RegionHolder & operator=(const RegionHolder & other) = delete;

  RegionHolder & operator=(RegionHolder && other) noexcept = default;
  RegionHolder(RegionHolder && other) noexcept = default;

private:
  cuda::device_t cuda_device_;
  cuda::memory::device::unique_region mem_;
};

RegionHolder make_region()
{
  return RegionHolder{};
}

int main()
{

  auto r1 = make_region();
  auto r2= make_region();
  std::swap(r1, r2);
}

Doesn't compile.

In file included from /workspace/cuda-api-wrappers/src/cuda/api.hpp:28,
                 from /workspace/cuda-api-wrappers/examples/other/new_cpp_standard/main.cpp:10:
/workspace/cuda-api-wrappers/src/cuda/api/unique_region.hpp: In instantiation of 'void cuda::memory::unique_region<Deleter>::reset(cuda::memory::region_t) [with Deleter = cuda::memory::device::detail_::deleter]':
/workspace/cuda-api-wrappers/src/cuda/api/unique_region.hpp:88:9:   required from 'cuda::memory::unique_region<Deleter>& cuda::memory::unique_region<Deleter>::operator=(cuda::memory::unique_region<Deleter>&&) [with Deleter = cuda::memory::device::detail_::deleter]'
/workspace/cuda-api-wrappers/examples/other/new_cpp_standard/main.cpp:59:18:   required from 'std::_Require<std::__not_<std::__is_tuple_like<_Tp> >, std::is_move_constructible<_Tp>, std::is_move_assignable<_Tp> > std::swap(_Tp&, _Tp&) [with _Tp = RegionHolder; _Require<__not_<__is_tuple_like<_Tp> >, is_move_constructible<_Tp>, is_move_assignable<_Tp> > = void]'
/workspace/cuda-api-wrappers/examples/other/new_cpp_standard/main.cpp:82:12:   required from here
/workspace/cuda-api-wrappers/src/cuda/api/unique_region.hpp:141:26: error: no match for call to '(cuda::memory::unique_region<cuda::memory::device::detail_::deleter>::deleter_type {aka cuda::memory::device::detail_::deleter}) ()'
  141 |             get_deleter()();
      |             ~~~~~~~~~~~~~^~
In file included from /workspace/cuda-api-wrappers/src/cuda/api/texture_view.hpp:14,
                 from /workspace/cuda-api-wrappers/src/cuda/api.hpp:22:
/workspace/cuda-api-wrappers/src/cuda/api/memory.hpp:343:14: note: candidate: 'void cuda::memory::device::detail_::deleter::operator()(void*) const'
  343 |         void operator()(void* ptr) const { cuda::memory::device::free(ptr); }
      |              ^~~~~~~~
/workspace/cuda-api-wrappers/src/cuda/api/memory.hpp:343:14: note:   candidate expects 1 argument, 0 provided
gmake[2]: *** [examples/CMakeFiles/cpp_14.dir/build.make:76: examples/CMakeFiles/cpp_14.dir/other/new_cpp_standard/main.cpp.o] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:1202: examples/CMakeFiles/cpp_14.dir/all] Error 2

It seems like the reset function is never used in the examples code.
I'll provide a PR to fix this.

@eyalroz
Copy link
Owner

eyalroz commented Feb 14, 2025

Indeed, one of the flaws of this library is that it doesn't have decent unit test coverage; and the examples aren't a good enough proxy for that; and some templated code doesn't necessarily get instantiated. This is also the reason I am reluctant to publish a "1.0" version. Thanks for the catch, I'm going to check out the PR later today.

eyalroz added a commit that referenced this issue Feb 14, 2025
* The `reset()` method now passes a pointer, rather than a region, to the deleter
* Now using `get_deleter()` in the destructor as well
* Comment corrections and deletions
eyalroz added a commit that referenced this issue Feb 14, 2025
* The `reset()` method now passes a pointer, rather than a region, to the deleter
* Now using `get_deleter()` in the destructor as well
* Comment corrections and deletions

Thanks goes to GitHub user @ralwing for catching the bug and proposing the fix.
@eyalroz eyalroz added the bug label Feb 14, 2025
eyalroz added a commit that referenced this issue Feb 14, 2025
* The `reset()` method now passes a pointer, rather than a region, to the deleter
* Now using `get_deleter()` in the destructor as well
* Comment corrections and deletions
* Added an instantiation of `unique_region` to the `new_cpp_standard` example program

Thanks goes to GitHub user @ralwing for catching the bug and proposing the fix.
eyalroz added a commit that referenced this issue Feb 16, 2025
* The `reset()` method now passes a pointer, rather than a region, to the deleter
* Now using `get_deleter()` in the destructor as well
* Comment corrections and deletions
* Added an instantiation of `unique_region` to the `new_cpp_standard` example program

Thanks goes to GitHub user @ralwing for catching the bug and proposing the fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants