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.
There was a problem hiding this comment.
Pull request overview
This PR enhances build-time and runtime performance tooling in Phlex by adding compile/link profiling support, tightening shared-library symbol visibility via generated export headers, and introducing _internal companion libraries to keep tests working with hidden symbols.
Changes:
- Add CMake modules for symbol visibility (generated export headers) and optimization flags (LTO,
-fno-semantic-interposition,-fno-plt) and apply them across shared libraries. - Introduce and adopt
_internalcompanion libraries for tests/plugins to avoid visibility/linkage issues. - Enable optional compile/link profiling and apply formatting fixes across Python and GitHub workflow YAML.
Reviewed changes
Copilot reviewed 68 out of 68 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
CMakeLists.txt |
Adds default build-type flags, includes new optimization/visibility modules, and adds optional build profiling flags. |
Modules/private/PhlexSymbolVisibility.cmake |
New helper to generate/install export headers and optionally hide symbols; adds _internal companion library helper. |
Modules/private/PhlexOptimization.cmake |
New helper to configure IPO/LTO and apply safe optimization flags. |
phlex/CMakeLists.txt |
Applies symbol visibility + optimizations to phlex_configuration_internal. |
phlex/app/CMakeLists.txt |
Applies symbol visibility/optimizations to run_phlex and creates run_phlex_internal. |
phlex/app/load_module.hpp |
Exports app APIs and adds missing includes. |
phlex/app/run.hpp |
Exports run() API. |
phlex/app/version.hpp |
Exports version() API. |
phlex/configuration.hpp |
Exports configuration + tag_invoke customization points and helper(s). |
phlex/concurrency.hpp |
Exports concurrency struct. |
phlex/core/CMakeLists.txt |
Applies symbol visibility/optimizations and creates phlex_core_internal + alias. |
phlex/core/consumer.hpp |
Adds core export header and exports consumer. |
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 generator and declared_unfold. |
phlex/core/detail/filter_impl.hpp |
Adds export header and exports internal classes used across DSOs. |
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. |
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). |
phlex/core/index_router.hpp |
Exports index_router and related helper classes. |
phlex/core/input_arguments.hpp |
Exports verify_no_duplicate_input_products helper. |
phlex/core/message.hpp |
Exports matcher/utilities used across library boundaries. |
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. |
phlex/core/registration_api.hpp |
Exports output_api. |
phlex/core/store_counters.hpp |
Exports store_counter and count_stores. |
phlex/model/CMakeLists.txt |
Applies symbol visibility/optimizations and creates phlex_model_internal + alias. |
phlex/model/algorithm_name.hpp |
Adds export header and exports algorithm_name. |
phlex/model/data_cell_counter.hpp |
Adds export header and exports counter-related classes. |
phlex/model/data_cell_index.hpp |
Adds export header and exports data_cell_index + stream operator. |
phlex/model/data_layer_hierarchy.hpp |
Adds export header and exports data_layer_hierarchy. |
phlex/model/identifier.hpp |
Adds export header; exports identifier and related operators/UDLs. |
phlex/model/product_matcher.hpp |
Adds export header and exports product_matcher. |
phlex/model/product_specification.hpp |
Adds export header; exports product_specification and to_product_specifications. |
phlex/model/product_store.hpp |
Adds export header; exports product_store and more_derived. |
phlex/model/products.hpp |
Adds export header; exports product container APIs. |
phlex/utilities/CMakeLists.txt |
Applies symbol visibility/optimizations and creates phlex_utilities_internal + alias. |
phlex/utilities/hashing.hpp |
Adds export header and exports hashing overloads. |
phlex/utilities/resource_usage.hpp |
Adds export header and exports resource_usage. |
plugins/CMakeLists.txt |
Adds layer_generator_internal companion target for tests. |
test/CMakeLists.txt |
Switches many tests to link against _internal libraries. |
test/utilities/CMakeLists.txt |
Switches utilities tests to link against phlex::utilities_internal. |
test/python/all_config.py |
Ruff formatting fixes. |
test/python/reducer.py |
Ruff formatting changes to transform registrations. |
test/python/sumit.py |
Ruff formatting changes to transform registrations. |
test/python/test_coverage.py |
Ruff formatting changes to tuple formatting. |
test/python/test_mismatch.py |
Ruff formatting changes to transform registration call. |
test/python/vectypes.py |
Ruff formatting changes to nested dict/list formatting. |
.github/workflows/clang-format-fix.yaml |
YAML formatting fixes. |
.github/workflows/clang-tidy-fix.yaml |
YAML formatting fixes. |
.github/workflows/cmake-format-fix.yaml |
YAML formatting fixes. |
.github/workflows/codeql-analysis.yaml |
YAML formatting fix (compact needs). |
.github/workflows/header-guards-fix.yaml |
YAML formatting fixes. |
.github/workflows/jsonnet-format-fix.yaml |
YAML formatting fixes. |
.github/workflows/markdown-fix.yaml |
YAML formatting fixes. |
.github/workflows/python-fix.yaml |
YAML formatting fixes. |
.github/workflows/yaml-fix.yaml |
YAML formatting fixes. |
| if(PHLEX_HIDE_SYMBOLS) | ||
| add_library(layer_generator_internal layer_generator.cpp) | ||
| target_link_libraries(layer_generator_internal PRIVATE phlex_core_internal) | ||
| else() |
There was a problem hiding this comment.
layer_generator_internal is created without an explicit library type. That means it becomes STATIC unless BUILD_SHARED_LIBS is set, which doesn’t match the comment that describes a separate .so build (and can change link/load behavior when profiling symbol visibility). Consider declaring it explicitly as SHARED (and, for consistency with the rest of the project, link against the phlex::core_internal alias rather than the raw phlex_core_internal target).
| @@ -58,11 +73,34 @@ set(CTEST_TEST_TIMEOUT 90 CACHE STRING "Per-test timeout (s) for CTest") | |||
| FetchContent_MakeAvailable(Catch2 GSL mimicpp) | |||
|
|
|||
There was a problem hiding this comment.
PhlexOptimization.cmake derives default values for PHLEX_ENABLE_IPO / PHLEX_HIDE_SYMBOLS from CMAKE_BUILD_TYPE, but CMAKE_BUILD_TYPE is only defaulted later in this file (to RelWithDebInfo). As a result, when users don’t set a build type explicitly, these options will default as if the build type were empty (OFF) even though the project later forces RelWithDebInfo. Consider setting the default CMAKE_BUILD_TYPE before including PhlexOptimization.cmake, or reworking the option-default logic so it reflects the eventual default build type.
| # Ensure a default build type is set before optimization options that depend on it. | |
| # Only apply to single-config generators and when the user has not set a build type. | |
| if(NOT CMAKE_CONFIGURATION_TYPES AND NOT CMAKE_BUILD_TYPE) | |
| set( | |
| CMAKE_BUILD_TYPE | |
| RelWithDebInfo | |
| CACHE STRING "Build type (defaulted to RelWithDebInfo if not set by user)" | |
| ) | |
| endif() |
| if(CMAKE_CXX_COMPILER_LINKER_ID STREQUAL "LLD") | ||
| message(STATUS "LLD detected (Phlex build) - Enabling tracing") | ||
| add_link_options("-Wl,--time-trace") | ||
| add_link_options("-Wl,--Map=phlex_link.map") |
There was a problem hiding this comment.
The build-profiling linker options are added globally, and -Wl,--Map=phlex_link.map uses a fixed filename. With multiple targets (or parallel links), this map file will be repeatedly overwritten and can race, producing incomplete or misleading output. Consider making the map filename target-specific (or omitting the map by default), and also gating these -Wl,--* flags to ELF platforms where they’re supported (lld on macOS/Mach-O typically won’t accept the same options).
| if(CMAKE_CXX_COMPILER_LINKER_ID STREQUAL "LLD") | |
| message(STATUS "LLD detected (Phlex build) - Enabling tracing") | |
| add_link_options("-Wl,--time-trace") | |
| add_link_options("-Wl,--Map=phlex_link.map") | |
| if(CMAKE_CXX_COMPILER_LINKER_ID STREQUAL "LLD" AND NOT APPLE) | |
| message(STATUS "LLD detected (Phlex build) - Enabling tracing") | |
| add_link_options("-Wl,--time-trace") |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #408 +/- ##
==========================================
+ Coverage 83.71% 83.80% +0.09%
==========================================
Files 126 126
Lines 3291 3291
Branches 583 583
==========================================
+ Hits 2755 2758 +3
Misses 325 325
+ Partials 211 208 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
Based off wrong branch |
CMAKE_BUILD_TYPEs