Skip to content

Commit

Permalink
Hide visibility of non-public symbols (#1644)
Browse files Browse the repository at this point in the history
Fixes #1645

Contributes to rapidsai/build-planning#33

Similar to rapidsai/cudf#15982

Proposes more tightly controlling the visibility of symbols in the shared libraries produces for the `rmm` Python library, via the following:

* compiling with `-fvisibility=hidden` by default
* marking intended-to-be-public parts of `rmm` *(everything in the `rmm::` namespace)* with `__attribute__((visibility("default")))`

## Benefits of this change

Reduces the risk of symbol conflicts when `rmm` is used alongside other libraries. For example, see this case in `cudf` where the `spdlog::` symbols in `rmm` are conflicting with the `spdlog::` symbols in `nvcomp`: rapidsai/cudf#15483 (comment)

Reduces library size by a bit (around 0.3 MB uncompressed), by reducing the size of symbol tables in DSOs.

## Notes for Reviewers

This is at the very edge of my C++ knowledge, apologies in advance if I've missed something obvious 😬 

#

Authors:
  - James Lamb (https://github.com/jameslamb)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Kyle Edwards (https://github.com/KyleFromNVIDIA)
  - Mark Harris (https://github.com/harrism)
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: #1644
  • Loading branch information
jameslamb committed Aug 15, 2024
1 parent 975c911 commit 43d01db
Show file tree
Hide file tree
Showing 4 changed files with 103 additions and 3 deletions.
6 changes: 5 additions & 1 deletion ci/build_wheel_python.sh
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ source rapids-date-string

rapids-generate-version > ./VERSION

cd "${package_dir}"
pushd "${package_dir}"

RAPIDS_PY_CUDA_SUFFIX="$(rapids-wheel-ctk-name-gen ${RAPIDS_CUDA_VERSION})"
CPP_WHEELHOUSE=$(RAPIDS_PY_WHEEL_NAME="rmm_${RAPIDS_PY_CUDA_SUFFIX}" rapids-download-wheels-from-s3 cpp /tmp/librmm_dist)
Expand All @@ -29,3 +29,7 @@ mkdir -p final_dist
python -m auditwheel repair -w final_dist dist/*

RAPIDS_PY_WHEEL_NAME="${package_name}_${RAPIDS_PY_CUDA_SUFFIX}" rapids-upload-wheels-to-s3 python final_dist

# switch back to the root of the repo and check symbol visibility
popd
ci/check_symbols.sh "$(echo ${package_dir}/final_dist/rmm_*.whl)"
82 changes: 82 additions & 0 deletions ci/check_symbols.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
#!/bin/bash
# Copyright (c) 2024, NVIDIA CORPORATION.

set -eEuo pipefail

echo "checking for symbol visibility issues"

WHEEL_FILE=${1}

raise-symbols-found-error() {
local pattern="${1}"

err_msg="ERROR: Found some exported symbols matching the pattern '${pattern}'.
These should be marked with 'hidden' visibility.
See https://cmake.org/cmake/help/latest/prop_tgt/LANG_VISIBILITY_PRESET.html and https://gcc.gnu.org/wiki/Visibility for details.
"

echo ""
echo "${err_msg}"
exit 1
}

WHEEL_EXPORT_DIR="$(mktemp -d)"

unzip \
-d "${WHEEL_EXPORT_DIR}" \
"${WHEEL_FILE}"

dso_files=$(
find \
"${WHEEL_EXPORT_DIR}" \
-type f \
\( -name '*.so' -o -name '*.so.*' \)
)

for dso_file in ${dso_files}; do
echo ""
echo "checking exported symbols in '${dso_file}'"
symbol_file="./syms.txt"
readelf --symbols --wide "${dso_file}" \
| c++filt \
> "${symbol_file}"

echo "symbol counts by type"
echo " * GLOBAL: $(grep --count -E ' GLOBAL ' < ${symbol_file})"
echo " * WEAK: $(grep --count -E ' WEAK ' < ${symbol_file})"
echo " * LOCAL: $(grep --count -E ' LOCAL ' < ${symbol_file})"

# Explanation for '-v' uses here:
#
# * 'format_error' symbols are intentionally exported, that type of error
# can be thrown across library boundaries. See "Problems with C++ exceptions"
# at https://gcc.gnu.org/wiki/Visibility.
echo "checking for 'fmt::' symbols..."
if grep -E 'fmt\:\:' < "${symbol_file}" \
| grep -v 'format_error'
then
raise-symbols-found-error 'fmt::'
fi

# Explanation for '-v' uses here:
#
# * trivially-destructible objects sometimes get an entry in the symbol table
# for a specialization of `std::_Destroy_aux()` called to destroy them.
# There is one for `spdlog::details::log_msg_buffer like that:
#
# 'std::_Destroy_aux<false>::__destroy<spdlog::details::log_msg_buffer*>'
#
# That should be safe to export.
#
echo "checking for 'spdlog::' symbols..."
if grep -E 'spdlog\:\:' < "${symbol_file}" \
| grep -v 'std\:\:_Destroy_aux'
then
raise-symbols-found-error 'spdlog::'
fi
echo "No symbol visibility issues found"
done

echo ""
echo "No symbol visibility issues found in any DSOs"
9 changes: 8 additions & 1 deletion python/rmm/rmm/_cuda/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# =============================================================================
# Copyright (c) 2022, NVIDIA CORPORATION.
# Copyright (c) 2022-2024, NVIDIA CORPORATION.
#
# Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
# in compliance with the License. You may obtain a copy of the License at
Expand All @@ -17,3 +17,10 @@ set(linked_libraries rmm::rmm)

rapids_cython_create_modules(SOURCE_FILES "${cython_sources}" LINKED_LIBRARIES "${linked_libraries}"
CXX)

# mark all symbols in these Cython targets "hidden" by default, so they won't collide with symbols
# loaded from other DSOs
foreach(_cython_target IN LISTS RAPIDS_CYTHON_CREATED_TARGETS)
set_target_properties(${_cython_target} PROPERTIES C_VISIBILITY_PRESET hidden
CXX_VISIBILITY_PRESET hidden)
endforeach()
9 changes: 8 additions & 1 deletion python/rmm/rmm/_lib/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# =============================================================================
# Copyright (c) 2022, NVIDIA CORPORATION.
# Copyright (c) 2022-2024, NVIDIA CORPORATION.
#
# Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
# in compliance with the License. You may obtain a copy of the License at
Expand All @@ -19,6 +19,13 @@ set(linked_libraries rmm::rmm)
rapids_cython_create_modules(SOURCE_FILES "${cython_sources}" LINKED_LIBRARIES "${linked_libraries}"
CXX)

# mark all symbols in these Cython targets "hidden" by default, so they won't collide with symbols
# loaded from other DSOs
foreach(_cython_target IN LISTS RAPIDS_CYTHON_CREATED_TARGETS)
set_target_properties(${_cython_target} PROPERTIES C_VISIBILITY_PRESET hidden
CXX_VISIBILITY_PRESET hidden)
endforeach()

add_library(_torch_allocator SHARED _torch_allocator.cpp)
# Want the output to be called _torch_allocator.so
set_target_properties(_torch_allocator PROPERTIES PREFIX "" SUFFIX ".so")
Expand Down

0 comments on commit 43d01db

Please sign in to comment.