Hide more symbols in ELF shared libs #447
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This looks okay. I think it is fine to just export the clingo related C symbols. I am just not sure if it is necessary to enable this in general. The advantage seems to be limited to your specific case where there are other static libraries contained within libclingo. Not enabling by default might prevent breakage (even though I don't see a reason) elsewhere. 😄 |
|
I sort of agree, but I decided to open the PR anyways because it's cleaner in general, given that even when doing shared linking against libstdc++, still around ~110 C++ symbols end up as public in the library: List of exported C++ symbols |
|
Dug into the pros and cons a bit further: to be able to catch exceptions in a different binary from If libclingo.so is supposed to be basically a C library, I guess that's not an issue. |
All exceptions are caught internally. Anything else is a bug. |
|
We can just give it a try. If there are issues on some platform, it is still possible to switch to a more conservative change. |
|
I (again) missed that this PR was against the master. All PRs have to go to the wip branch first. Can you please reopen? |
|
See #449, perhaps set the default branch to |
I wanted the latest stable code to be shown but PR against the wip. |
tgamblin
added a commit
to spack/spack
that referenced
this pull request
Feb 15, 2024
5.7.0 was just released. It includes a number of changes requested and/or upstreamed by Spack developers, e.g.: * API for accessing optimization priorities: potassco/clingo#406 * Hash optimization: potassco/clingo#441 * Contributing Guide: potassco/clingo#465 * Hiding more ELF symbols: * potassco/clingo#447 * potassco/clingo#449
alalazo
pushed a commit
to spack/spack
that referenced
this pull request
Feb 16, 2024
5.7.0 was just released. It includes a number of changes requested and/or upstreamed by Spack developers, e.g.: * API for accessing optimization priorities: potassco/clingo#406 * Hash optimization: potassco/clingo#441 * Contributing Guide: potassco/clingo#465 * Hiding more ELF symbols: * potassco/clingo#447 * potassco/clingo#449
mathomp4
pushed a commit
to mathomp4/spack
that referenced
this pull request
Mar 27, 2024
5.7.0 was just released. It includes a number of changes requested and/or upstreamed by Spack developers, e.g.: * API for accessing optimization priorities: potassco/clingo#406 * Hash optimization: potassco/clingo#441 * Contributing Guide: potassco/clingo#465 * Hiding more ELF symbols: * potassco/clingo#447 * potassco/clingo#449
teaguesterling
pushed a commit
to teaguesterling/spack
that referenced
this pull request
Jun 15, 2024
5.7.0 was just released. It includes a number of changes requested and/or upstreamed by Spack developers, e.g.: * API for accessing optimization priorities: potassco/clingo#406 * Hash optimization: potassco/clingo#441 * Contributing Guide: potassco/clingo#465 * Hiding more ELF symbols: * potassco/clingo#447 * potassco/clingo#449
kwryankrattiger
pushed a commit
to kwryankrattiger/spack
that referenced
this pull request
Jul 9, 2024
5.7.0 was just released. It includes a number of changes requested and/or upstreamed by Spack developers, e.g.: * API for accessing optimization priorities: potassco/clingo#406 * Hash optimization: potassco/clingo#441 * Contributing Guide: potassco/clingo#465 * Hiding more ELF symbols: * potassco/clingo#447 * potassco/clingo#449
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In Spack we'd like to distribute optimized
libclingo.so, to be dlopened byPython. In practice two optimizations make a big difference: (a) profile-guided
optimization and (b) use of mimalloc as an allocator.
To make (b) work, we either have to use a custom allocator in the code base,
which is a lot of work, or override
operator newand friends locally withmimalloc's version. The latter is pretty easy, but we have to deal with some
load order issues when using libclingo from Python: if another module gets
imported and pulls in libstdc++, libclingo would be using libstdc++'s
operatornewinstead of mimalloc's.So, the current solution is to put mimalloc's
operator newsymbol inside oflibclingo.so, statically link libstdc++ (doesn't error about duplicate
symbols), and make all non-clingo C++ symbols local to the library.
The latter requires a version script, cause
-fvisibility=hiddenaffectscompilation, but not symbols of statically linked libraries. Even if we use the
mimalloc implementation header in a clingo source file,
-fvisibility=hiddendoesn't help, cause mimalloc marks
operator newas visible.Long story short: this PR adds a linker script and ensures that only
clingo_*,gringo_*andg_clingo_*are global, to also hide symbolsthat aren't coming from clingo sources.
Global symbols