Skip to content

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