Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use hidden visibility for symbols for non-windows platforms #341

Merged
merged 6 commits into from
Jul 30, 2024

Conversation

svatasoiu
Copy link
Collaborator

this is to avoid polluting our .so's with visible symbols that might clash with other libraries when CSP is imported. default is basically public for gcc+clang, so this PR changes it to hidden (Windows does the opposite, the default is essentially publicly visible in the DLL symbol table)

reference: https://gcc.gnu.org/wiki/Visibility

@timkpaine timkpaine added lang: c++ Issues and PRs related to the C++ codebase type: enhancement Issues and PRs related to improvements to existing features labels Jul 17, 2024
@svatasoiu
Copy link
Collaborator Author

cannot reproduce the mac os build failures locally. I was able to successfully compile+run all tests on Python3.12 linux w/ clang 18.1.8, as well as clang 16.0.6 on mac os 13; but GH seems to be failing on clang 16.0.6 + OSX 12+14 builds with a mysterious error:

make: *** [test-py] Abort trap: 6

@timkpaine timkpaine marked this pull request as draft July 21, 2024 14:24
@svatasoiu svatasoiu force-pushed the hidden-visibility-non-windows branch 2 times, most recently from eec76ac to 60e55c4 Compare July 26, 2024 22:41
@svatasoiu svatasoiu force-pushed the hidden-visibility-non-windows branch from 60e55c4 to 91b2516 Compare July 30, 2024 17:57
@svatasoiu svatasoiu marked this pull request as ready for review July 30, 2024 18:48
@svatasoiu
Copy link
Collaborator Author

Before:

$ nm -A -C -D --defined-only ./csp/lib/*.so | egrep -v "typeinfo|vtable|std::"  | awk -F':' '{print $1}' | uniq -c | sort -n
      8 ./csp/lib/_cspbasketlibimpl.so
      8 ./csp/lib/_csptestlibimpl.so
     44 ./csp/lib/_cspstatsimpl.so
     52 ./csp/lib/_cspnpstatsimpl.so
     73 ./csp/lib/_cspmathimpl.so
    168 ./csp/lib/_kafkaadapterimpl.so
    237 ./csp/lib/_cspbaselibimpl.so
    456 ./csp/lib/_cspimpl.so
    476 ./csp/lib/_csptypesimpl.so
    557 ./csp/lib/_parquetadapterimpl.so
    595 ./csp/lib/_websocketadapterimpl.so

After:

$ nm -A -C -D --defined-only ./csp/lib/*.so | egrep -v "typeinfo|vtable|std::"  | awk -F':' '{print $1}' | uniq -c | sort -n
      3 ./csp/lib/_cspbaselibimpl.so
      3 ./csp/lib/_cspbasketlibimpl.so
      3 ./csp/lib/_cspmathimpl.so
      3 ./csp/lib/_cspnpstatsimpl.so
      3 ./csp/lib/_cspstatsimpl.so
      3 ./csp/lib/_csptestlibimpl.so
     25 ./csp/lib/_cspimpl.so
     29 ./csp/lib/_kafkaadapterimpl.so
     33 ./csp/lib/_csptypesimpl.so
     40 ./csp/lib/_parquetadapterimpl.so
     86 ./csp/lib/_websocketadapterimpl.so

cpp/csp/python/InitHelper.h Outdated Show resolved Hide resolved
@robambalu
Copy link
Collaborator

Pls just make sure to squash commits when merging

@robambalu robambalu self-requested a review July 30, 2024 21:11
@svatasoiu svatasoiu merged commit 4153d01 into main Jul 30, 2024
29 checks passed
@svatasoiu svatasoiu deleted the hidden-visibility-non-windows branch July 30, 2024 21:19
wrieg123 added a commit that referenced this pull request Sep 29, 2024
AdamGlustein added a commit that referenced this pull request Nov 13, 2024
AdamGlustein added a commit that referenced this pull request Nov 13, 2024
* Revert "Use hidden visibility for symbols for non-windows platforms (#341)"

This reverts commit 4153d01.

Signed-off-by: Adam Glustein <[email protected]>

* Remove CSP_PUBLIC from autogen build

Signed-off-by: Adam Glustein <[email protected]>

---------

Signed-off-by: Adam Glustein <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang: c++ Issues and PRs related to the C++ codebase type: enhancement Issues and PRs related to improvements to existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants