Fix memory leaks in barrier #584
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughReplaces raw cuSPARSE dense-vector descriptors with a new RAII wrapper Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/dual_simplex/barrier.cu (1)
2298-2306: Initialisecusparse_dy_before using it.Line 2302 calls
transpose_spmvwithdata.cusparse_dy_, but that wrapper is never reinitialised afterdata.d_dy_is resized. The descriptor staysnullptr, so the very first cusparse call dereferences an invalid handle. Please wrapdata.d_dy_before using it, e.g.:+ data.cusparse_dy_ = data.cusparse_view_.create_vector(data.d_dy_); // r1 <- A'*dy - r1 data.cusparse_view_.transpose_spmv(1.0, data.cusparse_dy_, -1.0, data.cusparse_r1_);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cpp/src/dual_simplex/barrier.cu(4 hunks)cpp/src/dual_simplex/cusparse_info.hpp(1 hunks)cpp/src/dual_simplex/cusparse_view.cu(5 hunks)cpp/src/dual_simplex/cusparse_view.hpp(2 hunks)cpp/src/dual_simplex/sparse_cholesky.cuh(0 hunks)
💤 Files with no reviewable changes (1)
- cpp/src/dual_simplex/sparse_cholesky.cuh
🧰 Additional context used
🧬 Code graph analysis (2)
cpp/src/dual_simplex/cusparse_view.hpp (2)
cpp/src/dual_simplex/cusparse_view.cu (2)
cusparse_view_t(117-237)cusparse_view_t(240-244)cpp/src/dual_simplex/barrier.cu (13)
alpha(835-841)alpha(835-838)alpha(1198-1232)alpha(1198-1207)alpha(1235-1266)alpha(1235-1239)alpha(1269-1296)alpha(1269-1272)alpha(1518-1524)alpha(1518-1521)alpha(2195-2201)alpha(2195-2198)y(603-603)
cpp/src/dual_simplex/cusparse_view.cu (3)
cpp/src/dual_simplex/barrier.cu (13)
y(603-603)alpha(835-841)alpha(835-838)alpha(1198-1232)alpha(1198-1207)alpha(1235-1266)alpha(1235-1239)alpha(1269-1296)alpha(1269-1272)alpha(1518-1524)alpha(1518-1521)alpha(2195-2201)alpha(2195-2198)cpp/src/dual_simplex/cusparse_view.hpp (7)
cusparse_view_t(28-28)cusparse_view_t(29-29)vec(31-31)spmv(34-37)alpha(38-41)alpha(47-50)transpose_spmv(43-46)cpp/src/utilities/copy_helpers.hpp (22)
device_copy(237-243)device_copy(237-238)device_copy(254-260)device_copy(254-256)device_copy(271-277)device_copy(271-272)device_copy(286-303)device_copy(286-286)host_copy(118-125)host_copy(118-118)host_copy(136-151)host_copy(136-136)host_copy(162-165)host_copy(162-162)host_copy(175-181)host_copy(175-175)host_copy(192-198)host_copy(192-192)host_copy(209-212)host_copy(209-209)host_copy(223-226)host_copy(223-223)
🪛 Clang (14.0.6)
cpp/src/dual_simplex/cusparse_view.hpp
[error] 9-9: 'dual_simplex/cusparse_dnvector.hpp' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: wheel-build-cuopt-server / 13.0.2, 3.13, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-server / 12.9.1, 3.13, amd64, rockylinux8
- GitHub Check: conda-cpp-build / 12.9.1, 3.10, arm64, rockylinux8
- GitHub Check: conda-cpp-build / 13.0.2, 3.10, amd64, rockylinux8
- GitHub Check: conda-cpp-build / 13.0.2, 3.10, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-sh-client / 13.0.2, 3.10, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.13, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.12, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.10, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.11, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.12, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.10, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.13, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.11, amd64, rockylinux8
| /* clang-format on */ | ||
| #pragma once | ||
|
|
||
| #include <dual_simplex/cusparse_dnvector.hpp> |
There was a problem hiding this comment.
Fix missing header include.
Line 9 adds #include <dual_simplex/cusparse_dnvector.hpp>, but Clang reports that header is not found. This will break every build unless the file is added or the include path is corrected (for example, pointing at the actual header if it lives under a different name). Please add the missing header or fix the include path. Based on static analysis hints.
🧰 Tools
🪛 Clang (14.0.6)
[error] 9-9: 'dual_simplex/cusparse_dnvector.hpp' file not found
(clang-diagnostic-error)
🤖 Prompt for AI Agents
In cpp/src/dual_simplex/cusparse_view.hpp around line 9, the new include
#include <dual_simplex/cusparse_dnvector.hpp> cannot be found by Clang; either
add the missing header file at that path in the repo or change the include to
the correct existing header path/name (or correct the case) and update the build
include paths (e.g., CMakeLists.txt target_include_directories) so the compiler
can locate it; after making the change, run a full build to confirm the header
resolves.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cpp/src/dual_simplex/cusparse_dnvector.hpp(1 hunks)
🧰 Additional context used
🪛 Clang (14.0.6)
cpp/src/dual_simplex/cusparse_dnvector.hpp
[error] 8-8: 'raft/sparse/detail/cusparse_wrappers.h' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.13, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.10, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.11, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.10, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.12, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.11, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-sh-client / 13.0.2, 3.10, amd64, rockylinux8
🔇 Additional comments (1)
cpp/src/dual_simplex/cusparse_dnvector.hpp (1)
31-35: Verify and fix const-correctness by using cuSPARSE's const-aware API.The constructor accepts a
constreference tormm::device_uvector<f_t>but usesconst_castto remove the const qualifier. cuSPARSE provides a const-aware variant,cusparseCreateConstDnVec(), which acceptsconst void*and returnscusparseConstDnVecDescr_tfor read-only use.Check if the
raft::sparse::detail::cusparsecreatednvecwrapper exposes this const variant. If available, use it instead ofconst_castto properly honor the const contract of the parameter. If the wrapper doesn't support the const variant, change the constructor parameter to a non-const reference.
|
|
||
| template <typename i_t, typename f_t> | ||
| struct cuoptdnvector_t { | ||
| cusparseDnVecDescr_t vec_descr{nullptr}; |
There was a problem hiding this comment.
Public member breaks RAII encapsulation.
The vec_descr member is public, which allows external code to directly manipulate or destroy the descriptor, defeating the RAII pattern. This could lead to double-free, use-after-free, or leaked descriptors if external code interferes with the managed resource.
Apply this diff to make the member private and add a public accessor:
+private:
cusparseDnVecDescr_t vec_descr{nullptr};
+public:
+ cusparseDnVecDescr_t get() const { return vec_descr; }
+
cuoptdnvector_t() = default;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cusparseDnVecDescr_t vec_descr{nullptr}; | |
| private: | |
| cusparseDnVecDescr_t vec_descr{nullptr}; | |
| public: | |
| cusparseDnVecDescr_t get() const { return vec_descr; } | |
| cuoptdnvector_t() = default; |
🤖 Prompt for AI Agents
In cpp/src/dual_simplex/cusparse_dnvector.hpp at line 16, the public member
`cusparseDnVecDescr_t vec_descr{nullptr};` breaks RAII; change its visibility to
private, keep the variable name, and add a public accessor method (e.g.,
`cusparseDnVecDescr_t get_vec_descr() const noexcept`) to expose the handle
without allowing external mutation or destruction; ensure the class still
manages creation/destruction of the descriptor in its ctor/dtor and do not
expose non-const references or raw pointer setters that would allow external
code to take ownership.
| cuoptdnvector_t() = default; | ||
|
|
||
| cuoptdnvector_t(cuoptdnvector_t const& other) = delete; | ||
| cuoptdnvector_t& operator=(cuoptdnvector_t const& other) = delete; |
There was a problem hiding this comment.
Add move constructor to complete move semantics.
The struct defines move assignment but lacks a move constructor. Without an explicit move constructor, the compiler generates a default one that copies vec_descr without nulling the source, which would cause a double-free when both objects are destroyed.
Apply this diff to add a move constructor:
cuoptdnvector_t() = default;
+ cuoptdnvector_t(cuoptdnvector_t&& other) noexcept
+ : vec_descr(other.vec_descr)
+ {
+ other.vec_descr = nullptr;
+ }
+
cuoptdnvector_t(cuoptdnvector_t const& other) = delete;
cuoptdnvector_t& operator=(cuoptdnvector_t const& other) = delete;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cuoptdnvector_t() = default; | |
| cuoptdnvector_t(cuoptdnvector_t const& other) = delete; | |
| cuoptdnvector_t& operator=(cuoptdnvector_t const& other) = delete; | |
| cuoptdnvector_t() = default; | |
| cuoptdnvector_t(cuoptdnvector_t&& other) noexcept | |
| : vec_descr(other.vec_descr) | |
| { | |
| other.vec_descr = nullptr; | |
| } | |
| cuoptdnvector_t(cuoptdnvector_t const& other) = delete; | |
| cuoptdnvector_t& operator=(cuoptdnvector_t const& other) = delete; |
🤖 Prompt for AI Agents
In cpp/src/dual_simplex/cusparse_dnvector.hpp around lines 18 to 21, the struct
defines move assignment but lacks an explicit move constructor so the
compiler-generated one will copy vec_descr and not null out the source, risking
a double-free; add an explicit move constructor that steals the resource by
copying vec_descr from the source, sets the source.vec_descr to nullptr (or
equivalent empty state), and leaves other members moved/initialized
appropriately so ownership is transferred safely.
| cuoptdnvector_t& operator=(cuoptdnvector_t&& other) noexcept | ||
| { | ||
| if (vec_descr != nullptr) { RAFT_CUSPARSE_TRY(cusparseDestroyDnVec(vec_descr)); } | ||
| vec_descr = other.vec_descr; | ||
| other.vec_descr = nullptr; | ||
| return *this; | ||
| } |
There was a problem hiding this comment.
Add self-assignment check to move assignment operator.
The move assignment operator doesn't guard against self-assignment. If this == &other, the existing descriptor is destroyed at line 25, then the null pointer is copied back, resulting in a leaked descriptor (the original value is lost before it can be transferred).
Apply this diff to add a self-assignment check:
cuoptdnvector_t& operator=(cuoptdnvector_t&& other) noexcept
{
+ if (this == &other) { return *this; }
if (vec_descr != nullptr) { RAFT_CUSPARSE_TRY(cusparseDestroyDnVec(vec_descr)); }
vec_descr = other.vec_descr;
other.vec_descr = nullptr;
return *this;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cuoptdnvector_t& operator=(cuoptdnvector_t&& other) noexcept | |
| { | |
| if (vec_descr != nullptr) { RAFT_CUSPARSE_TRY(cusparseDestroyDnVec(vec_descr)); } | |
| vec_descr = other.vec_descr; | |
| other.vec_descr = nullptr; | |
| return *this; | |
| } | |
| cuoptdnvector_t& operator=(cuoptdnvector_t&& other) noexcept | |
| { | |
| if (this == &other) { return *this; } | |
| if (vec_descr != nullptr) { RAFT_CUSPARSE_TRY(cusparseDestroyDnVec(vec_descr)); } | |
| vec_descr = other.vec_descr; | |
| other.vec_descr = nullptr; | |
| return *this; | |
| } |
🤖 Prompt for AI Agents
In cpp/src/dual_simplex/cusparse_dnvector.hpp around lines 23 to 29, the move
assignment operator destroys the current descriptor unconditionally which breaks
when assigning to itself; add a self-assignment guard (if (this != &other)) so
you only destroy and take ownership when the source is a different object, then
transfer other.vec_descr to vec_descr and null out other.vec_descr, and return
*this.
| namespace cuopt::linear_programming::dual_simplex { | ||
|
|
||
| template <typename i_t, typename f_t> | ||
| struct cuoptdnvector_t { |
There was a problem hiding this comment.
| struct cuoptdnvector_t { | |
| struct cuopt_cusparse_dnvector_t { |
or
struct cusparse_dnvector_wrapper_t
| ~cusparse_view_t(); | ||
|
|
||
| static cusparseDnVecDescr_t create_vector(const rmm::device_uvector<f_t>& vec); | ||
| cuoptdnvector_t<i_t, f_t> create_vector(const rmm::device_uvector<f_t>& vec); |
There was a problem hiding this comment.
is this a deep copy? you can rename it to something like copy_to_cusparse
There was a problem hiding this comment.
I deleted the copy constructor. I think it should use RVO or move constructor only
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
cpp/src/dual_simplex/cusparse_dnvector.hpp (3)
30-30: Public member still breaks RAII encapsulation.The
vec_descrmember remains public, allowing external code to directly manipulate or destroy the descriptor, which defeats the RAII pattern and can cause double-free or use-after-free bugs.This issue was flagged in a previous review but has not been addressed. Apply this diff to make the member private and add a public accessor:
+private: cusparseDnVecDescr_t vec_descr{nullptr}; +public: + cusparseDnVecDescr_t get() const { return vec_descr; } + cuopt_cusparse_dnvector_t() = default;Then update all usage sites (e.g., Line 288, 290, 331, 333 in
cusparse_view.cu) to call.get()instead of accessing.vec_descrdirectly.
32-35: Add move constructor to prevent double-free.The struct defines move assignment but lacks a move constructor. Without an explicit move constructor, the compiler generates a default one that copies
vec_descrwithout nulling the source, causing a double-free when both objects are destroyed.This issue was flagged in a previous review but has not been addressed. Apply this diff to add a move constructor:
cuopt_cusparse_dnvector_t() = default; + cuopt_cusparse_dnvector_t(cuopt_cusparse_dnvector_t&& other) noexcept + : vec_descr(other.vec_descr) + { + other.vec_descr = nullptr; + } + cuopt_cusparse_dnvector_t(cuopt_cusparse_dnvector_t const& other) = delete;
37-43: Add self-assignment check to move assignment operator.The move assignment operator doesn't guard against self-assignment. If
this == &other, the descriptor at Line 39 is destroyed, then the null pointer is copied back, leaking the original descriptor.This issue was flagged in a previous review but has not been addressed. Apply this diff to add a self-assignment check:
cuopt_cusparse_dnvector_t& operator=(cuopt_cusparse_dnvector_t&& other) noexcept { + if (this == &other) { return *this; } if (vec_descr != nullptr) { RAFT_CUSPARSE_TRY(cusparseDestroyDnVec(vec_descr)); } vec_descr = other.vec_descr; other.vec_descr = nullptr; return *this; }
🧹 Nitpick comments (1)
cpp/src/dual_simplex/cusparse_view.cu (1)
240-244: Consider using NO_THROW variant in destructor.The destructor uses
RAFT_CUSPARSE_TRY, which may throw exceptions. Destructors should generally benoexceptto avoid program termination during stack unwinding.Consider using
CUOPT_CUSPARSE_TRY_NO_THROWinstead (as done incusparse_info.hppat lines 41-49):cusparse_view_t<i_t, f_t>::~cusparse_view_t() { - RAFT_CUSPARSE_TRY(cusparseDestroySpMat(A_)); - RAFT_CUSPARSE_TRY(cusparseDestroySpMat(A_T_)); + CUOPT_CUSPARSE_TRY_NO_THROW(cusparseDestroySpMat(A_)); + CUOPT_CUSPARSE_TRY_NO_THROW(cusparseDestroySpMat(A_T_)); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cpp/src/dual_simplex/barrier.cu(4 hunks)cpp/src/dual_simplex/cusparse_dnvector.hpp(1 hunks)cpp/src/dual_simplex/cusparse_info.hpp(2 hunks)cpp/src/dual_simplex/cusparse_view.cu(5 hunks)cpp/src/dual_simplex/cusparse_view.hpp(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
cpp/src/dual_simplex/cusparse_view.cu (3)
cpp/src/dual_simplex/cusparse_view.hpp (7)
cusparse_view_t(28-28)cusparse_view_t(29-29)vec(31-31)spmv(34-37)alpha(38-41)alpha(47-50)transpose_spmv(43-46)cpp/src/dual_simplex/cusparse_dnvector.hpp (4)
cuopt_cusparse_dnvector_t(32-32)cuopt_cusparse_dnvector_t(34-34)cuopt_cusparse_dnvector_t(45-49)cuopt_cusparse_dnvector_t(51-54)cpp/src/utilities/copy_helpers.hpp (8)
device_copy(237-243)device_copy(237-238)device_copy(254-260)device_copy(254-256)device_copy(271-277)device_copy(271-272)device_copy(286-303)device_copy(286-286)
cpp/src/dual_simplex/cusparse_view.hpp (2)
cpp/src/dual_simplex/cusparse_view.cu (2)
cusparse_view_t(117-237)cusparse_view_t(240-244)cpp/src/dual_simplex/barrier.cu (13)
alpha(835-841)alpha(835-838)alpha(1198-1232)alpha(1198-1207)alpha(1235-1266)alpha(1235-1239)alpha(1269-1296)alpha(1269-1272)alpha(1518-1524)alpha(1518-1521)alpha(2195-2201)alpha(2195-2198)y(603-603)
cpp/src/dual_simplex/cusparse_dnvector.hpp (1)
cpp/src/dual_simplex/cusparse_view.hpp (1)
vec(31-31)
🪛 Clang (14.0.6)
cpp/src/dual_simplex/cusparse_info.hpp
[error] 10-10: 'dual_simplex/cusparse_dnvector.hpp' file not found
(clang-diagnostic-error)
cpp/src/dual_simplex/cusparse_view.hpp
[error] 9-9: 'dual_simplex/cusparse_dnvector.hpp' file not found
(clang-diagnostic-error)
cpp/src/dual_simplex/cusparse_dnvector.hpp
[error] 8-8: 'raft/sparse/detail/cusparse_wrappers.h' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: conda-cpp-build / 13.0.2, 3.10, arm64, rockylinux8
- GitHub Check: conda-cpp-build / 12.9.1, 3.10, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-sh-client / 13.0.2, 3.10, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.12, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.11, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.13, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.12, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.10, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.13, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.11, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.10, amd64, rockylinux8
🔇 Additional comments (8)
cpp/src/dual_simplex/cusparse_info.hpp (2)
40-49: LGTM! Exception-safe destructor implementation.The destructor now correctly uses
CUOPT_CUSPARSE_TRY_NO_THROWfor all cleanup operations and guards each with null checks, ensuring exception safety and preventing double-destroy.
52-55: LGTM! Defensive initialization.Initializing all descriptor members to
nullptris good defensive practice and ensures well-defined state.cpp/src/dual_simplex/cusparse_view.cu (2)
247-251: LGTM! Clean wrapper return.The
create_vectorimplementation correctly constructs and returns the wrapper by value, leveraging move semantics for efficient transfer of ownership.
260-266: LGTM! Consistent wrapper usage.The host-side
spmvoverload correctly creates wrappers from device vectors and passes them to the descriptor-based overload.cpp/src/dual_simplex/cusparse_view.hpp (1)
9-9: LGTM! Consistent API migration to wrappers.The header correctly declares:
- Destructor for cleanup (Line 29)
create_vectorreturning wrapper by value (Line 31) for move-enabled ownership transferspmvandtranspose_spmvtaking wrapper const references (Lines 38-41, 47-50) to avoid copies while maintaining const-correctnessAlso applies to: 29-29, 31-31, 38-41, 47-50
cpp/src/dual_simplex/barrier.cu (3)
1389-1402: LGTM! Member descriptors migrated to RAII wrappers.The member variables have been correctly migrated from raw
cusparseDnVecDescr_ttocuopt_cusparse_dnvector_t<i_t, f_t>wrappers. This ensures automatic cleanup and addresses the memory leaks described in the PR objectives.
2282-2282: LGTM! Local wrapper with automatic cleanup.Creating the wrapper with
autoensures proper RAII cleanup when the variable goes out of scope, preventing the descriptor leak.
3267-3273: LGTM! Member wrapper initialization.The member wrappers are correctly initialized from device vectors in the constructor, establishing proper ownership for the lifetime of the
iteration_data_tobject.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
cpp/src/dual_simplex/cusparse_dnvector.hpp (3)
15-15: Public member still breaks RAII encapsulation.The
vec_descrmember remains public, allowing external code to directly manipulate or destroy the descriptor. This defeats the RAII pattern and can lead to double-free, use-after-free, or leaked descriptors.Apply this diff to make the member private with a public accessor:
+private: cusparseDnVecDescr_t vec_descr{nullptr}; + +public: + cusparseDnVecDescr_t get() const noexcept { return vec_descr; }
17-17: Move constructor still missing.The struct defines move assignment but lacks an explicit move constructor. The compiler-generated default move constructor will copy
vec_descrwithout nulling the source, causing a double-free when both objects are destroyed.Apply this diff to add a move constructor:
cuopt_cusparse_dnvector_t() = default; + cuopt_cusparse_dnvector_t(cuopt_cusparse_dnvector_t&& other) noexcept + : vec_descr(other.vec_descr) + { + other.vec_descr = nullptr; + } + cuopt_cusparse_dnvector_t(cuopt_cusparse_dnvector_t const& other) = delete;
22-28: Self-assignment check still missing in move assignment.The move assignment operator doesn't guard against self-assignment. If
this == &other, the existing descriptor is destroyed at line 24, then the null pointer is copied back, resulting in a leaked descriptor.Apply this diff to add a self-assignment check:
cuopt_cusparse_dnvector_t& operator=(cuopt_cusparse_dnvector_t&& other) noexcept { + if (this == &other) { return *this; } if (vec_descr != nullptr) { RAFT_CUSPARSE_TRY(cusparseDestroyDnVec(vec_descr)); } vec_descr = other.vec_descr; other.vec_descr = nullptr; return *this; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cpp/src/dual_simplex/cusparse_dnvector.hpp(1 hunks)cpp/src/dual_simplex/cusparse_info.hpp(1 hunks)cpp/src/utilities/macros.cuh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/src/dual_simplex/cusparse_info.hpp
🧰 Additional context used
🧬 Code graph analysis (1)
cpp/src/dual_simplex/cusparse_dnvector.hpp (1)
cpp/src/dual_simplex/cusparse_view.hpp (1)
vec(31-31)
🪛 Clang (14.0.6)
cpp/src/dual_simplex/cusparse_dnvector.hpp
[error] 8-8: 'raft/sparse/detail/cusparse_wrappers.h' file not found
(clang-diagnostic-error)
🔇 Additional comments (3)
cpp/src/utilities/macros.cuh (1)
41-54: LGTM! Non-throwing error macro correctly implemented.The
CUOPT_CUSPARSE_TRY_NO_THROWmacro provides exception-safe error handling for cusparse calls in destructors and other noexcept contexts. The implementation correctly checks status, formats error messages, and avoids throwing exceptions.cpp/src/dual_simplex/cusparse_dnvector.hpp (2)
19-20: LGTM! Copy operations correctly deleted.Deleting the copy constructor and copy assignment operator is appropriate for this RAII wrapper, ensuring the resource cannot be accidentally copied.
36-39: LGTM! Destructor now uses exception-safe cleanup.The destructor correctly uses
CUOPT_CUSPARSE_TRY_NO_THROWto ensure it doesn't throw exceptions during cleanup. This properly addresses the exception safety concern from previous reviews.
Update to 26.02
hlinsen
force-pushed
the
fix-barrier-leaks
branch
from
6e797f9 to
36e9bdc
Compare
|
/merge |
rapids-bot
bot
merged commit 06ef927
into
NVIDIA:release/25.12
5 participants
This PR fixes:
Summary by CodeRabbit
Refactor
Bug Fixes