Enforce explicit symbol visibility for shared libraries - #5638
Open
jmarrec wants to merge 1 commit into
Open
Conversation
This commit introduces explicit symbol visibility control for shared libraries (DLLs/SOs) compiled with GCC or Clang on Unix-like systems.
It aligns with what we do already on MSVC.
Key changes:
* **CMake Configuration**:
* `CMAKE_POSITION_INDEPENDENT_CODE ON` is explicitly set, effectively handling Position Independent Code (PIC) and allowing removal of the redundant `-fPIC` flag from `CMAKE_C_FLAGS` and `CMAKE_CXX_FLAGS`.
* `CMAKE_CXX_VISIBILITY_PRESET hidden` and `CMAKE_VISIBILITY_INLINES_HIDDEN ON` are set to hide symbols by default, preventing internal symbols from being exported implicitly.
* **API Export Macros**:
* Existing `*_API` macros (e.g., `CSHARP_API`, `RUBY_API`, `UTILITIES_API`) and a new `OPENSTUDIO_ENUM_CLASS_API` macro are updated to explicitly use `__attribute__((visibility("default")))` when building the library. This marks only the intended public symbols for export.
This approach ensures that only symbols explicitly marked with these macros are exported from shared libraries, leading to several benefits:
* **Reduced Binary Size**: Fewer symbols in the export table result in smaller binaries (though in this case It didn't change much)
* **Improved Load Times**: Smaller export tables can lead to faster library loading.
* **Clearer API Surface**: Prevents unintended symbols from being exposed, making the public API more explicit and stable.
* **Reduced Symbol Conflicts**: Minimizes the risk of symbol clashes with other libraries loaded into the same process.
This is a crucial step towards better library hygiene and build performance, aligning with modern C++ shared library best practices..
-----
Results on mac, develop vs visibility hidden
## Results: `develop` vs `visibility_hidden`
**File sizes** (`du -sh Products/*`) — only test binaries and the two engine libs shifted by ~1% or less, nothing dramatic:
| Artifact | develop | visibility_hidden | Δ |
| ---------------------------------------------------------- | ------- | ----------------- | ------------------------------------------------ |
| `libopenstudiolib.dylib` | 90M | 90M | ~unchanged (rounds equal) |
| `librubyengine.so` | 147M | 146M | ↓ ~1M |
| `libpythonengine.so` | 204K | 204K | unchanged |
| all other entries (tests, `openstudio` CLI, `ruby/`, etc.) | — | — | ↓ 4K–1M each, noise from rebuild, not visibility |
**Exported symbol counts** (`nm -gU`, global/defined symbols) — this is where the real effect shows:
| Library | develop | visibility_hidden | Δ |
| ------------------------ | ------- | ----------------- | ------------------- |
| `libopenstudiolib.dylib` | 79,914 | 74,643 | **−5,271 (−6.6%)** |
| `librubyengine.so` | 17,816 | 11,209 | **−6,607 (−37.1%)** |
| `libpythonengine.so` | 25 | 1 | **−24 (−96%)** |
**Takeaway**: `-fvisibility=hidden` is working as intended — it substantially cuts the exported symbol table (especially for the Ruby/Python engine bindings, which were leaking almost everything before). But that symbol-table reduction barely moves file size, because symbol names/exports are a small fraction of a release binary's bytes (most of the size is code/data sections, debug-adjacent metadata, etc.). So: real ABI-surface improvement, negligible disk-size improvement.
jmarrec
added
Developer Issue
Pull Request - Ready for CI
Contributor
🧪 Test Results DashboardSummary
|
| Run | XML File | Status |
|---|---|---|
| run1 | results.xml |
✅ Found |
| run3 | results.xml |
✅ Found |
| run2 | results.xml |
✅ Found |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Pull request overview
This introduces explicit symbol visibility control for shared libraries (DLLs/SOs) compiled with GCC or Clang on Unix-like systems.
It aligns with what we do already on MSVC.
Key changes:
CMAKE_POSITION_INDEPENDENT_CODE ONis explicitly set, effectively handling Position Independent Code (PIC) and allowing removal of the redundant-fPICflag fromCMAKE_C_FLAGSandCMAKE_CXX_FLAGS.CMAKE_CXX_VISIBILITY_PRESET hiddenandCMAKE_VISIBILITY_INLINES_HIDDEN ONare set to hide symbols by default, preventing internal symbols from being exported implicitly.*_APImacros (e.g.,CSHARP_API,RUBY_API,UTILITIES_API) and a newOPENSTUDIO_ENUM_CLASS_APImacro are updated to explicitly use__attribute__((visibility("default")))when building the library. This marks only the intended public symbols for export.This approach ensures that only symbols explicitly marked with these macros are exported from shared libraries, leading to several benefits:
This is a crucial step towards better library hygiene and build performance, aligning with modern C++ shared library best practices.
Another thing is that we should be able to stop exporting openstudio::model namespace on os-4.0-dev
Results on mac, develop vs visibility hidden
Results:
developvsvisibility_hiddenFile sizes (
du -sh Products/*) — only test binaries and the two engine libs shifted by ~1% or less, nothing dramatic:libopenstudiolib.dyliblibrubyengine.solibpythonengine.soopenstudioCLI,ruby/, etc.)Exported symbol counts (
nm -gU, global/defined symbols) — this is where the real effect shows:libopenstudiolib.dyliblibrubyengine.solibpythonengine.soTakeaway:
-fvisibility=hiddenis working as intended — it substantially cuts the exported symbol table (especially for the Ruby/Python engine bindings, which were leaking almost everything before). But that symbol-table reduction barely moves file size, because symbol names/exports are a small fraction of a release binary's bytes (most of the size is code/data sections, debug-adjacent metadata, etc.). So: real ABI-surface improvement, negligible disk-size improvement.Pull Request Author
src/model/test)src/energyplus/Test)src/osversion/VersionTranslator.cpp)Labels:
IDDChangeAPIChangePull Request - Ready for CIso that CI builds your PRReview Checklist
This will not be exhaustively relevant to every PR.