rapidsai / cuml

cuML - RAPIDS Machine Learning Library
https://docs.rapids.ai/api/cuml/stable/
Apache License 2.0
4.16k stars 525 forks source link

[FEA] Option for hiding CXX symbols. #3859

Open trivialfis opened 3 years ago

trivialfis commented 3 years ago

CXX symbol clash can happen between libraries, which may lead to weird errors that are difficult to reproduce and fix. One time I ran into an cudaErrorInvalidValue error caused by it in XGBoost. This kind of error corrupts the program and is impossible to debug if you don't know about symbol clash. I'm suspecting that I might be running into it in cuml in one of my environments, even if I'm wrong, it would still be great to eliminate the possibility. For CMake projects, this can be done by:

set_target_properties(${target} PROPERTIES CXX_VISIBILITY_PRESET hidden)

I'm not sure what steps are needed for cython code. Opening an issue for discussion. For details on symbol visibility, please see: https://gcc.gnu.org/wiki/Visibility .

dantegd commented 3 years ago

@trivialfis for cython, it can be added to compiler options either file based or in the extension definition if I'm not mistaken (i.e. here https://github.com/rapidsai/cuml/blob/ef13080fa686bac6504b77062f5fb2e0727bfe7f/python/setup.py#L193).

That said, this is something we can look into after #3844 is merged. Not entirely sure immediately, but the first option that comes to my mind is to add the option in our cmake itself so that when an issue like this rises, you could build cuml with hidden visibility to see if indeed the issue is a hidden symbol crash.

BTW, sorry I missed your question in the above PR itself, but it should be pretty easy to add in here https://github.com/dantegd/cuml/blob/7450727a17d40278ef8e8fea3af4e33c39e17d2b/cpp/CMakeLists.txt#L281 probably conditionally

mdemoret-nv commented 3 years ago

@dantegd I looked at the linked documentation and it doesn't mention any downsides to hiding CXX symbols. Do you know any downsides to enabling this by default for cuML?

dantegd commented 3 years ago

@mdemoret-nv nope, I came to the same conclusion as you while reading, but would like to have an exclusive PR (that only touches this) and corresponding testing to be sure that there are no unexpected side effects.

trivialfis commented 3 years ago

For downsides:

trivialfis commented 3 years ago

I will try looking into it once @dantegd 's PR is merged

mdemoret-nv commented 3 years ago

@dantegd Lets revisit this once your PR is merged. The changes will be very small and easy to implement. Seems like the best course of action is to just try and see what happens in build/test.

trivialfis commented 3 years ago

Hi, @dantegd my plan is to add this as a CMake option that we can turn on when needed. I think the change won't be trivial as we are exporting a C++ interface, once the symbols are hidden, neither cython nor the tests/benchmarks can link libcuml++.so.

github-actions[bot] commented 2 years ago

This issue has been labeled inactive-90d due to no recent activity in the past 90 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed.