Armor shared libraries with symbol visibility; add _internal companions and build-type-aware optimizations#322
Open
Armor shared libraries with symbol visibility; add _internal companions and build-type-aware optimizations#322
Conversation
Copilot
AI
changed the title
[WIP] Update branch with main and implement enhancements
Armor core libraries with symbol visibility
Feb 13, 2026
Contributor
|
@phlexbot format |
Contributor
|
Automatic markdownlint fixes pushed (commit 3299ec5). |
Contributor
|
No automatic jsonnetfmt fixes were necessary. |
Contributor
|
Automatic clang-format fixes pushed (commit 943ef76). |
Contributor
|
Automatic cmake-format fixes pushed (commit ccb09dd). |
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the symbol-visibility feature branch to main and completes hidden-by-default symbol export coverage across Phlex shared libraries, while also modernizing the driver/source plumbing (data-cell indexing, providers, product queries) and refreshing tests/examples accordingly.
Changes:
- Apply
GenerateExportHeader+ hidden visibility presets and expand export-macro coverage across all shared libraries. - Replace legacy level-id/label APIs with
data_cell_index+product_query(and update tests/configs/plugins accordingly). - Add/refresh Python/plugin tooling and CI hygiene (ruff/mypy config, SARIF helper, workflow tweaks).
Reviewed changes
Copilot reviewed 287 out of 289 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| test/yielding_driver.cpp | Update test to data_cell_index driver model |
| test/vector_of_abstract_types.cpp | Update graph test to providers + "_in(layer)" queries |
| test/utilities/CMakeLists.txt | Migrate tests to cet_test |
| test/specified_label.cpp | Remove tests for deleted specified_label API |
| test/replicated.cpp | Narrow namespace usage for updated utilities |
| test/python/verify.py | Add Python observer helper for tests |
| test/python/test_phlex.py | Update Python import expectations / docs |
| test/python/sumit.py | Add numpy-based Python algorithm test module |
| test/python/source.cpp | Add C++ providers for Python tests |
| test/python/registry.py | Remove old Python registry glue |
| test/python/reducer.py | Add Python reduction workflow test module |
| test/python/pyvec.jsonnet | Add Python/numpy config fixture |
| test/python/pyreduce.jsonnet | Add Python reduction config fixture |
| test/python/pyfailure.jsonnet | Add intentional Python config failure fixture |
| test/python/pyconfig.jsonnet | Add “all config types” Python fixture |
| test/python/pyadd.jsonnet | Update Python add fixture to new driver/sources layout |
| test/python/phlexpy.py | Update cppyy bootstrap + pythonization docs |
| test/python/failing_test_wrap.sh | Add wrapper for expected-failure tests |
| test/python/driver.cpp | Remove legacy C++ driver for Python tests |
| test/python/all_config.py | Add Python-side configuration consumer |
| test/python/adder.py | Replace cppyy-defined add with pure Python + registration |
| test/provider_test.cpp | New provider-focused integration test |
| test/product_store.cpp | Extend product_store tests for size/empty & id changes |
| test/product_query.cpp | New tests for product_query + _in literal |
| test/plugins/source.cpp | Remove legacy plugin source |
| test/plugins/output.cpp | Update plugin registration macro / namespaces |
| test/plugins/module.cpp | Update plugin to product_query + new macro |
| test/plugins/ij_source.cpp | New provider plugin for i/j inputs |
| test/plugins/add.jsonnet | Update plugin job config to new driver/sources layout |
| test/plugins/CMakeLists.txt | Add provider plugin + migrate job test to cet_test |
| test/multiple_function_registration.cpp | Update multi-transform test to provider inputs / new hierarchy |
| test/mock-workflow/three_tuple_algorithm.cpp | Update workflow module registration + input index type |
| test/mock-workflow/source.cpp | Remove legacy mock-workflow source |
| test/mock-workflow/pd_fast_sim.cpp | Update plugin registration macro |
| test/mock-workflow/mock-workflow.jsonnet | Update workflow config to driver+provider layout |
| test/mock-workflow/largeant.cpp | Update plugin registration macro |
| test/mock-workflow/ion_and_scint.cpp | Update plugin registration macro |
| test/mock-workflow/id_provider.cpp | New provider plugin for “id” product |
| test/mock-workflow/event_product.libsonnet | Add helper for event-layer product specs |
| test/mock-workflow/algorithm.hpp | Update algorithm test helper to use product_query inputs |
| test/mock-workflow/SinglesGen.libsonnet | Update config keys (cpp) + product specs |
| test/mock-workflow/MC_truth_algorithm.cpp | Update input index type |
| test/mock-workflow/G4Stage2.libsonnet | Update config keys + product specs |
| test/mock-workflow/G4Stage1.libsonnet | Update config keys + product specs |
| test/mock-workflow/CMakeLists.txt | Replace source plugin + migrate test execution to cet_test |
| test/memory-checks/many_events.cpp | Update memory-check driver/source plumbing |
| test/memory-checks/CMakeLists.txt | Migrate test to cet_test + link layer_generator |
| test/max-parallelism/provide_parallelism.cpp | Add provider plugin for parallelism value |
| test/max-parallelism/check_parallelism_default.jsonnet.in | Update config to driver+provider layout |
| test/max-parallelism/check_parallelism.cpp | Remove embedded source; update to product_query input |
| test/function_registration.cpp | Update test inputs to provider registration |
| test/framework_graph.cpp | Add new graph behavior tests (exceptions, single-thread progress) |
| test/form/test_helpers.hpp | Add helpers for FORM test type map |
| test/form/form_test.jsonnet | Add form-module job config |
| test/form/data_products/CMakeLists.txt | Clean up ROOT dictionary linkage |
| test/form/CMakeLists.txt | Migrate FORM tests to cet_test + add job test |
| test/filter_impl.cpp | Add test for data_map completeness behavior |
| test/demo-giantdata/waveforms.cpp | Remove logging hooks; simplify dtors |
| test/demo-giantdata/waveform_generator_input.cpp | Remove logging hooks; simplify dtors |
| test/demo-giantdata/waveform_generator.hpp | Remove unused members/includes |
| test/demo-giantdata/waveform_generator.cpp | Remove logging hooks; simplify ctor/dtor |
| test/demo-giantdata/user_algorithms.hpp | Simplify APIs (drop unused id args) |
| test/demo-giantdata/user_algorithms.cpp | Remove logging hooks; simplify clamp/accumulate |
| test/demo-giantdata/CMakeLists.txt | Convert demo to cet_test and link generator |
| test/data_cell_index.cpp | Replace level-id tests with data-cell index hashing test |
| test/configuration.cpp | Add configuration error and product_query parsing tests |
| test/class_registration.cpp | Update test inputs to providers + product_query |
| test/cached_product_stores.cpp | Remove tests for deleted cache helper |
| test/cached_execution_source.hpp | Remove legacy cached execution source |
| test/cached_execution.cpp | Update cached execution test to providers + layer generator |
| test/benchmarks/verify_even_fibonacci_numbers.cpp | Update plugin macro + product_query input parsing |
| test/benchmarks/verify_difference.cpp | Update config parsing to product_query inputs |
| test/benchmarks/read_index.cpp | Update plugin macro + config parsing |
| test/benchmarks/read_id.cpp | Update id type + input query |
| test/benchmarks/plus_one.cpp | Update plugin macro + input layer |
| test/benchmarks/plus_101.cpp | Update plugin macro + input layer |
| test/benchmarks/last_index.cpp | Update id type + input layer |
| test/benchmarks/benchmarks_source.cpp | Remove legacy benchmark source |
| test/benchmarks/benchmarks_provider.cpp | Add provider plugin for benchmark id product |
| test/benchmarks/benchmark-09.jsonnet | Update config to driver+provider layout |
| test/benchmarks/benchmark-08.jsonnet | Update config to driver+provider layout |
| test/benchmarks/benchmark-07.jsonnet | Update config to driver+provider layout |
| test/benchmarks/benchmark-06.jsonnet | Update config to driver+provider layout |
| test/benchmarks/benchmark-05.jsonnet | Update config to driver+provider layout |
| test/benchmarks/benchmark-04.jsonnet | Update config to driver+provider layout |
| test/benchmarks/benchmark-03.jsonnet | Update config to driver+provider layout |
| test/benchmarks/benchmark-02.jsonnet | Update config to driver+provider layout |
| test/benchmarks/benchmark-01.jsonnet | Update config to driver+provider layout |
| test/benchmarks/accept_fibonacci_numbers.cpp | Update plugin macro + product_query input parsing |
| test/benchmarks/accept_even_numbers.cpp | Update plugin macro + product_query input parsing |
| test/benchmarks/accept_even_ids.cpp | Update id type + config parsing |
| test/benchmarks/CMakeLists.txt | Replace source plugin with provider + migrate job tests |
| test/adjust_config.cpp | Add tests for module config normalization (cpp/py) |
| scripts/sarif-alerts.py | Add SARIF alert printing tool |
| scripts/export_llvm_lcov.py | Add docstrings for script entrypoints |
| scripts/create_coverage_symlinks.py | Add docstrings for coverage symlink helper |
| pyproject.toml | Introduce ruff+mypy configuration |
| plugins/python/src/wrap.hpp | Add Python registration wrappers + GIL RAII |
| plugins/python/CMakeLists.txt | Add Python plugin build + optional NumPy support |
| plugins/generate_layers.cpp | Add configurable layer-generator driver plugin |
| plugins/CMakeLists.txt | Add core plugins build/install |
| phlex/utilities/stripped_name.hpp | Remove deleted utility header |
| phlex/utilities/stripped_name.cpp | Remove deleted utility implementation |
| phlex/utilities/async_driver.hpp | Switch to std::jthread + propagate driver exceptions |
| phlex/utilities/CMakeLists.txt | Migrate utilities build/install to cetmodules + visibility |
| phlex/module.hpp | Replace old registration macro with shared plugin macro helper |
| phlex/model/qualified_name.hpp | Remove deprecated qualified-name API |
| phlex/model/qualified_name.cpp | Remove deprecated qualified-name implementation |
| phlex/model/products.hpp | Switch to product_specification + export + add size/empty |
| phlex/model/products.cpp | Implement size/empty |
| phlex/model/product_specification.hpp | Add replacement for qualified-name outputs (with type id) |
| phlex/model/product_specification.cpp | Implement product_specification + helpers |
| phlex/model/product_matcher.hpp | Rename level→layer terminology + export |
| phlex/model/product_matcher.cpp | Update matcher pattern strings for layer terminology |
| phlex/model/level_id.hpp | Remove legacy level-id API |
| phlex/model/level_counter.hpp | Remove legacy level-counter API |
| phlex/model/level_counter.cpp | Remove legacy level-counter implementation |
| phlex/model/handle.hpp | Update handle to carry data_cell_index |
| phlex/model/fwd.hpp | Add data-cell index fwd decls + update model fwd set |
| phlex/model/data_layer_hierarchy.hpp | Rename/replace level hierarchy with data-layer hierarchy |
| phlex/model/data_cell_index.hpp | Add new data-cell index type |
| phlex/model/data_cell_counter.hpp | Add new counters for flush accounting |
| phlex/model/data_cell_counter.cpp | Implement new counters |
| phlex/model/CMakeLists.txt | Migrate model build/install to cetmodules + visibility |
| phlex/metaprogramming/type_deduction.hpp | Expand traits/concepts (atomic/container/tuple helpers) |
| phlex/metaprogramming/CMakeLists.txt | Migrate metaprogramming install/export to cetmodules |
| phlex/graph/serializer_node.hpp | Include-order tweak for export header |
| phlex/graph/serial_node.hpp | Include-order tweak for export header |
| phlex/graph/CMakeLists.txt | Migrate graph build/install to cetmodules + visibility |
| phlex/driver.hpp | Add unified driver plugin registration + create_next helper |
| phlex/detail/plugin_macros.hpp | Add shared plugin registration macro machinery |
| phlex/core/upstream_predicates.hpp | Rename when to experimental_when |
| phlex/core/store_counters.cpp | Update flush/store accounting to data-cell ids |
| phlex/core/specified_label.hpp | Remove deprecated specified-label API |
| phlex/core/specified_label.cpp | Remove deprecated specified-label implementation |
| phlex/core/registration_api.cpp | Rename when to experimental_when in API |
| phlex/core/registrar.hpp | Export helper used across shared-library boundary |
| phlex/core/products_consumer.hpp | Switch inputs to product_query + export |
| phlex/core/products_consumer.cpp | Switch inputs to product_query |
| phlex/core/product_query.hpp | Add product_query + _in(layer) literal + type population |
| phlex/core/product_query.cpp | Implement product_query + literal error handling |
| phlex/core/node_catalog.hpp | Export node_catalog + add provider catalog |
| phlex/core/node_catalog.cpp | Add providers to execution_counts + throw on unknown name |
| phlex/core/multiplexer.hpp | Update multiplexer finalize model for providers |
| phlex/core/message_sender.hpp | Update sender to use data-layer hierarchy + data-cell ids |
| phlex/core/message_sender.cpp | Update to new hierarchy constructor |
| phlex/core/message.hpp | Switch to product_query ports + export required symbols |
| phlex/core/message.cpp | Update to data_cell_index and new error text |
| phlex/core/input_arguments.hpp | Switch retrievers to product_query + export duplicate check |
| phlex/core/input_arguments.cpp | Update duplicate detection types |
| phlex/core/graph_proxy.hpp | Add provide() to graph proxy + namespace adjustments |
| phlex/core/glue.hpp | Add provider registration + export name verification |
| phlex/core/fwd.hpp | Move framework_driver alias to phlex::framework_driver |
| phlex/core/filter.hpp | Export filter class |
| phlex/core/end_of_message.hpp | Update to new hierarchy + export |
| phlex/core/end_of_message.cpp | Update to new hierarchy types |
| phlex/core/edge_maker.hpp | Add provider edge support + export helper |
| phlex/core/edge_maker.cpp | Implement provider edge wiring |
| phlex/core/edge_creation_policy.hpp | Switch producer matching to product_query + include types |
| phlex/core/edge_creation_policy.cpp | Add type-aware producer matching + better diagnostics |
| phlex/core/detail/maybe_predicates.hpp | Export predicate extraction + rename config key |
| phlex/core/detail/maybe_predicates.cpp | Read experimental_when from config |
| phlex/core/detail/make_algorithm_name.hpp | Export algorithm-name helper |
| phlex/core/detail/filter_impl.hpp | Export filter maps + switch to product_query |
| phlex/core/detail/filter_impl.cpp | Fix completeness logic for sparse store vectors |
| phlex/core/declared_unfold.cpp | Update unfold generator to new data-cell API |
| phlex/core/declared_transform.cpp | Update ctor arg types + improve cached-store reporting |
| phlex/core/declared_provider.cpp | Add declared provider implementation |
| phlex/core/declared_predicate.hpp | Switch to product_query + data-cell hashes + export |
| phlex/core/declared_predicate.cpp | Switch ctor arg types |
| phlex/core/declared_output.hpp | Export declared_output |
| phlex/core/declared_observer.hpp | Switch to product_query + data-cell hashes + export |
| phlex/core/declared_observer.cpp | Switch cached-hash map key type |
| phlex/core/declared_fold.cpp | Switch ctor arg types |
| phlex/core/consumer.hpp | Export consumer base |
| phlex/core/concurrency.cpp | Move concurrency constants to phlex namespace |
| phlex/core/concepts.hpp | Add provider concept |
| phlex/core/cached_product_stores.hpp | Remove deprecated store-cache helper |
| phlex/configuration.cpp | Add product_query JSON conversion + keys() helper |
| phlex/concurrency.hpp | Export concurrency + move to phlex namespace |
| phlex/app/version.hpp | Export version() across shared library boundary |
| phlex/app/run.hpp | Export run() across shared library boundary |
| phlex/app/run.cpp | Update config schema (driver/modules/sources) + better errors |
| phlex/app/phlex.cpp | Catch+print top-level exceptions in CLI |
| phlex/app/load_module.hpp | Export app API + add adjust_config + new loader functions |
| phlex/app/CMakeLists.txt | Migrate app build to cetmodules + symbol visibility |
| phlex/CMakeLists.txt | Register export set + split configuration lib + install headers |
| form/storage/CMakeLists.txt | Formatting / target definition cleanup |
| form/root_storage/root_ttree_container.cpp | Fix write semantics on destruction + add guard |
| form/root_storage/CMakeLists.txt | Formatting / target definition cleanup |
| form/mock_phlex/phlex_toy_core.hpp | Remove mock-phlex shim (no longer used) |
| form/mock_phlex/phlex_toy_core.cpp | Remove mock-phlex shim implementation |
| form/mock_phlex/phlex_toy_config.hpp | Remove mock-phlex config shim |
| form/mock_phlex/phlex_toy_config.cpp | Remove mock-phlex config shim implementation |
| form/mock_phlex/CMakeLists.txt | Remove mock-phlex library |
| form/form/form.hpp | Replace mock-phlex types with FORM-local types |
| form/README.md | Minor markdown cleanup |
| form/CMakeLists.txt | Enable PIC + add form_module plugin |
| codecov.yml | Raise coverage thresholds |
| README-dev.md | Remove developer notes file |
| Modules/private/PhlexTargetUtils.cmake | CMake formatting/cleanup |
| Modules/private/PhlexSymbolVisibility.cmake | CMake formatting/cleanup for visibility helper |
| .github/workflows/python-fix.yaml | Add workflow to auto-fix Python formatting/lint |
| .github/workflows/add-issues.yaml | Pin action by SHA + permissions hardening |
| .github/dependabot.yml | Add Dependabot for GitHub Actions |
| .github/actions/get-pr-info/action.yaml | Add composite action to resolve PR refs from comments |
| .github/actions/generate-build-matrix/action.yaml | Add composite action for dynamic build matrix |
| .github/actions/detect-relevant-changes/action.yaml | Add python file patterns + ref validation |
| .github/actions/detect-act-env/action.yaml | Add act-environment detection helper |
| .github/actions/configure-cmake/action.yaml | Refactor CMake configure args + enable color forcing |
| .github/actions/build-cmake/action.yaml | Force colored output |
| .github/actionlint.yaml | Add actionlint configuration scaffolding |
| .github/CodeQL-README.md | Markdown/formatting improvements |
| .github/CODEOWNERS | Add CODEOWNERS enforcement baseline |
| .flake8 | Remove flake8 config (ruff replaces it) |
Contributor
|
@copilot I think for maximum clarity, it would be better to reset this branch to upstream/main and then re-do the work following the current philosophy, library names, etc. Otherwise this looks like a huge body of work that would be way to much for a human to review. |
Contributor
Author
Done. In commit |
greenc-FNAL
force-pushed
the
copilot/update-branch-with-main
branch
2 times, most recently
from
e65cb92 to
e44c62a
Compare
greenc-FNAL
changed the base branch from
jules-feature-plugin-symbol-visibility
to
main
Contributor
|
@copilot apply changes based on the comments in this thread |
Contributor
Author
All three review comments addressed in
|
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #322 +/- ##
==========================================
+ Coverage 84.94% 84.97% +0.02%
==========================================
Files 127 127
Lines 3341 3341
Branches 574 574
==========================================
+ Hits 2838 2839 +1
Misses 306 306
+ Partials 197 196 -1
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
|
@phlexbot clang-fix |
Contributor
|
@phlexbot cmake-fix |
Contributor
|
Automatic cmake-format fixes pushed (commit bcffb4a). |
Contributor
|
@pcanal @knoepfel Just based on size for a Release build, the public libraries for |
Contributor
|
Also, the claim is that intelligent use of symbol hiding allows more aggressive optimization within compilation units. |
Copilot
AI
requested review from
aolivier23,
knoepfel,
marcpaterno and
sabasehrish
as code owners
March 10, 2026 18:09
Copilot
AI
changed the title
Armor shared libraries with hidden symbol visibility; add _internal companions for testing and comparison
Armor shared libraries with hidden symbol visibility; add _internal companions, PHLEX_HIDE_SYMBOLS toggle, and optimizer flags
Mar 10, 2026
Copilot
AI
changed the title
Armor shared libraries with hidden symbol visibility; add _internal companions, PHLEX_HIDE_SYMBOLS toggle, and optimizer flags
Armor shared libraries with hidden symbol visibility; add _internal companions, optimization flags, and PHLEX_HIDE_SYMBOLS toggle
Mar 10, 2026
Contributor
|
@phlexbot cmake-fix |
Contributor
|
Automatic cmake-format fixes pushed (commit c63bd45). |
… libraries, introduces non-installed `_internal` companion libraries for testing and comparison, and adds optimization flags that automatically exploit the visibility model.
## New CMake infrastructure
**`Modules/private/PhlexSymbolVisibility.cmake`**
- `phlex_apply_symbol_visibility(target)` — generates `phlex/X_export.hpp` (installed to `include/phlex/`), sets `-fvisibility=hidden` when `PHLEX_HIDE_SYMBOLS=ON`
- `phlex_make_internal_library(target LIBRARIES ...)` — `PHLEX_HIDE_SYMBOLS=ON`: compiles a separate SHARED lib from the same sources with full visibility (no source duplication — reads via `get_target_property`); `PHLEX_HIDE_SYMBOLS=OFF`: thin INTERFACE alias to the public target
**`Modules/private/PhlexOptimization.cmake`**
- `phlex_apply_optimizations(target)` — applies per-target flags based on option values
- `PHLEX_HIDE_SYMBOLS` and `PHLEX_ENABLE_IPO` default `ON` for `Release`/`RelWithDebInfo`, `OFF` otherwise; all four combinations are valid for benchmarking
| `PHLEX_HIDE_SYMBOLS` | `PHLEX_ENABLE_IPO` | Flags added |
|---|---|---|
| ON | ON | LTO + `-fno-semantic-interposition` + `-fno-plt` |
| OFF | ON | LTO + `-fno-plt` |
| ON | OFF | `-fno-semantic-interposition` + `-fno-plt` |
| OFF | OFF | `-fno-plt` |
`-fno-semantic-interposition` is gated on `PHLEX_HIDE_SYMBOLS=ON` — it is only safe once the exported-symbol surface is explicitly bounded. `-ffast-math` intentionally excluded.
## Export macro coverage
~45 headers annotated. Every non-template class and free function with a `.cpp` implementation carries `phlex_X_EXPORT`; template/header-only code is left unannotated. Boost.JSON `tag_invoke` overloads for `configuration`, `product_query`, and `identifier` explicitly exported (required for `value_to<T>()`).
```cpp
#include "phlex/phlex_core_export.hpp"
class phlex_core_EXPORT framework_graph { ... };
phlex_core_EXPORT std::size_t port_index_for(...);
```
## `_internal` companion libraries
| Target | Alias | Transitively includes |
|---|---|---|
| `phlex_utilities_internal` | `phlex::utilities_internal` | — |
| `phlex_model_internal` | `phlex::model_internal` | utilities |
| `phlex_core_internal` | `phlex::core_internal` | model, utilities |
| `run_phlex_internal` | — | core |
Tests link exclusively to `phlex::core_internal`; benchmark tests (`test/benchmarks/`, `test/max-parallelism/`) link exclusively to curated public targets — isolating the two `.so` sets per process to avoid ODR. `layer_generator_internal` follows the same `PHLEX_HIDE_SYMBOLS` conditionality as the core libs.
<!-- START COPILOT CODING AGENT TIPS -->
---
💬 We'd love your input! Share your thoughts on Copilot coding agent in our [2 minute survey](https://gh.io/copilot-coding-agent-survey).
Contributor
|
Review the full CodeQL report for details. |
|
|
||
| get_target_property(opts ${target} COMPILE_OPTIONS) | ||
| if(opts) | ||
| target_compile_options(${internal} PRIVATE ${opts}) |
There was a problem hiding this comment.
phlex_make_internal_library() copies the public target's COMPILE_OPTIONS onto the _internal target. When PHLEX_HIDE_SYMBOLS is ON, that includes -fno-semantic-interposition from phlex_apply_optimizations(), but the _internal library is explicitly built with full/default visibility (all symbols interposable). This undermines the safety assumptions documented for -fno-semantic-interposition and can make internal-vs-public benchmarking inaccurate. Filter out -fno-semantic-interposition for _internal targets (or avoid copying optimization flags wholesale and re-apply only the safe subset).
Suggested change
| target_compile_options(${internal} PRIVATE ${opts}) | |
| # Copy most compile options from the public target, but drop | |
| # -fno-semantic-interposition because _internal libraries are built | |
| # with default (interposable) visibility, which violates the safety | |
| # assumptions for that flag and would skew internal/public benchmarks. | |
| set(safe_opts "${opts}") | |
| list(FILTER safe_opts EXCLUDE REGEX "^-fno-semantic-interposition$") | |
| if(safe_opts) | |
| target_compile_options(${internal} PRIVATE ${safe_opts}) | |
| endif() |
Comment on lines
+128
to
+130
| Combined with PHLEX_ENABLE_IPO=ON: -fno-semantic-interposition is also applied | ||
| for maximum optimization. Setting OFF while PHLEX_ENABLE_IPO=ON is valid and | ||
| useful for comparing LTO performance with and without symbol hiding.]=] |
There was a problem hiding this comment.
The help text for PHLEX_HIDE_SYMBOLS says -fno-semantic-interposition is applied when combined with PHLEX_ENABLE_IPO=ON, but phlex_apply_optimizations() applies -fno-semantic-interposition whenever PHLEX_HIDE_SYMBOLS is ON (independent of IPO). Please update the option description (or gate the flag on PHLEX_ENABLE_IPO) so the documented behavior matches the implementation.
Suggested change
| Combined with PHLEX_ENABLE_IPO=ON: -fno-semantic-interposition is also applied | |
| for maximum optimization. Setting OFF while PHLEX_ENABLE_IPO=ON is valid and | |
| useful for comparing LTO performance with and without symbol hiding.]=] | |
| When ON and supported by the compiler, -fno-semantic-interposition is also | |
| applied for additional optimization. This behavior is independent of | |
| PHLEX_ENABLE_IPO; both options can be toggled separately to compare profiles.]=] |
| */ | ||
| template <typename T> | ||
| class glue { | ||
| class phlex_core_EXPORT glue { |
There was a problem hiding this comment.
glue is a class template defined in this header. Exporting a template with phlex_core_EXPORT is generally incorrect on platforms that use dllimport/dllexport (templates are instantiated in the consumer unless explicitly instantiated in the library) and contradicts the stated approach of leaving header-only/template code unannotated. Remove the export macro from the template (or switch to explicit instantiation in the shared library if you truly need to export specific instantiations).
Suggested change
| class phlex_core_EXPORT glue { | |
| class glue { |
Comment on lines
10
to
11
| static concurrency const unlimited; | ||
| static concurrency const serial; |
There was a problem hiding this comment.
Marking concurrency as exported does not automatically ensure the out-of-class definitions of the static data members (unlimited, serial) are exported when -fvisibility=hidden is enabled. This can cause link failures for consumers referencing concurrency::unlimited/serial in Release/RelWithDebInfo builds. Export the static data members themselves (declaration and definition), or make them inline variables in the header so no exported definition is required.
Suggested change
| static concurrency const unlimited; | |
| static concurrency const serial; | |
| static phlex_core_EXPORT concurrency const unlimited; | |
| static phlex_core_EXPORT concurrency const serial; |
Comment on lines
18
to
20
| cet_test(config_test USE_CATCH2_MAIN SOURCE configuration.cpp LIBRARIES | ||
| phlex::configuration | ||
| ) |
There was a problem hiding this comment.
PR description says tests link exclusively to phlex::core_internal, but this file still defines tests that link against public targets (e.g., config_test links to phlex::configuration). Please reconcile the description with the actual test linkage strategy, or update these remaining tests to use _internal targets if that was the intent.
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.
Enforces hidden-by-default symbol visibility on all five Phlex shared libraries, introduces non-installed
_internalcompanion libraries for testing and comparison, and adds optimization flags that automatically exploit the visibility model.New CMake infrastructure
Modules/private/PhlexSymbolVisibility.cmakephlex_apply_symbol_visibility(target)— generatesphlex/X_export.hpp(installed toinclude/phlex/), sets-fvisibility=hiddenwhenPHLEX_HIDE_SYMBOLS=ONphlex_make_internal_library(target LIBRARIES ...)—PHLEX_HIDE_SYMBOLS=ON: compiles a separate SHARED lib from the same sources with full visibility (no source duplication — reads viaget_target_property);PHLEX_HIDE_SYMBOLS=OFF: thin INTERFACE alias to the public targetModules/private/PhlexOptimization.cmakephlex_apply_optimizations(target)— applies per-target flags based on option valuesPHLEX_HIDE_SYMBOLSandPHLEX_ENABLE_IPOdefaultONforRelease/RelWithDebInfo,OFFotherwise; all four combinations are valid for benchmarkingPHLEX_HIDE_SYMBOLSPHLEX_ENABLE_IPO-fno-semantic-interposition+-fno-plt-fno-plt-fno-semantic-interposition+-fno-plt-fno-plt-fno-semantic-interpositionis gated onPHLEX_HIDE_SYMBOLS=ON— it is only safe once the exported-symbol surface is explicitly bounded.-ffast-mathintentionally excluded.Export macro coverage
~45 headers annotated. Every non-template class and free function with a
.cppimplementation carriesphlex_X_EXPORT; template/header-only code is left unannotated. Boost.JSONtag_invokeoverloads forconfiguration,product_query, andidentifierexplicitly exported (required forvalue_to<T>())._internalcompanion librariesphlex_utilities_internalphlex::utilities_internalphlex_model_internalphlex::model_internalphlex_core_internalphlex::core_internalrun_phlex_internalTests link exclusively to
phlex::core_internal; benchmark tests (test/benchmarks/,test/max-parallelism/) link exclusively to curated public targets — isolating the two.sosets per process to avoid ODR.layer_generator_internalfollows the samePHLEX_HIDE_SYMBOLSconditionality as the core libs.💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.