eyalroz / cuda-api-wrappers

Thin, unified, C++-flavored wrappers for the CUDA APIs
BSD 3-Clause "New" or "Revised" License
766 stars 79 forks source link

unique_span assignement operator bug with C++20. #660

Closed raplonu closed 3 weeks ago

raplonu commented 1 month ago

Hi,

I notice that cuda::unique_span does not compile with C++20 (and I suppose it is also the case with older standards too)

The issue comes from the move assignment operator:

unique_span& operator=(unique_span&& other) noexcept
{
    span_type released = other.release();
    if (data() != nullptr) {
        deleter_type{}(data());
    }
    data() = released.data();
    size() = released.size();
}

std::span data & size returns values, not references thus it is not possible to reassign them. I think what you did in the destructor is the correct approach.

unique_span& operator=(unique_span&& other) noexcept
{
    span_type released = other.release();
    if (data() != nullptr) {
        deleter_type{}(data());
    }
    static_cast<span_type&>(*this) = released;
}

Thanks a lot for all the efforts you put in this project.

eyalroz commented 1 month ago

Well, I do compile an example program with C++20, and C++23 even... can you provide a program which fails, and the compiler/platform on which it fails?

Still, I don't mind the change.

raplonu commented 1 month ago

Thanks for you reply !

Sure, the following code:

#include <cuda/api.hpp>

int main() {
    cuda::memory::host::unique_span<float> data1(nullptr, 0);
    cuda::memory::host::unique_span<float> data2(nullptr, 0);

    data1 = std::move(data2);
}

with gcc 11, it produce this:

In file included from /home/jbernard/workspace/other/cuda-api-wrappers/src/cuda/api/memory.hpp:36,
                 from /home/jbernard/workspace/other/cuda-api-wrappers/src/cuda/api/texture_view.hpp:14,
                 from /home/jbernard/workspace/other/cuda-api-wrappers/src/cuda/api.hpp:22,
                 from /home/jbernard/workspace/other/cuda-api-wrappers/examples/modified_cuda_samples/vectorAdd/vectorAdd.cpp:1:
/home/jbernard/workspace/other/cuda-api-wrappers/src/cuda/api/detail/unique_span.hpp: In instantiation of ‘cuda::unique_span<T, Deleter>& cuda::unique_span<T, Deleter>::operator=(cuda::unique_span<T, Deleter>&&) [with T = float; Deleter = cuda::memory::host::detail_::deleter]’:
/home/jbernard/workspace/other/cuda-api-wrappers/examples/modified_cuda_samples/vectorAdd/vectorAdd.cpp:7:25:   required from here
/home/jbernard/workspace/other/cuda-api-wrappers/src/cuda/api/detail/unique_span.hpp:109:21: error: lvalue required as left operand of assignment
  109 |                 data() = released.data();
      |                 ~~~~^~
/home/jbernard/workspace/other/cuda-api-wrappers/src/cuda/api/detail/unique_span.hpp:110:21: error: lvalue required as left operand of assignment
  110 |                 size() = released.size();
      |                 ~~~~^~
/home/jbernard/workspace/other/cuda-api-wrappers/src/cuda/api/detail/unique_span.hpp:111:9: warning: no return statement in function returning non-void [-Wreturn-type]
  110 |                 size() = released.size();
  +++ |+              return *this;
  111 |         }
      |         ^

And with nvcc 12, it produces:

/home/jbernard/workspace/other/cuda-api-wrappers/src/cuda/api/detail/unique_span.hpp(109): error: expression must be a modifiable lvalue
    data() = released.data();
    ^
          detected during instantiation of "cuda::unique_span<T, Deleter> &cuda::unique_span<T, Deleter>::operator=(cuda::unique_span<T, Deleter> &&) [with T=float, Deleter=cuda::memory::host::detail_::deleter]" at line 7 of /home/jbernard/workspace/other/cuda-api-wrappers/examples/modified_cuda_samples/vectorAdd/vectorAdd.cu

/home/jbernard/workspace/other/cuda-api-wrappers/src/cuda/api/detail/unique_span.hpp(110): error: expression must be a modifiable lvalue
    size() = released.size();
    ^
          detected during instantiation of "cuda::unique_span<T, Deleter> &cuda::unique_span<T, Deleter>::operator=(cuda::unique_span<T, Deleter> &&) [with T=float, Deleter=cuda::memory::host::detail_::deleter]" at line 7 of /home/jbernard/workspace/other/cuda-api-wrappers/examples/modified_cuda_samples/vectorAdd/vectorAdd.cu

I forgot to mention the missing return *this; statement. This example is compiled using C++ 11.

eyalroz commented 1 month ago

Hmm... I guess the compiler didn't complain about the unique_span because it doesn't get explicitly instantiated in any of the headers.

eyalroz commented 1 month ago

I aught to pre-release a beta of 0.7.1 soon, I think.

raplonu commented 1 month ago

Yes, C++ template type's members are implicitly instantiated when used. Cheers

eyalroz commented 1 month ago

The bug is only closed when the commit lands on the master branch (i.e. in a release)... in the mean time I mark it as resolved-on-development.

Anyway, thanks for your kind words and this report. The best support I get is when people file issues or comment on questions/discussions; or use the library in a publicly-available project and mention it somewhere in the README/documentation.

kdkavanagh commented 1 month ago

Does this bug impact the non-operator move constructor as well?

I see that the move ctr just constructs from other.release()... Release seems to update the current object using the move operator: static_cast<span_type &>(*this) = span_type{static_cast<T*>(nullptr), 0};

Asking cuz I seem to be unable to move unique_spans explicitly w/o their deleter running... Wondering if this bug is preventing the data ptr on the moved-from object to be left as non-null/therefore deleted when the dtor runs