Skip to content

[flang-rt] Use --as-needed for linking flang-rt libraries. #130856

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 14, 2025

Conversation

vzakhari
Copy link
Contributor

This change makes sure that there is no unnecessary library
dependencies like libc++/libstdc++.

This change makes sure that there is no unnecessary library
dependencies like libc++/libstdc++.
@vzakhari vzakhari requested review from Meinersbur and klausler March 11, 2025 22:35
@Meinersbur
Copy link
Member

I don't it does what you think it does. The static library is just a collection of object files. Only when linking the executable you actually know what "is needed". Until then, you want all the object files in the library.

We actually have a test that checks that no libc++ dependencies are required: https://github.com/llvm/llvm-project/blob/main/flang-rt/test/Runtime/no-cpp-dep.c

@vzakhari
Copy link
Contributor Author

I don't it does what you think it does. The static library is just a collection of object files. Only when linking the executable you actually know what "is needed". Until then, you want all the object files in the library.

We actually have a test that checks that no libc++ dependencies are required: https://github.com/llvm/llvm-project/blob/main/flang-rt/test/Runtime/no-cpp-dep.c

It works for the shared libraries build, I checked.

I know that we have the test, but the library dependencies are still established by the linker, and they need to be satisfied. Which makes it awkward that we need to link c++ library for Fortran programs.

@Meinersbur
Copy link
Member

It works for the shared libraries build, I checked.

I think thats what set_target_properties(${tgtname} PROPERTIES LINKER_LANGUAGE C) already does, which just does not add -lc++, respectively -lstdc++. What other unnecessary libraries are there?

I know that we have the test, but the library dependencies are still established by the linker, and they need to be satisfied. Which makes it awkward that we need to link c++ library for Fortran programs.

If this is for the shared library, please only apply it to the shared libary. This can be done by moving it into the build_shared section.

--as-needed is a positional option: It only applies to libraries listed after it. I don't know whether CMake/gcc/clang guarantee that LINK_FLAGS will be inserted before all the libraries. I'd avoid it if not necessary.

@Meinersbur
Copy link
Member

At least, could you add to the comment that this is for the shared library only? For the static library, the linker is not even used (CMAKE_AR is being used) and LINKER_LANGUAGE/LINK_FLAGS being ignored.

@vzakhari
Copy link
Contributor Author

It works for the shared libraries build, I checked.

I think thats what set_target_properties(${tgtname} PROPERTIES LINKER_LANGUAGE C) already does, which just does not add -lc++, respectively -lstdc++. What other unnecessary libraries are there?

Unfortunately, it is not enough. See the comment about CMAKE_CXX_IMPLICIT_LINK_LIBRARIES: the C++ library is added into this var automatically by CMake. I did not want to mess with this var, but that is another solution, i.e. to remove c++ and stdc++ from it. I think --as-needed is a more clean solution, and it will cover other libs, e.g. I believe I saw pthreads being linked, while it was not needed.

If this is for the shared library, please only apply it to the shared libary. This can be done by moving it into the build_shared section.

--as-needed is a positional option: It only applies to libraries listed after it. I don't know whether CMake/gcc/clang guarantee that LINK_FLAGS will be inserted before all the libraries. I'd avoid it if not necessary.

Thanks! I will move the code around. Good point about the position of the option: it works now, but I will check if I can do it more robust.

Copy link
Member

@Meinersbur Meinersbur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vzakhari vzakhari merged commit eeb2733 into llvm:main Mar 14, 2025
9 checks passed
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.

3 participants