Skip to content

[Core][Apps] Making visibility hidden by default #13060

Open
roigcarlo wants to merge 84 commits intomasterfrom
core/the-purge
Open

[Core][Apps] Making visibility hidden by default #13060
roigcarlo wants to merge 84 commits intomasterfrom
core/the-purge

Conversation

@roigcarlo
Copy link
Copy Markdown
Member

@roigcarlo roigcarlo commented Jan 29, 2025

📝 Description
This PR aims to unify the windows and Linux behavior making all symbols in Linux (gcc and llvm based compilers) hidden by default. This will probably help with linking times and linking restrictions (see llvm in win refusing to link targets with more than 65K symbols)

Since exporting template classes with definitions in sources works a bit different (finally i figured out how to make them visible!), some additional decorators will need to be added to several explicit instantiations.

Comments:

  • Changes in find_global_nodal_neighbours_for_entities_process.cpp are not strictly necessary, but is better this way (imo).
  • @KratosMultiphysics/dem DEM application was failing due to a forward declaration of the SphereElement. Please take a look (0e8e547)
  • @KratosMultiphysics/mpi There was an error in the AMGCSolve function as the header was never being used. I made it so it uses the header declaration and the source definition instead of using the definition as a header in the source. Please confirm this was the intended behavior.
  • @AlejandroCornejo CL is mostly yours check everything needed is exposed, I really don't know what is used from outside so I treid to make everything visible but maybe I am missing something used in other apps I don't have access to.

@loumalouomega
Copy link
Copy Markdown
Member

You don't need to kodify KRATOS_API for non MSVC ?

@matekelemen
Copy link
Copy Markdown
Contributor

Oh yes thank you! I got back into compile-heavy development again and was about to tear my nonexistent hair out during linking.

@loumalouomega
Copy link
Copy Markdown
Member

Almost compiled, just Metis is failing apparently

@roigcarlo
Copy link
Copy Markdown
Member Author

KRATOS_API

Should work as it is rn, I foresaw this when I did the Kratos api, but if some application gets grumpy I will touch it.

@roigcarlo roigcarlo marked this pull request as ready for review January 30, 2025 11:54
@roigcarlo roigcarlo requested review from a team as code owners January 30, 2025 11:54
@roigcarlo roigcarlo requested a review from a team as a code owner January 30, 2025 14:30
@roigcarlo
Copy link
Copy Markdown
Member Author

It's alive

@loumalouomega
Copy link
Copy Markdown
Member

@matekelemen I am trying to compile with Intel LLVM in Windows and your multigrid assemblers are giving issues

@matekelemen
Copy link
Copy Markdown
Contributor

@matekelemen I am trying to compile with Intel LLVM in Windows and your multigrid assemblers are giving issues

They're fine on master. I don't know why this PR would trigger an ICE in intel's wonderful compiler but as a general rule, I don't think we should play around defective compilers.

@rfaasse
Copy link
Copy Markdown
Contributor

rfaasse commented Apr 2, 2026

To chip in here, we use Clang-LLVM (with llvm-cov) on windows for generating our coverage report in our pipelines. Since this PR seems to be close to merging, I tested it in our pipelines and for the clang-windows build (core + linear solvers + structural + geo apps) we still see some linker errors, for example (certainly not a complete list):

  • lld-link: error: undefined symbol: public: static class Kratos::DataCommunicator & __cdecl Kratos::ParallelEnvironment::GetDefaultDataCommunicator(void)
  • lld-link: error: undefined symbol: public: __cdecl Kratos::Exception::Exception(class std::basic_string<char, struct std::char_traits, class std::allocator> const &, class Kratos::CodeLocation const &)
  • lld-link: error: undefined symbol: class Kratos::Variable Kratos::TEMPERATURE

For this particular build, we use Clang 19.1.0 with MSVC-like command-line. Is this expected @roigcarlo?

@loumalouomega
Copy link
Copy Markdown
Member

@matekelemen I am trying to compile with Intel LLVM in Windows and your multigrid assemblers are giving issues

@matekelemen I am trying to compile with Intel LLVM in Windows and your multigrid assemblers are giving issues

They're fine on master. I don't know why this PR would trigger an ICE in intel's wonderful compiler but as a general rule, I don't think we should play around defective compilers.

Well, is not just Intel LLVM, Clang for Windows is also failing. And the reason this PR exists is making work any of those 2 compilers because MSVC is terrible kin providing properly optimized code. We have tests in internal projects that just compiling with any of those compilers instead of MSVC improves performance 20-25% out of the box. Any case we can fix compilation in independent PR.

@loumalouomega
Copy link
Copy Markdown
Member

I need to check internally this to see if affects us.

@loumalouomega
Copy link
Copy Markdown
Member

I cherry-picked some fixes I did in #14346, anyway I thought that maybe fix the compilation in LLVM compilers can be done in a later PR.

@loumalouomega
Copy link
Copy Markdown
Member

loumalouomega commented Apr 3, 2026

Fantastic, I just broke Windows, some symbols were missing to compile locally in Ubuntu. Well at least is the last 2 commits, so easy to revert o to track

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hide all symbols by default [Discussion] Uniform Export Policy?

4 participants