Closed
Conversation
Create PhlexSymbolVisibility.cmake with phlex_apply_symbol_visibility()
that generates export headers into centralized ${PROJECT_BINARY_DIR}/include,
sets CXX_VISIBILITY_PRESET hidden + VISIBILITY_INLINES_HIDDEN ON, and
installs the headers for downstream consumers.
Apply phlex_apply_symbol_visibility() to all 5 shared libraries:
phlex_utilities, phlex_model, phlex_core, phlex_configuration_internal,
run_phlex.
Annotate ~45 headers across the whole phlex namespace (including
phlex::experimental) with export macros — every non-template class and
free function with a .cpp implementation.
Also add missing includes flagged by misc-include-cleaner:
- <cstddef> and <compare> to identifier.hpp
- <cstddef> to hashing.hpp
- <string> to load_module.hpp
Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
The Boost.JSON customization point overloads for configuration, product_query, and experimental::identifier were hidden by the visibility policy, causing config_test link failure: undefined reference to phlex::tag_invoke(boost::json::value_to_tag<phlex::configuration>...) phlex::tag_invoke(boost::json::value_to_tag<phlex::product_query>...) These are public API symbols (they back value_to<T>() for JSON deserialization) that were missed in the initial visibility sweep. Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
Three non-template free functions had .cpp implementations but were missing export macros, causing linker failures in test executables: - phlex_model_EXPORT on friend operator== / operator<=> for identifier_query in identifier.hpp (called from identifier test) - phlex_core_EXPORT on detail::verify_name in glue.hpp (called from template instantiations of glue<T>::provide/observe/transform/...) - phlex_core_EXPORT on port_index_for and more_derived in message.hpp (called from template instantiations in observer_node et al.) Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
…_derived Three more non-template symbols with .cpp implementations were missing export macros, causing linker failures across most test executables: - phlex_model_EXPORT on to_product_specifications() in product_specification.hpp (called from declared_transform.hpp and declared_fold.hpp template instantiations in every test binary) - phlex_core_EXPORT on output_api class in registration_api.hpp (constructor called from glue<T>::output template instantiations) - phlex_model_EXPORT on more_derived(product_store_ptr, product_store_ptr) in product_store.hpp (distinct from the message::more_derived fixed earlier; called directly from product_store test and from template get_most_derived) Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
…parison
Introduce phlex_make_internal_library(target LIBRARIES ...) in
PhlexSymbolVisibility.cmake. The function:
- reads SOURCES from the already-defined public target (no duplication)
- handles generated sources (configure_file output) via src/bin dir fallback
- creates ${target}_internal SHARED using add_library (not cet_make_library)
so cetmodules never registers it for installation or package export
- propagates $<BUILD_INTERFACE:PROJECT_SOURCE_DIR> PUBLIC so consumers
such as layer_generator_internal can resolve phlex/* headers
- propagates COMPILE_DEFINITIONS and COMPILE_OPTIONS from the public target
- omits CXX_VISIBILITY_PRESET hidden so all symbols are visible by default
Applied to all four non-trivial shared libraries:
phlex_utilities, phlex_model, phlex_core, run_phlex
The _internal chain is phlex_core_internal -> phlex_model_internal ->
phlex_utilities_internal so tests linking only to phlex::core_internal
get all internal symbols transitively without loading the public .so files.
layer_generator_internal added in plugins/CMakeLists.txt (same sources,
links PRIVATE phlex_core_internal) to avoid loading both the visibility-hidden
and all-visible phlex_core variants in the same test process (ODR).
All tests in test/CMakeLists.txt and test/utilities/CMakeLists.txt switched to
the _internal variants, except:
- config_test and identifier which need phlex::configuration and thus
transitively pull in the public phlex_core/model/utilities libs; since
the relevant symbols are already exported these tests are fine on the
public libraries
- type_deduction which only requires the header-only phlex::metaprogramming
Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
Generate and install phlex_X_export.hpp files under include/phlex/ instead of flat in include/, matching the layout of all other Phlex installed headers. Update every #include "X_export.hpp" directive in the 42 affected public headers to #include "phlex/X_export.hpp". The target_include_directories entries are unchanged — the base paths (PROJECT_BINARY_DIR/include for build, include for install) already resolve correctly once the phlex/ subdirectory is part of the filename. Co-authored-by: pcanal <5175087+pcanal@users.noreply.github.com>
cetmodules adds INSTALL_INTERFACE:include to every target it creates via
cet_make_library, so the explicit INSTALL_INTERFACE:include entry in
phlex_apply_symbol_visibility was a no-op duplicate. The only unique
contribution of the target_include_directories call is the
BUILD_INTERFACE to ${PROJECT_BINARY_DIR}/include, which is where
generate_export_header writes the generated phlex/X_export.hpp headers.
Co-authored-by: pcanal <5175087+pcanal@users.noreply.github.com>
Introduce option(PHLEX_HIDE_SYMBOLS ... ON) in CMakeLists.txt and wire
it through both functions in PhlexSymbolVisibility.cmake:
phlex_apply_symbol_visibility(target)
ON (default): generate export header + CXX_VISIBILITY_PRESET hidden
OFF: generate export header only; all symbols visible by default
phlex_make_internal_library(target LIBRARIES ...)
ON (default): compile separate SHARED lib with full symbol visibility
OFF: create a thin INTERFACE target wrapping the public lib; no extra
compilation or .so needed since the public lib already exports
everything
In OFF mode _internal aliases and layer_generator_internal still exist
and link correctly via the INTERFACE wrappers, so no other files change.
Benchmark tests (test/benchmarks/, test/max-parallelism/) already link
exclusively to curated public targets in both modes.
Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
When PHLEX_HIDE_SYMBOLS=OFF, layer_generator already exports all symbols, so compiling a separate .so for layer_generator_internal is unnecessary. Use a thin INTERFACE alias instead, matching the behaviour of phlex_make_internal_library() for the core libraries. All plugins used by the benchmark tests (test/benchmarks/, test/max-parallelism/) already link exclusively against curated public targets (phlex::module, phlex::core, layer_generator), not the _internal variants. This is unaffected by PHLEX_HIDE_SYMBOLS. Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
…HLEX_ENABLE_IPO The three optimizations are chosen specifically for the symbol-hiding + external-plugin architecture: 1. -fno-semantic-interposition (GCC >= 9, Clang >= 8; when PHLEX_HIDE_SYMBOLS=ON) Allows the compiler to inline, devirtualise, and generate direct (non-PLT) calls for same-DSO accesses to exported functions. Safe only once the exported-symbol surface is explicitly bounded by export macros — exactly what PHLEX_HIDE_SYMBOLS=ON provides. 2. -fno-plt (GCC >= 7.3, Clang >= 4, ELF/Linux only) Replaces PLT stubs with GOT-based dispatch for calls FROM a Phlex library TO external DSOs (TBB, Boost, spdlog, ...). Removes one level of indirection from every cross-DSO call after load-time resolution. 3. PHLEX_ENABLE_IPO (default OFF) Sets CMAKE_INTERPROCEDURAL_OPTIMIZATION_RELEASE and _RELWITHDEBINFO after confirming compiler support. LTO is safe with symbol hiding because export attributes preserve the complete exported-symbol set; the linker cannot eliminate or rename any default-visibility symbol. External plugins compiled without LTO are unaffected. Intentionally excluded: -ffast-math/-funsafe-math-optimizations, which can silently break floating-point correctness in physics/numerical code. PhlexOptimization.cmake is included from the root CMakeLists.txt. phlex_apply_optimizations() is called after phlex_apply_symbol_visibility() in each of the five shared library CMakeLists.txt files. Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
cmake -DCMAKE_BUILD_TYPE=Release now automatically opts into LTO without requiring a separate -DPHLEX_ENABLE_IPO=ON. The default is derived from CMAKE_BUILD_TYPE at first configure time: Release / RelWithDebInfo → PHLEX_ENABLE_IPO defaults to ON Debug / Coverage / sanitizer builds → PHLEX_ENABLE_IPO defaults to OFF Multi-config generators (no CMAKE_BUILD_TYPE) → defaults to OFF The option can always be overridden explicitly with -DPHLEX_ENABLE_IPO=ON|OFF. The compile-level flags (-fno-semantic-interposition, -fno-plt) are unchanged: they are applied unconditionally (subject to compiler/platform support) regardless of build type, complementing CMake's per-config CMAKE_CXX_FLAGS_RELEASE (-O3 -DNDEBUG) without duplicating it. Documentation in the module header is updated to describe the full build-type interaction. Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
Mirror the PHLEX_ENABLE_IPO pattern: for Release/RelWithDebInfo builds, default PHLEX_HIDE_SYMBOLS=ON (curated API + -fno-semantic-interposition safe); for Debug, Coverage, and sanitizer builds, default OFF so all symbols are visible to debuggers, profilers, and sanitizer stack traces. cmake -DCMAKE_BUILD_TYPE=Release → PHLEX_HIDE_SYMBOLS=ON, PHLEX_ENABLE_IPO=ON cmake -DCMAKE_BUILD_TYPE=Debug → PHLEX_HIDE_SYMBOLS=OFF, PHLEX_ENABLE_IPO=OFF cmake (no CMAKE_BUILD_TYPE) → both OFF (conservative fallback) Both options can still be overridden explicitly at any time. Update PhlexOptimization.cmake header to document the combined interaction of both options with CMAKE_BUILD_TYPE. Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
Move PHLEX_HIDE_SYMBOLS from CMakeLists.txt into PhlexOptimization.cmake so both options are co-located and their defaults are derived together. PHLEX_ENABLE_IPO is now defined first; PHLEX_HIDE_SYMBOLS defaults to ON whenever PHLEX_ENABLE_IPO=ON (in addition to Release/RelWithDebInfo): cmake -DPHLEX_ENABLE_IPO=ON → PHLEX_HIDE_SYMBOLS defaults ON cmake -DCMAKE_BUILD_TYPE=Release → both default ON (unchanged) cmake (Debug / no type) → both default OFF (unchanged) LTO achieves its full potential only with hidden symbols: without -fvisibility=hidden the LTO optimizer must treat every exported symbol as interposable and cannot apply -fno-semantic-interposition. Making IPO automatically pull in symbol hiding as its default ensures the optimal combination is selected whenever LTO is requested. The per-target compile flags in phlex_apply_optimizations() are unchanged: they already self-adjust at build time based on the final option values. The note emitted when a user explicitly overrides PHLEX_HIDE_SYMBOLS=OFF after requesting IPO is retained (now factual rather than advisory). Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
option() only sets the cache default when the variable is not yet present,
so an explicit -DPHLEX_HIDE_SYMBOLS=OFF on the command line bypassed the
defaulting logic and produced the invalid combination in CMakeCache.txt.
Fix: after both options are resolved from the cache, check for the
PHLEX_ENABLE_IPO=ON + PHLEX_HIDE_SYMBOLS=OFF combination and use
set(... FORCE) to correct PHLEX_HIDE_SYMBOLS to ON. A STATUS message
explains the forced correction. CMakeCache.txt will now always show
PHLEX_HIDE_SYMBOLS=ON when PHLEX_ENABLE_IPO=ON.
Before: cmake ... -DPHLEX_HIDE_SYMBOLS=OFF -DCMAKE_BUILD_TYPE=Release
→ CMakeCache.txt: PHLEX_ENABLE_IPO=ON, PHLEX_HIDE_SYMBOLS=OFF ✗
After: cmake ... -DPHLEX_HIDE_SYMBOLS=OFF -DCMAKE_BUILD_TYPE=Release
→ STATUS "PHLEX_HIDE_SYMBOLS forced ON because PHLEX_ENABLE_IPO=ON"
→ CMakeCache.txt: PHLEX_ENABLE_IPO=ON, PHLEX_HIDE_SYMBOLS=ON ✓
Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
…endent Revert the set(FORCE) that prevented PHLEX_ENABLE_IPO=ON + PHLEX_HIDE_SYMBOLS=OFF: this combination is valid and essential for benchmarking the LTO benefit with and without symbol hiding. All four combinations are now fully supported: PHLEX_HIDE_SYMBOLS=ON PHLEX_ENABLE_IPO=ON → LTO + -fno-semantic-interposition (maximum) PHLEX_HIDE_SYMBOLS=OFF PHLEX_ENABLE_IPO=ON → LTO only (benchmark comparison case) PHLEX_HIDE_SYMBOLS=ON PHLEX_ENABLE_IPO=OFF → -fno-semantic-interposition only PHLEX_HIDE_SYMBOLS=OFF PHLEX_ENABLE_IPO=OFF → no special flags (debug/default) Both options now default independently from CMAKE_BUILD_TYPE (ON for Release/RelWithDebInfo, OFF otherwise) with no cross-dependency in the defaulting logic. The per-target flags in phlex_apply_optimizations() already self-adjust correctly to whichever combination is in effect. Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
- May be overridden from CMake command line, environment, etc.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens shared-library symbol visibility and optimization behavior across Phlex, introduces “_internal” companion libraries for tests/benchmarking under hidden-visibility builds, and applies formatting fixes across CMake/Python/YAML.
Changes:
- Add export headers/macros to core/model/utilities/configuration/run_phlex APIs and enable symbol-hiding/optimization controls via new CMake modules.
- Introduce
_internalcompanion libraries and update tests/plugins to link against them to avoid visibility-mismatch mixing. - Apply automated formatting (ruff/clang-format/cmake-format/YAML formatting) across touched files.
Reviewed changes
Copilot reviewed 68 out of 68 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| CMakeLists.txt | Adds default per-build-type flags; includes new symbol-visibility/optimization modules |
| Modules/private/PhlexSymbolVisibility.cmake | Implements export-header generation + _internal library helper |
| Modules/private/PhlexOptimization.cmake | Defines PHLEX_HIDE_SYMBOLS / PHLEX_ENABLE_IPO defaults and applies safe optimization flags |
| phlex/CMakeLists.txt | Applies symbol visibility + optimizations to configuration library |
| phlex/app/CMakeLists.txt | Applies symbol visibility/optimizations; creates run_phlex_internal |
| phlex/app/load_module.hpp | Exports load/adjust entry points for run_phlex |
| phlex/app/run.hpp | Exports run() API |
| phlex/app/version.hpp | Exports version() API |
| phlex/configuration.hpp | Exports configuration + Boost.JSON tag_invoke customization points |
| phlex/concurrency.hpp | Exports concurrency type for core API |
| phlex/core/CMakeLists.txt | Applies symbol visibility/optimizations; creates phlex_core_internal and alias |
| phlex/core/consumer.hpp | Exports consumer base type |
| phlex/core/declared_fold.hpp | Exports declared_fold |
| phlex/core/declared_observer.hpp | Exports declared_observer |
| phlex/core/declared_output.hpp | Exports declared_output |
| phlex/core/declared_predicate.hpp | Exports declared_predicate |
| phlex/core/declared_provider.hpp | Exports declared_provider |
| phlex/core/declared_transform.hpp | Exports declared_transform |
| phlex/core/declared_unfold.hpp | Exports declared_unfold + generator |
| phlex/core/detail/filter_impl.hpp | Exports decision_map/data_map (visibility fixes) |
| phlex/core/detail/make_algorithm_name.hpp | Exports make_algorithm_name helper |
| phlex/core/detail/maybe_predicates.hpp | Exports maybe_predicates helper |
| phlex/core/detail/repeater_node.hpp | Exports repeater_node composite node |
| phlex/core/edge_creation_policy.hpp | Exports edge_creation_policy |
| phlex/core/edge_maker.hpp | Exports edge_maker |
| phlex/core/filter.hpp | Exports filter |
| phlex/core/framework_graph.hpp | Exports framework_graph |
| phlex/core/glue.hpp | Exports glue (template wrapper) and verify_name helper |
| phlex/core/index_router.hpp | Exports index_router and related types |
| phlex/core/input_arguments.hpp | Exports verify_no_duplicate_input_products helper |
| phlex/core/message.hpp | Exports message_matcher/more_derived/port_index_for |
| phlex/core/node_catalog.hpp | Exports node_catalog |
| phlex/core/product_query.hpp | Exports product_query |
| phlex/core/products_consumer.hpp | Exports products_consumer |
| phlex/core/registrar.hpp | Exports add_to_error_messages helper |
| phlex/core/registration_api.hpp | Exports output_api |
| phlex/core/store_counters.hpp | Exports store_counter/count_stores |
| phlex/model/CMakeLists.txt | Applies symbol visibility/optimizations; creates phlex_model_internal and alias |
| phlex/model/algorithm_name.hpp | Exports algorithm_name |
| phlex/model/data_cell_counter.hpp | Exports counter/flush types |
| phlex/model/data_cell_index.hpp | Exports data_cell_index and stream operator |
| phlex/model/data_layer_hierarchy.hpp | Exports data_layer_hierarchy |
| phlex/model/identifier.hpp | Exports identifier + operators/UDLs |
| phlex/model/product_matcher.hpp | Exports product_matcher |
| phlex/model/product_specification.hpp | Exports product_specification and to_product_specifications |
| phlex/model/product_store.hpp | Exports product_store and more_derived |
| phlex/model/products.hpp | Exports product_base and products container |
| phlex/utilities/CMakeLists.txt | Applies symbol visibility/optimizations; creates phlex_utilities_internal and alias |
| phlex/utilities/hashing.hpp | Exports hashing overloads and fixes includes |
| phlex/utilities/resource_usage.hpp | Exports resource_usage |
| plugins/CMakeLists.txt | Adds layer_generator_internal for tests depending on PHLEX_HIDE_SYMBOLS |
| test/CMakeLists.txt | Switches tests to link against internal variants (core/model/utilities/run_phlex/layer_generator) |
| test/utilities/CMakeLists.txt | Switches utilities tests to link against utilities_internal |
| test/python/all_config.py | Ruff formatting (quotes/spacing) |
| test/python/reducer.py | Ruff formatting (call wrapping) |
| test/python/sumit.py | Ruff formatting (call wrapping) |
| test/python/test_coverage.py | Ruff formatting (tuple formatting) |
| test/python/test_mismatch.py | Ruff formatting (call wrapping) |
| test/python/vectypes.py | Ruff formatting (list/dict formatting) |
| .github/workflows/clang-format-fix.yaml | YAML formatting adjustments |
| .github/workflows/clang-tidy-fix.yaml | YAML formatting adjustments |
| .github/workflows/cmake-format-fix.yaml | YAML formatting adjustments |
| .github/workflows/codeql-analysis.yaml | YAML formatting adjustment (needs list) |
| .github/workflows/header-guards-fix.yaml | YAML formatting adjustments |
| .github/workflows/jsonnet-format-fix.yaml | YAML formatting adjustments |
| .github/workflows/markdown-fix.yaml | YAML formatting adjustments |
| .github/workflows/python-fix.yaml | YAML formatting adjustments |
| .github/workflows/yaml-fix.yaml | YAML formatting adjustments |
Comment on lines
75
to
+77
| include(Modules/private/CreateCoverageTargets.cmake) | ||
| include(Modules/private/PhlexSymbolVisibility.cmake) | ||
| include(Modules/private/PhlexOptimization.cmake) |
There was a problem hiding this comment.
PHLEX_HIDE_SYMBOLS / PHLEX_ENABLE_IPO defaults in PhlexOptimization.cmake are derived from CMAKE_BUILD_TYPE at include time, but this file sets a default CMAKE_BUILD_TYPE later (RelWithDebInfo). On a fresh configure with no CMAKE_BUILD_TYPE provided, the options will default as if the build type were unset (OFF/OFF), even though the project then forces RelWithDebInfo. Consider setting CMAKE_BUILD_TYPE earlier (before including PhlexOptimization.cmake) or computing these option defaults after the build-type default is established so they match the effective configuration.
Comment on lines
+17
to
+19
| if(PHLEX_HIDE_SYMBOLS) | ||
| add_library(layer_generator_internal layer_generator.cpp) | ||
| target_link_libraries(layer_generator_internal PRIVATE phlex_core_internal) |
There was a problem hiding this comment.
layer_generator_internal is created without an explicit library type, so it will follow BUILD_SHARED_LIBS. The comment above indicates this is meant to be a separate shared library (.so) to avoid mixing visibility-hidden vs all-visible variants in one process; if BUILD_SHARED_LIBS is OFF, this becomes a static library and the rationale no longer holds. Consider making the intended type explicit (e.g., SHARED) to ensure consistent behavior.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #407 +/- ##
=======================================
Coverage 83.71% 83.71%
=======================================
Files 126 126
Lines 3291 3291
Branches 583 583
=======================================
Hits 2755 2755
Misses 325 325
Partials 211 211
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Contributor
Author
|
Based off wrong branch |
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
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.
CMAKE_BUILD_TYPEs