From 58a8ef69f38069335d19650b1aa4d9ff9ce584b8 Mon Sep 17 00:00:00 2001 From: Eyal Rozenberg Date: Mon, 6 Jun 2022 01:03:27 +0300 Subject: [PATCH] Fixes #357, #356: Implement, remove and update assignment operators as necessary * Made sure all (relevant) proxies have a (`noexcept`) move-assignment operator. * Made sure all (relevant) proxies have no copy-assignment operator. * Ensured move assignment operators regard all members, including `hold_pc_refcount_unit`. * primary context assignment operators now actually return primary context references... --- .../action-scripts/install-cuda-windows.ps1 | 6 ++++- src/cuda/api/context.hpp | 13 +---------- src/cuda/api/event.hpp | 22 +++++++++++++------ src/cuda/api/kernel.hpp | 13 +++++++++++ src/cuda/api/link.hpp | 19 +++++++++++----- src/cuda/api/module.hpp | 13 +++++++++-- src/cuda/api/multi_wrapper_impls/module.hpp | 12 ++++++---- src/cuda/api/primary_context.hpp | 4 ++-- src/cuda/api/stream.hpp | 20 ++++++++++++----- src/cuda/nvrtc/compilation_options.hpp | 10 +++++++++ 10 files changed, 92 insertions(+), 40 deletions(-) diff --git a/.github/action-scripts/install-cuda-windows.ps1 b/.github/action-scripts/install-cuda-windows.ps1 index 183824dc..43a14688 100755 --- a/.github/action-scripts/install-cuda-windows.ps1 +++ b/.github/action-scripts/install-cuda-windows.ps1 @@ -169,11 +169,14 @@ if (!$?) { # Store the CUDA_PATH in the environment for the current session, to be forwarded in the action. $CUDA_PATH = "C:\Program Files\NVIDIA GPU Computing Toolkit\CUDA\v$($CUDA_MAJOR).$($CUDA_MINOR)" $CUDA_PATH_VX_Y = "CUDA_PATH_V$($CUDA_MAJOR)_$($CUDA_MINOR)" +$NVTOOLSEXT_PATH = "C:\Program Files\NVIDIA GPU Computing Toolkit\nvToolsExt" # Set environmental variables in this session $env:CUDA_PATH = "$($CUDA_PATH)" $env:CUDA_PATH_VX_Y = "$($CUDA_PATH_VX_Y)" +$env:NVTOOLSEXT_PATH = "$($NVTOOLSEXT_PATH)" Write-Output "CUDA_PATH $($CUDA_PATH)" Write-Output "CUDA_PATH_VX_Y $($CUDA_PATH_VX_Y)" +Write-Output "NVTOOLSEXT_PATH $($NVTOOLSEXT_PATH)" # PATH needs updating elsewhere, anything in here won't persist. # Append $CUDA_PATH/bin to path. @@ -183,8 +186,9 @@ Write-Output "CUDA_PATH_VX_Y $($CUDA_PATH_VX_Y)" # If executing on github actions, emit the appropriate echo statements to update environment variables if (Test-Path "env:GITHUB_ACTIONS") { # Set paths for subsequent steps, using $env:CUDA_PATH - echo "Adding CUDA to CUDA_PATH, CUDA_PATH_X_Y and PATH" + echo "Adding CUDA-related environment entries: CUDA_PATH, NVTOOLSEXT_PATH" echo "CUDA_PATH=$env:CUDA_PATH" | Out-File -FilePath $env:GITHUB_ENV -Encoding utf8 -Append + echo "NVTOOLSEXT_PATH=$env:NVTOOLSEXT_PATH" | Out-File -FilePath $env:GITHUB_ENV -Encoding utf8 -Append echo "$env:CUDA_PATH_VX_Y=$env:CUDA_PATH" | Out-File -FilePath $env:GITHUB_ENV -Encoding utf8 -Append echo "$env:CUDA_PATH/bin" | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8 -Append } diff --git a/src/cuda/api/context.hpp b/src/cuda/api/context.hpp index 5b116952..c90d8930 100644 --- a/src/cuda/api/context.hpp +++ b/src/cuda/api/context.hpp @@ -648,18 +648,7 @@ class context_t { public: // operators - context_t& operator=(const context_t& other) - { - if (owning_) { - context::detail_::destroy(handle_); - } - device_id_ = other.device_id_; - handle_ = other.handle_; - owning_ = false; - return *this; - } - - // Deleted since the handle_t and handle_t are constant + context_t& operator=(const context_t&) = delete; context_t& operator=(context_t&& other) noexcept { ::std::swap(device_id_, other.device_id_); diff --git a/src/cuda/api/event.hpp b/src/cuda/api/event.hpp index 34d6ae6f..595c1cb0 100644 --- a/src/cuda/api/event.hpp +++ b/src/cuda/api/event.hpp @@ -284,17 +284,25 @@ class event_t { public: // operators - event_t& operator=(const event_t& other) = delete; - event_t& operator=(event_t&& other) = delete; + event_t& operator=(const event_t&) = delete; + event_t& operator=(event_t&& other) noexcept + { + ::std::swap(device_id_, other.device_id_); + ::std::swap(context_handle_, other.context_handle_); + ::std::swap(handle_, other.handle_); + ::std::swap(owning, other.owning); + ::std::swap(holds_pc_refcount_unit, holds_pc_refcount_unit); + return *this; + } protected: // data members - const device::id_t device_id_; - const context::handle_t context_handle_; - const event::handle_t handle_; - bool owning; + device::id_t device_id_; + context::handle_t context_handle_; + event::handle_t handle_; + bool owning; // this field is mutable only for enabling move construction; other // than in that case it must not be altered - bool holds_pc_refcount_unit; + bool holds_pc_refcount_unit; // When context_handle_ is the handle of a primary context, this event may // be "keeping that context alive" through the refcount - in which case // it must release its refcount unit on destruction diff --git a/src/cuda/api/kernel.hpp b/src/cuda/api/kernel.hpp index 3f19fb5b..0e95fe89 100644 --- a/src/cuda/api/kernel.hpp +++ b/src/cuda/api/kernel.hpp @@ -137,6 +137,19 @@ class kernel_t { kernel::handle_t handle() const noexcept { return handle_; } #endif +public: // operators + + kernel_t& operator=(const kernel_t&) = delete; + kernel_t& operator=(kernel_t&& other) noexcept + { + ::std::swap(device_id_, other.device_id_); + ::std::swap(context_handle_, other.context_handle_); + ::std::swap(handle_, other.handle_); + ::std::swap(holds_pc_refcount_unit, holds_pc_refcount_unit); + return *this; + } + + public: // non-mutators VIRTUAL_UNLESS_CAN_GET_APRIORI_KERNEL_HANDLE diff --git a/src/cuda/api/link.hpp b/src/cuda/api/link.hpp index 7b6caf9e..683c57ff 100644 --- a/src/cuda/api/link.hpp +++ b/src/cuda/api/link.hpp @@ -157,14 +157,21 @@ class link_t { public: // operators - link_t& operator=(const link_t& other) = delete; - link_t& operator=(link_t&& other) = delete; + link_t& operator=(const link_t&) = delete; + link_t& operator=(link_t&& other) noexcept + { + ::std::swap(context_handle_, other.context_handle_); + ::std::swap(handle_, other.handle_); + ::std::swap(options_, other.options_); + ::std::swap(owning, owning); + return *this; + } protected: // data members - const context::handle_t context_handle_; - const link::handle_t handle_; - link::options_t options_; - bool owning; + context::handle_t context_handle_; + link::handle_t handle_; + link::options_t options_; + bool owning; // this field is mutable only for enabling move construction; other // than in that case it must not be altered }; diff --git a/src/cuda/api/module.hpp b/src/cuda/api/module.hpp index 409c3e8f..7d8b5eba 100644 --- a/src/cuda/api/module.hpp +++ b/src/cuda/api/module.hpp @@ -185,8 +185,17 @@ class module_t { public: // operators - module_t& operator=(const module_t& other) = delete; - module_t& operator=(module_t&& other) = delete; + module_t& operator=(const module_t&) = delete; + module_t& operator=(module_t&& other) noexcept + { + ::std::swap(device_id_, other.device_id_); + ::std::swap(context_handle_, other.context_handle_); + ::std::swap(handle_, other.handle_); + ::std::swap(options_, other.options_); + ::std::swap(owning_, other.owning_); + ::std::swap(holds_pc_refcount_unit, holds_pc_refcount_unit); + return *this; + } protected: // data members device::id_t device_id_; diff --git a/src/cuda/api/multi_wrapper_impls/module.hpp b/src/cuda/api/multi_wrapper_impls/module.hpp index 12d5ca4c..079445dc 100644 --- a/src/cuda/api/multi_wrapper_impls/module.hpp +++ b/src/cuda/api/multi_wrapper_impls/module.hpp @@ -48,7 +48,7 @@ inline cuda::kernel_t module_t::get_kernel(const char* name) const namespace module { -namespace detail_{ +namespace detail_ { template module_t create(const context_t& context, const void* module_data, Creator creator_function) @@ -61,10 +61,14 @@ module_t create(const context_t& context, const void* module_data, Creator creat + cuda::detail_::ptr_as_hex(module_data) + " within " + context::detail_::identify(context)); bool do_take_ownership { true }; + bool doesnt_hold_pc_refcount_unit { false }; + // TODO: Do we want to allow holding a refcount unit here, if context is + // the primary context? + // TODO: Make sure the default-constructed options correspond to what cuModuleLoadData uses as defaults - return detail_::construct( - context.device_id(), context.handle(), new_module_handle, - link::options_t{}, do_take_ownership); + return detail_::wrap( + context.device_id(), context.handle(), new_module_handle, + link::options_t{}, do_take_ownership, doesnt_hold_pc_refcount_unit); } diff --git a/src/cuda/api/primary_context.hpp b/src/cuda/api/primary_context.hpp index 21c350de..80be8d9a 100644 --- a/src/cuda/api/primary_context.hpp +++ b/src/cuda/api/primary_context.hpp @@ -195,8 +195,8 @@ class primary_context_t : public context_t { public: // operators - context_t& operator=(const primary_context_t& other) = delete; - context_t& operator=(primary_context_t&& other) = delete; + primary_context_t& operator=(const primary_context_t& other) = delete; + primary_context_t& operator=(primary_context_t&& other) = default; public: // mutators of the proxied primary context, but not of the proxy diff --git a/src/cuda/api/stream.hpp b/src/cuda/api/stream.hpp index 677a445b..9101ffc6 100644 --- a/src/cuda/api/stream.hpp +++ b/src/cuda/api/stream.hpp @@ -837,7 +837,15 @@ class stream_t { // TODO: Do we really want to allow assignments? Hmm... probably not, it's // too risky - someone might destroy one of the streams and use the others stream_t& operator=(const stream_t& other) = delete; - stream_t& operator=(stream_t& other) = delete; + stream_t& operator=(stream_t& other) noexcept + { + ::std::swap(device_id_, other.device_id_); + ::std::swap(context_handle_, other.context_handle_); + ::std::swap(handle_, other.handle_); + ::std::swap(owning, other.owning); + ::std::swap(holds_pc_refcount_unit, holds_pc_refcount_unit); + return *this; + } public: // friendship @@ -864,11 +872,11 @@ class stream_t { } protected: // data members - const device::id_t device_id_; - const context::handle_t context_handle_; - const stream::handle_t handle_; - bool owning; - bool holds_pc_refcount_unit; + device::id_t device_id_; + context::handle_t context_handle_; + stream::handle_t handle_; + bool owning; + bool holds_pc_refcount_unit; // When context_handle_ is the handle of a primary context, this event may // be "keeping that context alive" through the refcount - in which case // it must release its refcount unit on destruction diff --git a/src/cuda/nvrtc/compilation_options.hpp b/src/cuda/nvrtc/compilation_options.hpp index 0d2dda96..6acf858e 100644 --- a/src/cuda/nvrtc/compilation_options.hpp +++ b/src/cuda/nvrtc/compilation_options.hpp @@ -261,12 +261,22 @@ struct compilation_options_t { /** * A sequence of directories to be searched for headers. These paths are searched _after_ the * list of headers given to nvrtcCreateProgram. + * + * @note The members here are `std::string`'s rather than `const char*` or `std::string_view`'s, + * since this class is a value-type, and cannot rely someone else keeping these strings alive. + * + * @todo In C++17, consider making the elements `std::filesystem::path`'s. */ ::std::vector<::std::string> additional_include_paths; /** * Header files to preinclude during preprocessing of the source. * + * @note The members here are `std::string`'s rather than `const char*` or `std::string_view`'s, + * since this class is a value-type, and cannot rely someone else keeping these strings alive. + * + * @todo In C++17, consider making the elements `std::filesystem::path`'s. + * * @todo Check how these strings are interpreted. Do they need quotation marks? brackets? full paths? */ ::std::vector<::std::string> preinclude_files;