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_span assignement operator bug with C++20. #660

Closed
raplonu opened this issue Jul 15, 2024 · 7 comments
Closed

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

raplonu opened this issue Jul 15, 2024 · 7 comments

Comments

@raplonu
Copy link

raplonu commented Jul 15, 2024

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
Copy link
Owner

eyalroz commented Jul 15, 2024

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
Copy link
Author

raplonu commented Jul 15, 2024

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
Copy link
Owner

eyalroz commented Jul 15, 2024

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
Copy link
Owner

eyalroz commented Jul 15, 2024

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

@raplonu
Copy link
Author

raplonu commented Jul 15, 2024

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

@raplonu raplonu closed this as completed Jul 15, 2024
@eyalroz eyalroz reopened this Jul 15, 2024
@eyalroz
Copy link
Owner

eyalroz commented Jul 15, 2024

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
Copy link

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

@eyalroz eyalroz closed this as completed in f18303f Aug 4, 2024
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

3 participants