Fix symbol visibility and add checks#1526
Conversation
arhag23
requested review from
akifcorduk,
jameslamb,
nguidotti and
ramakrishnap-nv
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds export visibility controls for public C++/C APIs, hides default symbols in the libcuopt build, exports selected explicit template instantiations, adds a symbol-leak CI check, and updates tests to link the static target. ChangesSymbol Visibility Hiding
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (1)
cpp/include/cuopt/routing/routing_structures.hpp (1)
18-19: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueKeep
cuopt::routing::detailhidden
namespace CUOPT_EXPORT routingmakes the nesteddetailhelpers part of the exported surface. If they’re internal-only, hide that namespace explicitly (or move it outside the exported namespace) so future out-of-line definitions don’t become ABI-visible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/include/cuopt/routing/routing_structures.hpp` around lines 18 - 19, The exported routing namespace is exposing internal detail helpers through the public ABI. Update the namespace declaration around cuopt::routing in routing_structures.hpp so that detail stays hidden, either by removing the export from the enclosing routing namespace or by placing detail outside the exported namespace; use the cuopt::routing and cuopt::routing::detail symbols to keep internal-only helpers out of the exported surface.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ci/check_symbols.sh`:
- Around line 36-46: The forbidden-symbol check in check_symbols.sh is missing
Abseil coverage, so leaked strong absl:: symbols can pass unnoticed. Update the
patterns array in the symbol-check script to include an absl:: entry alongside
the existing cub::, thrust::, grpc::, and other namespaces. Keep the change
focused on the patterns list so the existing symbol filtering logic continues to
work unchanged.
In `@conda/recipes/libcuopt/recipe.yaml`:
- Around line 108-110: The symbol-check step in the libcuopt build recipe is
missing the toolchain dependency needed by ci/check_symbols.sh, so the check can
fail in a clean environment. Update the output build environment for the
libcuopt recipe to include binutils alongside cmake and stdlib("c"), since
check_symbols.sh relies on readelf and c++filt. Make this change in the recipe
section that defines the libcuopt output build requirements so the symbol
verification step always has the required tools available.
In `@cpp/CMakeLists.txt`:
- Around line 601-638: `cuopt_static` is used outside the `if(BUILD_TESTS)`
block even though it is only created there. Move the
`target_link_libraries(cuopt_static PRIVATE $<TARGET_FILE:PSLP>)` and
`add_dependencies(cuopt_static PSLP)` calls inside the same `BUILD_TESTS` guard
(or otherwise guard them on target existence) so CMake does not reference a
non-existent target when tests are disabled.
In `@cpp/include/cuopt/grpc/grpc_client_env.hpp`:
- Around line 10-11: Add the missing cuopt export header to make
grpc_client_env.hpp self-contained, since the CUOPT_EXPORT symbol is used in the
namespace declaration but is only brought in transitively today. Update the
includes in grpc_client_env.hpp so it directly includes cuopt/export.hpp
alongside grpc_client.hpp, ensuring the header compiles on its own regardless of
include order.
---
Nitpick comments:
In `@cpp/include/cuopt/routing/routing_structures.hpp`:
- Around line 18-19: The exported routing namespace is exposing internal detail
helpers through the public ABI. Update the namespace declaration around
cuopt::routing in routing_structures.hpp so that detail stays hidden, either by
removing the export from the enclosing routing namespace or by placing detail
outside the exported namespace; use the cuopt::routing and
cuopt::routing::detail symbols to keep internal-only helpers out of the exported
surface.
🪄 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: 14cd8098-3d2d-49e3-be87-1acfca2543f1
📒 Files selected for processing (70)
ci/check_symbols.shconda/recipes/libcuopt/recipe.yamlcpp/CMakeLists.txtcpp/include/cuopt/error.hppcpp/include/cuopt/export.hppcpp/include/cuopt/grpc/cython_grpc_client.hppcpp/include/cuopt/grpc/grpc_client_env.hppcpp/include/cuopt/mathematical_optimization/backend_selection.hppcpp/include/cuopt/mathematical_optimization/cpu_optimization_problem.hppcpp/include/cuopt/mathematical_optimization/cpu_optimization_problem_solution.hppcpp/include/cuopt/mathematical_optimization/cpu_pdlp_warm_start_data.hppcpp/include/cuopt/mathematical_optimization/cuopt_c.hcpp/include/cuopt/mathematical_optimization/io/data_model_view.hppcpp/include/cuopt/mathematical_optimization/io/mps_data_model.hppcpp/include/cuopt/mathematical_optimization/io/mps_writer.hppcpp/include/cuopt/mathematical_optimization/io/parser.hppcpp/include/cuopt/mathematical_optimization/io/utilities/cython_parser.hppcpp/include/cuopt/mathematical_optimization/io/writer.hppcpp/include/cuopt/mathematical_optimization/mip/solver_settings.hppcpp/include/cuopt/mathematical_optimization/mip/solver_solution.hppcpp/include/cuopt/mathematical_optimization/optimization_problem.hppcpp/include/cuopt/mathematical_optimization/optimization_problem_solution.hppcpp/include/cuopt/mathematical_optimization/pdlp/pdlp_warm_start_data.hppcpp/include/cuopt/mathematical_optimization/pdlp/solver_settings.hppcpp/include/cuopt/mathematical_optimization/pdlp/solver_solution.hppcpp/include/cuopt/mathematical_optimization/solve.hppcpp/include/cuopt/mathematical_optimization/solve_remote.hppcpp/include/cuopt/mathematical_optimization/solver_settings.hppcpp/include/cuopt/mathematical_optimization/utilities/cython_solve.hppcpp/include/cuopt/mathematical_optimization/utilities/cython_types.hppcpp/include/cuopt/routing/assignment.hppcpp/include/cuopt/routing/cython/cython.hppcpp/include/cuopt/routing/data_model_view.hppcpp/include/cuopt/routing/routing_structures.hppcpp/include/cuopt/routing/solve.hppcpp/include/cuopt/routing/solver_settings.hppcpp/src/grpc/client/solve_remote.cppcpp/src/grpc/grpc_problem_mapper.cppcpp/src/grpc/grpc_settings_mapper.cppcpp/src/grpc/grpc_solution_mapper.cppcpp/src/io/data_model_view.cppcpp/src/io/lp_parser.cppcpp/src/io/mps_data_model.cppcpp/src/io/mps_writer.cppcpp/src/io/parser.cppcpp/src/io/writer.cppcpp/src/math_optimization/solution_reader.hppcpp/src/math_optimization/solver_settings.cucpp/src/mip_heuristics/solve.cucpp/src/mip_heuristics/solver_settings.cucpp/src/mip_heuristics/solver_solution.cucpp/src/pdlp/cpu_optimization_problem.cppcpp/src/pdlp/cpu_pdlp_warm_start_data.cucpp/src/pdlp/optimization_problem.cucpp/src/pdlp/pdlp_warm_start_data.cucpp/src/pdlp/solution_conversion.cucpp/src/pdlp/solve.cucpp/src/pdlp/solver_settings.cucpp/src/pdlp/solver_solution.cucpp/src/routing/assignment.cucpp/src/routing/data_model_view.cucpp/src/routing/distance_engine/waypoint_matrix.cppcpp/src/routing/solve.cucpp/src/routing/solver_settings.cucpp/src/routing/utilities/cython.cucpp/src/utilities/logger.hppcpp/tests/CMakeLists.txtcpp/tests/examples/routing/CMakeLists.txtcpp/tests/linear_programming/CMakeLists.txtcpp/tests/linear_programming/grpc/CMakeLists.txt
arhag23
force-pushed
the
fix-symbol-visibility-2
branch
from
bb9a313 to
96f1647
Compare
CI Test Summary✅ All 31 test job(s) passed. |
arhag23
force-pushed
the
fix-symbol-visibility-2
branch
from
96f1647 to
cdfa680
Compare
Signed-off-by: Arha Gatram <agatram@nvidia.com>
…static archive Signed-off-by: Arha Gatram <agatram@nvidia.com>
Signed-off-by: Arha Gatram <agatram@nvidia.com>
Signed-off-by: Arha Gatram <agatram@nvidia.com>
arhag23
force-pushed
the
fix-symbol-visibility-2
branch
from
cdfa680 to
9a23ba5
Compare
| template optimization_problem_solution_t<int, F_TYPE> solve_lp_with_method( \ | ||
| mip::problem_t<int, F_TYPE>& problem, \ | ||
| pdlp_solver_settings_t<int, F_TYPE> const& settings, \ | ||
| const timer_t& timer, \ | ||
| bool is_batch_mode); \ | ||
| \ | ||
| template optimization_problem_solution_t<int, F_TYPE> batch_pdlp_solve( \ | ||
| raft::handle_t const* handle_ptr, \ | ||
| const cuopt::mathematical_optimization::io::mps_data_model_t<int, F_TYPE>& mps_data_model, \ | ||
| const std::vector<int>& fractional, \ | ||
| const std::vector<F_TYPE>& root_soln_x, \ | ||
| pdlp_solver_settings_t<int, F_TYPE> const& settings); \ | ||
| \ | ||
| template optimization_problem_solution_t<int, F_TYPE> run_batch_pdlp( \ | ||
| optimization_problem_t<int, F_TYPE>& problem, \ | ||
| pdlp_solver_settings_t<int, F_TYPE> const& settings); \ | ||
| \ | ||
| template size_t compute_optimal_batch_size(const optimization_problem_t<int, F_TYPE>& problem, \ | ||
| bool per_climber_objectives, \ | ||
| bool per_climber_constraint_bounds, \ | ||
| bool collect_solutions); \ | ||
| \ | ||
| template optimization_problem_t<int, F_TYPE> mps_data_model_to_optimization_problem( \ | ||
| raft::handle_t const* handle_ptr, \ | ||
| const cuopt::mathematical_optimization::io::mps_data_model_t<int, F_TYPE>& data_model); \ | ||
| template void set_pdlp_solver_mode(pdlp_solver_settings_t<int, F_TYPE>& settings); |
There was a problem hiding this comment.
We should probably export all of these as well. @Kh4ster to check
There was a problem hiding this comment.
I checked these against the public include file, batch_pdlp_solve and mps_data_model_to_optimization_problem are present in <cuopt/mathematical_optimization/solve.hpp> so I will export them also. The rest aren't in any public header file.
| "CUOPT_LOG_ACTIVE_LEVEL=RAPIDS_LOGGER_LOG_LEVEL_${LIBCUOPT_LOGGING_LEVEL}" | ||
| CUSPARSE_ENABLE_EXPERIMENTAL_API | ||
| ) | ||
|
|
There was a problem hiding this comment.
@ramakrishnap-nv should we add a target_link_options(cuopt PRIVATE "LINKER:--exclude-libs,ALL")? Otherwise PSLP symbols are embedded with default visibility afaik
There was a problem hiding this comment.
I went with this, but is there any reason for using ALL rather than explicitly specifying PSLP (and if any other PRIVATE static libraries)?
| target_link_libraries(GRPC_CLIENT_TEST | ||
| PRIVATE | ||
| cuopt | ||
| cuopt_static |
There was a problem hiding this comment.
Any reason to change this to static?
There was a problem hiding this comment.
Yes, nearly all tests now link against the static library since almost all directly use internal functions that are hidden when linking against the shared library.
| "CUOPT_LOG_ACTIVE_LEVEL=RAPIDS_LOGGER_LOG_LEVEL_${LIBCUOPT_LOGGING_LEVEL}" | ||
| CUSPARSE_ENABLE_EXPERIMENTAL_API | ||
| ) | ||
|
|
…late instantiations
|
@ramakrishnap-nv We now build a static version of cuopt to link the tests against since they directly reference internal symbols not in the public header API. This grealy increases the size of the test binaries (~100 MB -> ~10 GB) since this embeds the kernel device code in the tests. Is this acceptable or do we want to look at alternatives? We can try exporting every internal symbol used for the tests, but this may lead to too much of the internals being visible. |
Yeah, that was my main concern that the size of the tests would increase significantly. Are we doing same across all other RAPIDS projects? And these are normally uploaded to nightly registries, so that is another concern. |
AFAIK other RAPIDS projects didn't need such a change for symbol visibility controls. |
Hmm, in the issue @bdice mentioned that this has been done for cudf, cuml and others, so what is the extent. If possible, I would like to avoid static linking with test. |
I think those libraries didn't have tests against the library internals so they never ran into this issue. To avoid static linking with the tests, we have to either export all the internals tested against or disable visibility controls when building for the tests. Unfortunately none of these options seem like the "correct" thing to do. |
3 participants
Description
Updates CMake code to build with hidden visibility. Adds explicit exports for symbols that should be visible in the shared library. Adds a separate static library for use with the tests which require access to the hidden symbols. Adds a CI check for ensuring no symbols from dependencies and implementation details are leaking.
Issue
Closes #1213
Builds off #1215 which can be closed after this is merged
Checklist