Fix symbol export - #8037
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test |
9de013e to
43cc963
Compare
d616293 to
fe5d0c3
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughApplies a new export/visibility header and macro (CUML_EXPORT/CUML_HIDDEN); annotates many namespaces and some macro-generated functions with CUML_EXPORT; marks numerous explicit template instantiations in sources as exported; removes certain default parameters (LARS, SVR); updates a CMake target property and SPDX years. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cpp/include/cuml/prims/opg/comm_utils.h (1)
1-10:⚠️ Potential issue | 🔴 CriticalRestore a self include guard for this header
comm_utils.hcontains in-header template definitions without a file-level include guard. This can trigger multiple-definition errors when the header is included in multiple translation units (as occurs here with 10 different .cu files). Add#pragma onceat the top of the file.Suggested fix
/* * SPDX-FileCopyrightText: Copyright (c) 2019-2026, NVIDIA CORPORATION. * SPDX-License-Identifier: Apache-2.0 */ +#pragma once + `#include` <cuml/common/export.hpp> `#include` <raft/core/comms.hpp> `#include` <raft/util/cuda_utils.cuh>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/include/cuml/prims/opg/comm_utils.h` around lines 1 - 10, comm_utils.h lacks a file-level include guard which causes multiple-definition linker errors because it defines in-header templates used across many translation units; fix by adding a header guard such as placing `#pragma` once at the very top of comm_utils.h so the template definitions (e.g., any templates in this file used by functions/classes in comm_utils.h) are included only once per translation unit.cpp/include/cuml/common/pinned_host_vector.hpp (1)
12-55:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
pinned_host_vectorhas UB on default destruction and leaks onresize.
data_andsize_are uninitialized in the default constructor, but the destructor always deallocates them. Also,resizereplacesdata_without freeing existing storage.Suggested fix
template <typename T> class pinned_host_vector { public: - pinned_host_vector() = default; + pinned_host_vector() : data_{nullptr}, size_{0} {} @@ - ~pinned_host_vector() { pinned_mr.deallocate_sync(data_, size_ * sizeof(T)); } + ~pinned_host_vector() + { + if (data_ != nullptr) { pinned_mr.deallocate_sync(data_, size_ * sizeof(T)); } + } @@ void resize(std::size_t n) { - size_ = n; - data_ = static_cast<T*>(pinned_mr.allocate_sync(n * sizeof(T))); - std::uninitialized_fill(data_, data_ + n, static_cast<T>(0)); + if (data_ != nullptr) { pinned_mr.deallocate_sync(data_, size_ * sizeof(T)); } + size_ = n; + data_ = (n == 0) ? nullptr : static_cast<T*>(pinned_mr.allocate_sync(n * sizeof(T))); + if (data_ != nullptr) { std::uninitialized_fill(data_, data_ + n, static_cast<T>(0)); } } @@ private: rmm::mr::pinned_host_memory_resource pinned_mr{}; - T* data_; - std::size_t size_; + T* data_; + std::size_t size_; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/include/cuml/common/pinned_host_vector.hpp` around lines 12 - 55, The class has undefined behavior because data_ and size_ are uninitialized and the destructor unconditionally deallocates them, and resize leaks because it overwrites data_ without freeing existing memory. Fix by initializing members (set data_ = nullptr and size_ = 0) in the default constructor (and member initializers), change the destructor to only call pinned_mr.deallocate_sync if data_ != nullptr and size_ > 0, and make resize exception-safe by allocating a new buffer into a temporary pointer, fill it, then swap/set data_ and size_ and deallocate the old buffer (only if non-null) — reference the class pinned_host_vector, its constructor, destructor (~pinned_host_vector), resize method, and members data_ and size_ when applying the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/include/cuml/linear_model/qn.h`:
- Around line 7-12: The header uses the CUML_EXPORT macro in the namespace
declaration (namespace ML { namespace CUML_EXPORT GLM {) but does not include
its definition; add an `#include` of cuml/common/export.hpp at the top of qn.h so
the CUML_EXPORT macro is defined and the header is self-sufficient and
consistent with other public headers.
In `@cpp/include/cuml/prims/opg/matrix/matrix_utils.hpp`:
- Around line 5-10: The header matrix_utils.hpp is missing an include guard
which causes redeclaration of functions like the randomize(...) overloads; add a
header guard (e.g., wrap the entire file in `#ifndef` SOME_UNIQUE_SYMBOL / `#define`
SOME_UNIQUE_SYMBOL ... `#endif`) or add `#pragma` once at the top of
matrix_utils.hpp so the randomize declarations (and any other symbols) are not
reprocessed on multiple includes.
In `@cpp/include/cuml/svm/svm_model.h`:
- Line 7: The header uses the CUML_EXPORT macro before it's defined; add an
include for the export header so CUML_EXPORT is defined (e.g., include
<cuml/common/export.hpp> near the top of svm_model.h before the namespace/usage
of CUML_EXPORT) so symbols like CUML_EXPORT and the namespace declaration
"namespace CUML_EXPORT ML" compile correctly.
In `@cpp/include/cuml/tree/algo_helper.h`:
- Line 8: The header uses the macro CUML_EXPORT in the namespace declaration but
doesn't include its definition; add an `#include` for the header that defines
CUML_EXPORT (e.g., export.hpp) at the top of cpp/include/cuml/tree/algo_helper.h
before the line with "namespace CUML_EXPORT ML {" so the macro is available when
the namespace is declared and compilation succeeds.
In `@cpp/src/svm/svr.cu`:
- Around line 24-42: The explicit template instantiations for svrFitSparse are
missing the CUML_EXPORT annotation, so add CUML_EXPORT to both
svrFitSparse<float> and svrFitSparse<double> instantiations in svr.cu (matching
how svrFit is exported) to ensure the sparse API symbols are exported for
external linking; locate the svrFitSparse template instantiation blocks in the
file and prepend CUML_EXPORT to each template declaration for float and double.
---
Outside diff comments:
In `@cpp/include/cuml/common/pinned_host_vector.hpp`:
- Around line 12-55: The class has undefined behavior because data_ and size_
are uninitialized and the destructor unconditionally deallocates them, and
resize leaks because it overwrites data_ without freeing existing memory. Fix by
initializing members (set data_ = nullptr and size_ = 0) in the default
constructor (and member initializers), change the destructor to only call
pinned_mr.deallocate_sync if data_ != nullptr and size_ > 0, and make resize
exception-safe by allocating a new buffer into a temporary pointer, fill it,
then swap/set data_ and size_ and deallocate the old buffer (only if non-null) —
reference the class pinned_host_vector, its constructor, destructor
(~pinned_host_vector), resize method, and members data_ and size_ when applying
the changes.
In `@cpp/include/cuml/prims/opg/comm_utils.h`:
- Around line 1-10: comm_utils.h lacks a file-level include guard which causes
multiple-definition linker errors because it defines in-header templates used
across many translation units; fix by adding a header guard such as placing
`#pragma` once at the very top of comm_utils.h so the template definitions (e.g.,
any templates in this file used by functions/classes in comm_utils.h) are
included only once per translation unit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1464725a-52fd-4012-acbc-d6898aec44b0
📒 Files selected for processing (108)
cpp/CMakeLists.txtcpp/bench/sg/svr.cucpp/include/cuml/cluster/dbscan.hppcpp/include/cuml/cluster/hdbscan.hppcpp/include/cuml/cluster/kmeans.hppcpp/include/cuml/cluster/kmeans_params.hppcpp/include/cuml/cluster/linkage.hppcpp/include/cuml/cluster/spectral_clustering.hppcpp/include/cuml/common/callback.hppcpp/include/cuml/common/distance_type.hppcpp/include/cuml/common/export.hppcpp/include/cuml/common/logger.hppcpp/include/cuml/common/pinned_host_vector.hppcpp/include/cuml/common/utils.hppcpp/include/cuml/datasets/make_arima.hppcpp/include/cuml/datasets/make_blobs.hppcpp/include/cuml/datasets/make_regression.hppcpp/include/cuml/decomposition/params.hppcpp/include/cuml/decomposition/pca.hppcpp/include/cuml/decomposition/pca_mg.hppcpp/include/cuml/decomposition/sign_flip_mg.hppcpp/include/cuml/decomposition/tsvd.hppcpp/include/cuml/decomposition/tsvd_mg.hppcpp/include/cuml/ensemble/randomforest.hppcpp/include/cuml/explainer/kernel_shap.hppcpp/include/cuml/explainer/permutation_shap.hppcpp/include/cuml/explainer/tree_shap.hppcpp/include/cuml/fil/constants.hppcpp/include/cuml/fil/decision_forest.hppcpp/include/cuml/fil/detail/specializations/device_initialization_macros.hppcpp/include/cuml/fil/detail/specializations/infer_macros.hppcpp/include/cuml/fil/exceptions.hppcpp/include/cuml/fil/forest_model.hppcpp/include/cuml/fil/infer_kind.hppcpp/include/cuml/fil/postproc_ops.hppcpp/include/cuml/fil/tree_layout.hppcpp/include/cuml/fil/treelite_importer.hppcpp/include/cuml/forest/exceptions.hppcpp/include/cuml/forest/integrations/treelite.hppcpp/include/cuml/forest/traversal/traversal_forest.hppcpp/include/cuml/forest/traversal/traversal_node.hppcpp/include/cuml/forest/traversal/traversal_order.hppcpp/include/cuml/genetic/common.hcpp/include/cuml/genetic/genetic.hcpp/include/cuml/genetic/node.hcpp/include/cuml/genetic/program.hcpp/include/cuml/linear_model/glm.hppcpp/include/cuml/linear_model/ols_mg.hppcpp/include/cuml/linear_model/preprocess_mg.hppcpp/include/cuml/linear_model/qn.hcpp/include/cuml/linear_model/qn_mg.hppcpp/include/cuml/linear_model/ridge_mg.hppcpp/include/cuml/manifold/common.hppcpp/include/cuml/manifold/spectral_embedding.hppcpp/include/cuml/manifold/tsne.hcpp/include/cuml/manifold/umap.hppcpp/include/cuml/manifold/umapparams.hcpp/include/cuml/matrix/kernel_params.hppcpp/include/cuml/metrics/metrics.hppcpp/include/cuml/neighbors/knn.hppcpp/include/cuml/neighbors/knn_mg.hppcpp/include/cuml/neighbors/knn_sparse.hppcpp/include/cuml/prims/opg/comm_utils.hcpp/include/cuml/prims/opg/linalg/eig.hppcpp/include/cuml/prims/opg/linalg/gemm.hppcpp/include/cuml/prims/opg/linalg/lstsq.hppcpp/include/cuml/prims/opg/linalg/mean_squared_error.hppcpp/include/cuml/prims/opg/linalg/mm_aTa.hppcpp/include/cuml/prims/opg/linalg/mv_aTb.hppcpp/include/cuml/prims/opg/linalg/norm.hppcpp/include/cuml/prims/opg/linalg/svd.hppcpp/include/cuml/prims/opg/matrix/data.hppcpp/include/cuml/prims/opg/matrix/math.hppcpp/include/cuml/prims/opg/matrix/matrix_utils.hppcpp/include/cuml/prims/opg/matrix/part_descriptor.hppcpp/include/cuml/prims/opg/stats/cov.hppcpp/include/cuml/prims/opg/stats/mean.hppcpp/include/cuml/prims/opg/stats/mean_center.hppcpp/include/cuml/prims/opg/stats/stddev.hppcpp/include/cuml/solvers/cd_mg.hppcpp/include/cuml/solvers/lars.hppcpp/include/cuml/solvers/params.hppcpp/include/cuml/solvers/solver.hppcpp/include/cuml/svm/linear.hppcpp/include/cuml/svm/svc.hppcpp/include/cuml/svm/svm_model.hcpp/include/cuml/svm/svm_parameter.hcpp/include/cuml/svm/svr.hppcpp/include/cuml/tree/algo_helper.hcpp/include/cuml/tree/decisiontree.hppcpp/include/cuml/tsa/arima_common.hcpp/include/cuml/tsa/auto_arima.hcpp/include/cuml/tsa/batched_arima.hppcpp/include/cuml/tsa/batched_kalman.hppcpp/include/cuml/tsa/holtwinters.hcpp/include/cuml/tsa/holtwinters_params.hcpp/include/cuml/tsa/stationarity.hcpp/src/decisiontree/batched-levelalgo/quantiles.cuhcpp/src/glm/glm.cucpp/src/kmeans/kmeans_fit.cucpp/src/kmeans/kmeans_predict.cucpp/src/kmeans/kmeans_transform.cucpp/src/matrix/kernel_params.cppcpp/src/metrics/trustworthiness.cucpp/src/randomforest/randomforest.cucpp/src/solver/lars_impl.cuhcpp/src/svm/svc.cucpp/src/svm/svr.cu
|
Addressed all CodeRabbit feedback: Inline comments (all fixed):
Outside-diff comments:
Also added |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/include/cuml/common/pinned_host_vector.hpp`:
- Around line 34-39: The resize() is not exception-safe because it deallocates
old storage and updates size_ before guaranteeing the new
allocation/initialization succeeds; if allocate_sync() or
std::uninitialized_fill throws you end up with data_ pointing at freed memory.
Fix by allocating to a temporary pointer (e.g., T* new_data = (n==0? nullptr :
static_cast<T*>(pinned_mr.allocate_sync(...)))), perform initialization on
new_data (std::uninitialized_fill) and only after both allocation and
initialization succeed deallocate the old data_ via pinned_mr.deallocate_sync,
update data_ = new_data and size_ = n; if initialization/allocation fails,
ensure new_data is deallocated (or use RAII) so no leaks occur.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 62d15a6c-98b6-4830-adcb-7d871d12295d
📒 Files selected for processing (13)
cpp/include/cuml/common/pinned_host_vector.hppcpp/include/cuml/linear_model/qn.hcpp/include/cuml/prims/opg/comm_utils.hcpp/include/cuml/prims/opg/matrix/matrix_utils.hppcpp/include/cuml/svm/svm_model.hcpp/include/cuml/tree/algo_helper.hcpp/src/arima/arima_common.cucpp/src/decisiontree/decisiontree.cucpp/src/glm/glm.cucpp/src/glm/qn_mg.cucpp/src/solver/lars.cucpp/src/svm/linear.cucpp/src/svm/svr.cu
✅ Files skipped from review due to trivial changes (4)
- cpp/include/cuml/prims/opg/comm_utils.h
- cpp/include/cuml/prims/opg/matrix/matrix_utils.hpp
- cpp/include/cuml/tree/algo_helper.h
- cpp/src/glm/glm.cu
🚧 Files skipped from review as they are similar to previous changes (3)
- cpp/include/cuml/svm/svm_model.h
- cpp/include/cuml/linear_model/qn.h
- cpp/src/svm/svr.cu
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cpp/include/cuml/common/pinned_host_vector.hpp (1)
34-39:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
resize()is still not exception-safe and can double-free on allocation failure.
data_is deallocated before new allocation/initialization is guaranteed. Ifallocate_sync()throws,data_remains a dangling non-null pointer and the destructor can deallocate it again.Suggested fix
void resize(std::size_t n) { - if (data_ != nullptr) { pinned_mr.deallocate_sync(data_, size_ * sizeof(T)); } - size_ = n; - data_ = (n == 0) ? nullptr : static_cast<T*>(pinned_mr.allocate_sync(n * sizeof(T))); - if (data_ != nullptr) { std::uninitialized_fill(data_, data_ + n, static_cast<T>(0)); } + T* new_data = (n == 0) ? nullptr : static_cast<T*>(pinned_mr.allocate_sync(n * sizeof(T))); + try { + if (new_data != nullptr) { std::uninitialized_fill(new_data, new_data + n, static_cast<T>(0)); } + } catch (...) { + if (new_data != nullptr) { pinned_mr.deallocate_sync(new_data, n * sizeof(T)); } + throw; + } + + if (data_ != nullptr) { pinned_mr.deallocate_sync(data_, size_ * sizeof(T)); } + data_ = new_data; + size_ = n; }Based on learnings: "Check for GPU memory leaks in exception paths and ensure cleanup is properly invoked using RAII or try-finally patterns."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/include/cuml/common/pinned_host_vector.hpp` around lines 34 - 39, resize() currently deallocates data_ before performing the new allocation/initialization which can leave data_ as a dangling non-null pointer if pinned_mr.allocate_sync throws; change resize(std::size_t n) to first allocate into a temporary pointer (e.g. T* new_data = (n==0)? nullptr : static_cast<T*>(pinned_mr.allocate_sync(...))) and only after allocation and initialization (use std::uninitialized_fill_n or placement-new loop and catch exceptions to deallocate new_data) deallocate the old data_ via pinned_mr.deallocate_sync(data_, size_*sizeof(T)) and then assign data_ = new_data and size_ = n, ensuring that on allocation or initialization failure the original data_ remains intact and any partially constructed new_data is cleaned up to avoid double-free or leaks; reference symbols: resize, data_, size_, pinned_mr.allocate_sync, pinned_mr.deallocate_sync, and std::uninitialized_fill.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cpp/include/cuml/common/pinned_host_vector.hpp`:
- Around line 34-39: resize() currently deallocates data_ before performing the
new allocation/initialization which can leave data_ as a dangling non-null
pointer if pinned_mr.allocate_sync throws; change resize(std::size_t n) to first
allocate into a temporary pointer (e.g. T* new_data = (n==0)? nullptr :
static_cast<T*>(pinned_mr.allocate_sync(...))) and only after allocation and
initialization (use std::uninitialized_fill_n or placement-new loop and catch
exceptions to deallocate new_data) deallocate the old data_ via
pinned_mr.deallocate_sync(data_, size_*sizeof(T)) and then assign data_ =
new_data and size_ = n, ensuring that on allocation or initialization failure
the original data_ remains intact and any partially constructed new_data is
cleaned up to avoid double-free or leaks; reference symbols: resize, data_,
size_, pinned_mr.allocate_sync, pinned_mr.deallocate_sync, and
std::uninitialized_fill.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 076c8764-1f9b-497a-b830-d21a2d85d049
📒 Files selected for processing (13)
cpp/include/cuml/common/pinned_host_vector.hppcpp/include/cuml/linear_model/qn.hcpp/include/cuml/prims/opg/comm_utils.hcpp/include/cuml/prims/opg/matrix/matrix_utils.hppcpp/include/cuml/svm/svm_model.hcpp/include/cuml/tree/algo_helper.hcpp/src/arima/arima_common.cucpp/src/decisiontree/decisiontree.cucpp/src/glm/glm.cucpp/src/glm/qn_mg.cucpp/src/solver/lars.cucpp/src/svm/linear.cucpp/src/svm/svr.cu
✅ Files skipped from review due to trivial changes (3)
- cpp/include/cuml/prims/opg/matrix/matrix_utils.hpp
- cpp/include/cuml/prims/opg/comm_utils.h
- cpp/src/glm/qn_mg.cu
🚧 Files skipped from review as they are similar to previous changes (3)
- cpp/include/cuml/tree/algo_helper.h
- cpp/include/cuml/linear_model/qn.h
- cpp/src/solver/lars.cu
|
487cc16 shows CI mostly passing, including the upstream CUVS and RAFT changes. The only failing tests are due to challenges in propagating the correct NVRTC dependencies all the way downstream in Conda when we build CUVS from source, but that is not critical to demonstrate here. The various other passing tests and examples demonstrate that all of the necessary symbols are being exported, which is the main thing we're concerned about. |
|
OK, now that we're seeing a seg fault here it's easier to debug. The problem is that the decision we made in NVIDIA/raft#3006 to continue exporting detail functions (see NVIDIA/raft#3006 (comment)) in raft is a problem because now every library that uses header-only components of raft is reexporting the symbols in raft and therefore potentially susceptible to symbol conflicts. It isn't a new problem, but it's explicitly showing up consistently in this PR now. That means that at least for the header-only parts of raft we have to avoid exporting them now to avoid this problem, including the detail bits that we hid before. |
Set CXX_VISIBILITY_PRESET=hidden and CUDA_VISIBILITY_PRESET=hidden on cuml_objs so that only symbols explicitly marked CUML_EXPORT are visible in the shared library. This prevents symbol interposition between libcuml.so and libcuvs.so when both link raft headers that instantiate identical template specializations in detail namespaces. Changes: - Add cpp/include/cuml/common/export.hpp defining CUML_EXPORT macro - Set hidden visibility presets on cuml_objs target in CMakeLists.txt - Annotate all public API declarations in cpp/include/cuml/ with CUML_EXPORT - Wrap implementation details in cpp/src/ with anonymous namespaces - Exclude export.hpp from Doxygen
|
https://github.com/rapidsai/cuml/actions/runs/26194638861?pr=8037 shows enough CI passing with the artifacts from NVIDIA/cuvs#2101 and NVIDIA/raft#3019 (there are some unrelated failures in tests but all symbol handling looks fine). |
# Conflicts: # cpp/include/cuml/fil/constants.hpp # cpp/include/cuml/fil/decision_forest.hpp # cpp/include/cuml/fil/detail/specializations/device_initialization_macros.hpp # cpp/include/cuml/fil/detail/specializations/infer_macros.hpp # cpp/include/cuml/fil/exceptions.hpp # cpp/include/cuml/fil/forest_model.hpp # cpp/include/cuml/fil/infer_kind.hpp # cpp/include/cuml/fil/postproc_ops.hpp # cpp/include/cuml/fil/tree_layout.hpp # cpp/include/cuml/fil/treelite_importer.hpp # cpp/include/cuml/forest/exceptions.hpp # cpp/include/cuml/forest/integrations/treelite.hpp # cpp/include/cuml/forest/traversal/traversal_forest.hpp # cpp/include/cuml/forest/traversal/traversal_node.hpp # cpp/include/cuml/forest/traversal/traversal_order.hpp
|
/merge |
This PR adds symbol visibility controls to cuml to avoid exporting weak symbols that it shouldn't.
Contributes to https://github.com/rapidsai/build-infra/issues/53