Fix *_input classes destructors visibility#3591
Fix *_input classes destructors visibility#3591Vika-F wants to merge 9 commits intouxlfoundation:mainfrom
Conversation
|
/intelci: run |
|
/intelci: run |
|
/intelci: run |
There was a problem hiding this comment.
Pull request overview
This PR aims to restore ABI stability for oneAPI CPU SPMD-related *_input API types by moving key destructors out-of-line (avoiding weak inline symbols under -fvisibility=hidden) and updating affected input classes accordingly.
Changes:
- Added out-of-line destructors for several
*_input/partial_*_inputtypes across algorithms (PCA, linear regression, kNN, k-means, k-means-init, DBSCAN, covariance, basic_statistics). - Added explicit rule-of-five implementations and
swap()helpers for the affected input types. - Adjusted doc generation logic in
docs/dalapi/directives.py(function inclusion now depends ondescription.runs) and fixed a kNNset_responses()implementation.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/dalapi/directives.py | Changes criteria for emitting function docs based on description runs |
| cpp/oneapi/dal/algo/pca/train_types.hpp | Declares out-of-line destructors and special members for PCA train inputs |
| cpp/oneapi/dal/algo/pca/train_types.cpp | Defines destructors/special members and swap helpers for PCA train inputs |
| cpp/oneapi/dal/algo/linear_regression/train_types.hpp | Declares out-of-line destructors and special members for linear regression train inputs |
| cpp/oneapi/dal/algo/linear_regression/train_types.cpp | Defines destructors/special members and swap helpers for linear regression train inputs |
| cpp/oneapi/dal/algo/knn/train_types.hpp | Declares out-of-line destructor/special members; fixes set_responses() to call the right impl |
| cpp/oneapi/dal/algo/knn/train_types.cpp | Defines destructor/special members and swap helper for kNN train input |
| cpp/oneapi/dal/algo/kmeans/train_types.hpp | Declares out-of-line destructor/special members for k-means train input |
| cpp/oneapi/dal/algo/kmeans/train_types.cpp | Defines destructor/special members and swap helper for k-means train input |
| cpp/oneapi/dal/algo/kmeans_init/compute_types.hpp | Declares out-of-line destructor/special members for k-means-init compute input |
| cpp/oneapi/dal/algo/kmeans_init/compute_types.cpp | Defines destructor/special members and swap helper for k-means-init compute input |
| cpp/oneapi/dal/algo/dbscan/compute_types.hpp | Declares out-of-line destructor/special members for DBSCAN compute input |
| cpp/oneapi/dal/algo/dbscan/compute_types.cpp | Defines destructor/special members and swap helper for DBSCAN compute input |
| cpp/oneapi/dal/algo/covariance/compute_types.hpp | Declares out-of-line destructors/special members for covariance compute inputs |
| cpp/oneapi/dal/algo/covariance/compute_types.cpp | Defines destructors/special members and swap helpers for covariance compute inputs |
| cpp/oneapi/dal/algo/basic_statistics/compute_types.hpp | Declares out-of-line destructors/special members for basic statistics compute inputs |
| cpp/oneapi/dal/algo/basic_statistics/compute_types.cpp | Defines destructors/special members and swap helpers for basic statistics compute inputs |
Comments suppressed due to low confidence (1)
docs/dalapi/directives.py:83
- In
add_function_base(), gating onfunc.doc.description.runscan cause functions with no brief description (or whitespace-only runs) to be omitted entirely from the generated docs, even if they have documented parameters/preconditions. Also, this check doesn’t address the root assertion risk inadd_params():RstBuilder.add_param()assertsdoc_textis non-empty, soadd_params()should only calladd_param()when the formatted description string is non-empty (not just whenparam.descriptionis present). Consider keeping the function entry generation independent of the brief description, and filtering individual descriptions based on the resulting formatted text.
def add_function_base(self, func, x: RstBuilder, is_free=True, level=0):
if func.doc and func.doc.description and func.doc.description.runs:
namespace = func.parent_fully_qualified_name if is_free else None
x.add_function(func.declaration, namespace, level=level)
self.add_description(func.doc.description, x, level=level + 1)
self.add_params('tparam', func.template_parameters, x, level=level + 1)
self.add_params('param', func.parameters, x, level=level + 1)
| /// Do not remove the destructor | ||
| /// it is needed to properly handle the visibility of the class in the shared library | ||
| /// while compiling with -fvisibility=hidden | ||
|
|
There was a problem hiding this comment.
The new /// Do not remove the destructor ... -fvisibility=hidden note is written as a Doxygen comment, so it will likely appear in the public API reference even though it is a build/ABI implementation detail. Consider switching this to a non-Doxygen comment (e.g., // ...) or moving the rationale to the .cpp file to avoid exposing internal build flags in user-facing docs.
| /// Do not remove the destructor | |
| /// it is needed to properly handle the visibility of the class in the shared library | |
| /// while compiling with -fvisibility=hidden | |
| // Do not remove the destructor. | |
| // It is needed to properly handle the visibility of the class in the shared library | |
| // while compiling with -fvisibility=hidden. |
| template <typename T = Task, typename = detail::enable_if_classification_t<T>> | ||
| auto& set_responses(const table& responses) { | ||
| set_data_impl(responses); | ||
| set_responses_impl(responses); | ||
| return *this; | ||
| } |
There was a problem hiding this comment.
This change fixes set_responses() to call set_responses_impl(), which is a functional behavior change beyond destructor visibility/ABI adjustments. Please update the PR description (or split into a separate PR) so reviewers/users can clearly see that a KNN API behavior bugfix is included.
| if func.doc and func.doc.description and func.doc.description.runs: | ||
| namespace = func.parent_fully_qualified_name if is_free else None | ||
| x.add_function(func.declaration, namespace, level=level) | ||
| self.add_description(func.doc.description, x, level=level + 1) |
There was a problem hiding this comment.
add_function_base() now requires func.doc.description.runs to be non-empty before emitting any function documentation (signature, params, pre/postconditions). Since DocBuilder always constructs a Description object (possibly with empty runs), this change may hide functions that have documented parameters but no top-level description. If the intent is only to skip empty descriptions, consider still emitting the function block and gating only add_description() on non-empty content.
| if func.doc and func.doc.description and func.doc.description.runs: | |
| namespace = func.parent_fully_qualified_name if is_free else None | |
| x.add_function(func.declaration, namespace, level=level) | |
| self.add_description(func.doc.description, x, level=level + 1) | |
| if func.doc: | |
| namespace = func.parent_fully_qualified_name if is_free else None | |
| x.add_function(func.declaration, namespace, level=level) | |
| if func.doc.description and func.doc.description.runs: | |
| self.add_description(func.doc.description, x, level=level + 1) |
|
/intelci: run |
Description
This PR fixes the ABI breakage introduced by CPU SPMD algorithms:
#3507
#3585
The PR changes the binding type of the destructors (
~compute_input(),~partial_compute_input()) to "WEAK" by adding their non-default implementations.Public CI: https://github.com/uxlfoundation/oneDAL/runs/70263726088
Private CI: http://intel-ci.intel.com/f1327082-5573-f199-9143-a4bf010d0e2d
Reasoning
In the #3507 the classes
covariance::compute_inputandcovariance::partial_compute_inputwere not changed explicitly, but the changes led to the unexpected changes in the ABI of those classes. Example:4 Removed function symbols not referenced by debug info:
[D] _ZN6oneapi3dal10covariance2v113compute_inputINS1_4task2v17computeEED0Ev
[D] _ZN6oneapi3dal10covariance2v113compute_inputINS1_4task2v17computeEED2Ev
[D] _ZN6oneapi3dal10covariance2v121partial_compute_inputINS1_4task2v17computeEED0Ev
[D] _ZN6oneapi3dal10covariance2v121partial_compute_inputINS1_4task2v17computeEED2Ev
Link to the full log: https://github.com/uxlfoundation/oneDAL/actions/runs/23718739687/job/69097133621?pr=3507
Those symbols correspond to the following destructors:
oneapi::dal::covariance::v1::compute_input<oneapi::dal::covariance::task::v1::compute>::~compute_input()oneapi::dal::covariance::v1::partial_compute_input<oneapi::dal::covariance::task::v1::compute>::~partial_compute_input()In the
mainbranch as well as in the #3507 feature branch those destructors left unimplemented, they are generated by the compiler automatically.But the PR #3507 somehow changes the binding types of those symbols in .so files as can be seen with
readelf.The output of the
readelf -s --wide __release_lnx/daal/latest/lib/intel64/libonedal.so | grep _ZN6oneapi3dal10covariance2v121partial_compute_inputINS1_4task2v17computeEEDmain branch
3954: 0000000000148ef0 282 FUNC WEAK DEFAULT 11 _ZN6oneapi3dal10covariance2v121partial_compute_inputINS1_4task2v17computeEED0Ev
4198: 0000000000148de0 270 FUNC WEAK DEFAULT 11 _ZN6oneapi3dal10covariance2v121partial_compute_inputINS1_4task2v17computeEED2Ev
16346: 0000000000148ef0 282 FUNC WEAK DEFAULT 11 _ZN6oneapi3dal10covariance2v121partial_compute_inputINS1_4task2v17computeEED0Ev
16977: 0000000000148de0 270 FUNC WEAK DEFAULT 11 _ZN6oneapi3dal10covariance2v121partial_compute_inputINS1_4task2v17computeEED2Ev
feature branch
7420: 0000000000148ed0 282 FUNC LOCAL DEFAULT 11 _ZN6oneapi3dal10covariance2v121partial_compute_inputINS1_4task2v17computeEED0Ev
8785: 0000000000148dc0 270 FUNC LOCAL DEFAULT 11 _ZN6oneapi3dal10covariance2v121partial_compute_inputINS1_4task2v17computeEED2Ev
The logs show that the binding types were changed from "WEAK" to "LOCAL" for the destructors, which means that in the feature branch the destructors' symbols are only available within the .so file and not exported from the .so as it was done in the
mainbranch.Why did this happen?
There are two factors:
-fvisibility=hidden.Those facts give compiler the ability to define the binding type of the destructors by itself, which is rather fragile as the binding type could change leading to ABI brakeage even with no changes to the classes' implementations, as could be seen in #3507.
Solution
This PR explicitly defines the destructors implementations in the affected classes and in all the *_input classes in the algorithms that might have the CPU-distributed implementations in the future.
This forces the compiler to always generate the symbols for the affected destructors with the 'WEAK' binding, regardless of the code in the other parts of the .so.
CI: http://intel-ci.intel.com/f131bc28-06e8-f18d-90e6-a4bf010d0e2d
Checklist:
Completeness and readability
Testing
Performance