Fix dynamic cast leads the undefined symbol in clang/msys2/windows #1724

yhmtsai · 2024-11-12T15:52:17Z

It is reported in #1654 . We have some undefined symbol when building the shared library (libginkgo_reference) by the clang-18 from msys2 in windows.

In clang 18 (at least), the compiler include convert_to/move_to/... virtual function as undefined symbols when we use dynamic_cast. These symbols are from ConvertibleTo<..>, which contain general function like convert_to calling virtual functions, such that these functions are considered in the library.
It can be observed with clang-18 on linux, too. However, there are different behaviors between systems.

Linux - allow any undefined symbol when building shared library.
MacOS - allow undefined symbol with -undefined dynamic_lookup
Windows (specific to clang in msys2) - clang ld.lld has --allow-shlib-undefined, which is enabled by default, but it seems to be available in Linux https://man.archlinux.org/man/extra/lld/ld.lld.1.en In the environment, I check the help documentation and the linker also reports unknown argument when passing the argument.

The older compiler or gcc compiler put these function as weak symbol, so it does not complain undefined symbols.

This PR moves the matrix/preconditioner dispatch from the kernels to core such that there are no dynamic_cast in the codes.

It also needs to add -fno-assume-unique-vtables to clang/msys2/windowns to make the dynamic_cast to the class marked by final work.
https://reviews.llvm.org/D154658 should be relevant

TODO:

make the macros work in MSVC

MarcelKoch · 2024-11-18T14:04:14Z

core/base/batch_instantiation.hpp

+ *
+ * @note the second and third arguments only accept the base type.s
+ */
+#define GKO_INSTANTIATE_FOR_BATCH_VALUE_MATRIX_PRECONDITIONER(_macro)          \


I think this needs to be unified with the changes from #1629, but I don't immediately see how to do that.

I reuse the part in types.hpp.

I update the macro usage a bit such that make the for-stack easy and clear.
I think it can not reuse the main stack of dispatch type.
one dispatches the device data structure which are only available in namespace cuda/hip/..., but the other uses the main class.

MarcelKoch

I have some smaller comments. The changes to the macro structure are quite nice, thanks for that!

MarcelKoch · 2024-11-21T15:39:38Z

core/solver/batch_cg.cpp

+    run<batch::matrix::Dense<ValueType>, batch::matrix::Csr<ValueType>,
+        batch::matrix::Ell<ValueType>>(
+        this->system_matrix_.get(), [&](auto matrix) {
+            if (this->preconditioner_ == nullptr) {


maybe just set preconditioner_ to identity, so we don't need an if here.

It is already set to Identity if nullptr in generation.
set_preconditioner will ignore nullptr not change the underlying preconditioner.
preconditioner can not be nullptr in any way.

core/solver/batch_dispatch.hpp

MarcelKoch · 2024-11-21T15:44:40Z

core/base/batch_instantiation.hpp

+#define GKO_CALL(_macro, ...) _macro(__VA_ARGS__)
+
+#define GKO_BATCH_INSTANTIATE_PRECONDITIONER(_next, ...)            \
+    _next(GKO_INDIRECT(__VA_ARGS__), gko::batch::matrix::Identity); \


the GKO_INDIRECT should only be necessary on the root macro (GKO_INSTANTIATE_FOR_EACH_VALUE_TYPE in this case). So maybe just add it there, it should not hurt other usages of that macro.

I was using it wrongly.
no, I think it is required here.
in MSVC, they consider the VA_ARGS as one arguments by default.
GKO_CALL(A_loop, B_loop, ...) -> A_loop("B_loop, ...").
will just be "B_loop, ..."(A_1) "B_loop, ..."(A_2) -> only the first one get expanded properly, the B_loop, ... is considered as one arguments. no __VA_ARGS__ in A_loop.

I need to use GKO_INDIRECT to take the first arguments out of __VA_ARGS__ when it may have the nested call.

reference/test/solver/batch_bicgstab_kernels.cpp

reference/test/solver/batch_cg_kernels.cpp

upsj

LGTM! Just a naming nit

dpcpp/solver/batch_bicgstab_kernels.dp.cpp

In clang18 (at least), compiler will include convert_to/move_to/... as undefined symbol by dynamic_cast. It is not an issue in linux when building the shared library and OSX with `-undefined dynamic_lookup`. However, clang in msys2 (WINDOWS) does not have `--allow-shlib-undefined`, which should be enabled by default when building shared libraries.

…s work

Co-authored-by: Marcel Koch <[email protected]>

sonarqubecloud · 2024-11-22T11:29:05Z

Quality Gate failed

Failed conditions
20.7% Duplication on New Code (required ≤ 20%)

See analysis details on SonarQube Cloud

codecov · 2024-11-22T11:34:11Z

Codecov Report

Attention: Patch coverage is 84.84848% with 5 lines in your changes missing coverage. Please review.

Project coverage is 91.63%. Comparing base (75434f7) to head (1bff73b).
Report is 11 commits behind head on develop.

Files with missing lines	Patch %	Lines
test/preconditioner/batch_jacobi_kernels.cpp	0.00%	3 Missing ⚠️
core/device_hooks/common_kernels.inc.cpp	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1724      +/-   ##
===========================================
+ Coverage    89.94%   91.63%   +1.68%     
===========================================
  Files          782      782              
  Lines        63456    63243     -213     
===========================================
+ Hits         57078    57952     +874     
+ Misses        6378     5291    -1087

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

… in clang/msys2/windows This PR fixes dynamic cast leads the undefined symbol in clang/msys2/windows and update the stack macro usage Related PR: ginkgo-project#1724

yhmtsai self-assigned this Nov 12, 2024

ginkgo-bot added reg:ci-cd This is related to the continuous integration system. type:solver This is related to the solvers mod:all This touches all Ginkgo modules. labels Nov 12, 2024

yhmtsai added the 1:ST:WIP This PR is a work in progress. Not ready for review. label Nov 12, 2024

yhmtsai force-pushed the batch_dynamic_cast branch 6 times, most recently from ae83bd6 to 848adaf Compare November 13, 2024 13:09

yhmtsai added 1:ST:ready-for-review This PR is ready for review and removed 1:ST:WIP This PR is a work in progress. Not ready for review. labels Nov 13, 2024

yhmtsai force-pushed the batch_dynamic_cast branch 2 times, most recently from 667b9ad to d22eecc Compare November 13, 2024 15:56

yhmtsai mentioned this pull request Nov 13, 2024

mingw and clang error #1654

Closed

yhmtsai added this to the Ginkgo 1.9.0 milestone Nov 15, 2024

yhmtsai requested a review from a team November 18, 2024 10:28

MarcelKoch requested changes Nov 18, 2024

View reviewed changes

yhmtsai force-pushed the batch_dynamic_cast branch from 3408b2b to c14d27b Compare November 21, 2024 14:57

yhmtsai requested a review from MarcelKoch November 21, 2024 15:16

MarcelKoch requested changes Nov 21, 2024

View reviewed changes

yhmtsai force-pushed the batch_dynamic_cast branch from c645d8e to 2fb4434 Compare November 21, 2024 17:26

MarcelKoch approved these changes Nov 21, 2024

View reviewed changes

upsj approved these changes Nov 21, 2024

View reviewed changes

dpcpp/solver/batch_bicgstab_kernels.dp.cpp Outdated Show resolved Hide resolved

yhmtsai force-pushed the batch_dynamic_cast branch 3 times, most recently from 20b7e8b to 0b7409e Compare November 21, 2024 22:42

yhmtsai added 1:ST:ready-to-merge This PR is ready to merge. and removed 1:ST:ready-for-review This PR is ready for review labels Nov 21, 2024

yhmtsai and others added 9 commits November 22, 2024 00:21

fix test solve_lambda

0aefbcd

msys

69dce68

also seperate config to another library in mingw (including msys2/clang)

bff7569

use -fno-assume-unique-vtables to make dyanmic_cast to the final clas…

84c8531

…s work

reuse the macro from another pr

ed46a66

Co-authored-by: Marcel Koch <[email protected]>

make the macro stack clear

862fd12

add GKO_INDIRECT to properly expand the __VA_ARGS__ in msvc

0e15a0f

no way to face nullptr preconditioner in batch apply

2dfc7d6

Co-authored-by: Marcel Koch <[email protected]>

yhmtsai force-pushed the batch_dynamic_cast branch from 0b7409e to 2dfc7d6 Compare November 21, 2024 23:21

Revert "msys"

1bff73b

yhmtsai merged commit 681caa0 into develop Nov 22, 2024
13 of 14 checks passed

yhmtsai deleted the batch_dynamic_cast branch November 22, 2024 12:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix dynamic cast leads the undefined symbol in clang/msys2/windows #1724

Fix dynamic cast leads the undefined symbol in clang/msys2/windows #1724

yhmtsai commented Nov 12, 2024 •

edited

Loading

MarcelKoch Nov 18, 2024

yhmtsai Nov 18, 2024

yhmtsai Nov 21, 2024

MarcelKoch left a comment

MarcelKoch Nov 21, 2024

yhmtsai Nov 21, 2024

MarcelKoch Nov 21, 2024

yhmtsai Nov 21, 2024

upsj left a comment

sonarqubecloud bot commented Nov 22, 2024

codecov bot commented Nov 22, 2024 •

edited

Loading

Fix dynamic cast leads the undefined symbol in clang/msys2/windows #1724

Fix dynamic cast leads the undefined symbol in clang/msys2/windows #1724

Conversation

yhmtsai commented Nov 12, 2024 • edited Loading

MarcelKoch Nov 18, 2024

Choose a reason for hiding this comment

yhmtsai Nov 18, 2024

Choose a reason for hiding this comment

yhmtsai Nov 21, 2024

Choose a reason for hiding this comment

MarcelKoch left a comment

Choose a reason for hiding this comment

MarcelKoch Nov 21, 2024

Choose a reason for hiding this comment

yhmtsai Nov 21, 2024

Choose a reason for hiding this comment

MarcelKoch Nov 21, 2024

Choose a reason for hiding this comment

yhmtsai Nov 21, 2024

Choose a reason for hiding this comment

upsj left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Nov 22, 2024

Quality Gate failed

codecov bot commented Nov 22, 2024 • edited Loading

Codecov Report

yhmtsai commented Nov 12, 2024 •

edited

Loading

codecov bot commented Nov 22, 2024 •

edited

Loading