At the moment the Fortran_main library is not installed, so it cannot be
found by the driver when run from an install directory. This patch fixes
the issue by replacing llvm_add_library with add_flang_library, which
already contains all the proper incantations for installing a library.
It also enhances add_flang_library to support a STATIC arg which forces
the library to be static even when BUILD_SHARED_LIBS is on.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
I just tried this patch but it seems to fail when it tries to build a shared libFortran_main.so:
[31/34] Linking C shared library lib/libFortran_main.so.15git FAILED: lib/libFortran_main.so.15git : && /usr/bin/cc -fPIC -fPIC -fno-semantic-interposition -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-comment -Wmisleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG -Wl,-z,defs -Wl,-z,nodelete -Wl,-rpath-link,/home/dpalermo/git/trunk/llvm-project/build/./lib -Wl,--gc-sections -shared -Wl,-soname,libFortran_main.so.15git -o lib/libFortran_main.so.15git tools/flang/runtime/FortranMain/CMakeFiles/obj.Fortran_main.dir/Fortran_main.c.o -Wl,-rpath,"\$ORIGIN/../lib" && : /usr/bin/ld: tools/flang/runtime/FortranMain/CMakeFiles/obj.Fortran_main.dir/Fortran_main.c.o: in function `main': Fortran_main.c:(.text.startup.main+0x9): undefined reference to `_FortranAProgramStart' /usr/bin/ld: Fortran_main.c:(.text.startup.main+0x10): undefined reference to `_QQmain' /usr/bin/ld: Fortran_main.c:(.text.startup.main+0x15): undefined reference to `_FortranAProgramEndStatement' collect2: error: ld returned 1 exit status
This is the cmake I am using:
cmake ../llvm -G 'Ninja' \
-DCMAKE_INSTALL_PREFIX=$TOP/install/ \
-DCMAKE_BUILD_TYPE=Release \
-DLLVM_ENABLE_ASSERTIONS=ON \
-DLLVM_ENABLE_PROJECTS="clang;mlir;openmp;flang" \
-DLLVM_TARGETS_TO_BUILD="X86;AMDGPU" \
-DBUILD_SHARED_LIBS=ON \
-DLLVM_INSTALL_UTILS=On \
-DCMAKE_CXX_STANDARD=17Comment Actions
If the install logic in add_flang_library is moved into a separate macro (to avoid the "SHARED STATIC"), this problem is avoided and only the archive version of Fortran_main is built/installed:
diff --git a/flang/cmake/modules/AddFlang.cmake b/flang/cmake/modules/AddFlang.cmake index 369e303e148a..4426f8628bf0 100644 --- a/flang/cmake/modules/AddFlang.cmake +++ b/flang/cmake/modules/AddFlang.cmake @@ -16,6 +16,27 @@ macro(add_flang_subdirectory name) add_llvm_subdirectory(FLANG TOOL ${name}) endmacro() +macro(install_flang_library name) + if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY OR ${name} STREQUAL "libflang") + get_target_export_arg(${name} Flang export_to_flangtargets UMBRELLA flang-libraries) + install(TARGETS ${name} + COMPONENT ${name} + ${export_to_flangtargets} + LIBRARY DESTINATION lib${LLVM_LIBDIR_SUFFIX} + ARCHIVE DESTINATION lib${LLVM_LIBDIR_SUFFIX} + RUNTIME DESTINATION "${CMAKE_INSTALL_BINDIR}") + + if (NOT LLVM_ENABLE_IDE) + add_llvm_install_targets(install-${name} + DEPENDS ${name} + COMPONENT ${name}) + endif() + + set_property(GLOBAL APPEND PROPERTY FLANG_LIBS ${name}) + endif() + set_property(GLOBAL APPEND PROPERTY FLANG_EXPORTS ${name}) +endmacro() + macro(add_flang_library name) cmake_parse_arguments(ARG "SHARED" @@ -64,25 +85,7 @@ macro(add_flang_library name) llvm_add_library(${name} ${LIBTYPE} ${ARG_UNPARSED_ARGUMENTS} ${srcs}) if (TARGET ${name}) - - if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY OR ${name} STREQUAL "libflang") - get_target_export_arg(${name} Flang export_to_flangtargets UMBRELLA flang-libraries) - install(TARGETS ${name} - COMPONENT ${name} - ${export_to_flangtargets} - LIBRARY DESTINATION lib${LLVM_LIBDIR_SUFFIX} - ARCHIVE DESTINATION lib${LLVM_LIBDIR_SUFFIX} - RUNTIME DESTINATION "${CMAKE_INSTALL_BINDIR}") - - if (NOT LLVM_ENABLE_IDE) - add_llvm_install_targets(install-${name} - DEPENDS ${name} - COMPONENT ${name}) - endif() - - set_property(GLOBAL APPEND PROPERTY FLANG_LIBS ${name}) - endif() - set_property(GLOBAL APPEND PROPERTY FLANG_EXPORTS ${name}) + install_flang_library(${name}) else() # Add empty "phony" target add_custom_target(${name}) diff --git a/flang/runtime/FortranMain/CMakeLists.txt b/flang/runtime/FortranMain/CMakeLists.txt index aa214cee31ff..2167e3fc918b 100644 --- a/flang/runtime/FortranMain/CMakeLists.txt +++ b/flang/runtime/FortranMain/CMakeLists.txt @@ -1,3 +1,5 @@ llvm_add_library(Fortran_main STATIC Fortran_main.c ) + +install_flang_library(Fortran_main)
Comment Actions
@dpalermo Thanks for trying it out! I've updated the review with your patch. I guess it's ok to treat Fortran_main differently from the other libs, and anyway it's difficult for me to test that all the other things in add_flang_library actually work as expected for it. So it seems like a good idea to split out just the part dealing with the install. Would you like to commit this yourself? If not, I can just add you as author and commit it.
Comment Actions
Thanks for the quick response. I've verified that this revision now builds flang successfully for me. I am fine with you committing this. I'll add @awarzynski as a reviewer (since this is related to his https://reviews.llvm.org/rG97a32d3e43fe) in case he has any other suggestions.
Comment Actions
Hi @rovka & @dpalermo, thanks for working on this!
I am in favor of this change, but would much prefer the original approach from this patch, which is in line with how e.g. LLVM or Clang implement add_llvm_library and add_clang_library, respectively.
I think that it would be good for the overall health of project to keep CMake modules semi-consistent across sub-projects. If we do diverge, that I'd like to make sure that that's unavoidable :) In particular, the error message from one of the earlier comments suggests build issue rather than an installation issue. I would hope that that can be solved without creating a dedicated macro for installing a library.
I'm traveling today, and for testing Flang I need stable SSH. Once I have access to a better connection, I can poke around - that's in the next few days. Would you mind waiting or, alternatively, having another go yourself ?
Thanks again for this work!
Comment Actions
Well, the main problem there is that it was trying to build it as a shared library instead of a static one, although I had passed STATIC to add_flang_library. To be honest I didn't investigate why it still thought it should build it as shared, because @dpalermo 's patch works fine and imo the Fortran_main lib is sufficiently different from all the others that we can afford to take a different path with it. Note that the other libs would still use add_flang_library with either of the 2 patches, so we wouldn't be diverging that much.
I'm traveling today, and for testing Flang I need stable SSH. Once I have access to a better connection, I can poke around - that's in the next few days. Would you mind waiting or, alternatively, having another go yourself ?
Unfortunately I don't have time to look into it more this week either. I'm not going to commit it yet, and we can revisit next week.
Comment Actions
I went back to the original patch, added some more debugging, and found that the options in cmake_parse_arguments had a space instead of a semicolon so ARG_STATIC was never being set.
The changes below (original patch + semicolon instead of space) also now work for me:
diff --git a/flang/cmake/modules/AddFlang.cmake b/flang/cmake/modules/AddFlang.cmake index 369e303e148a..f6fc2ac363fc 100644 --- a/flang/cmake/modules/AddFlang.cmake +++ b/flang/cmake/modules/AddFlang.cmake @@ -18,7 +18,7 @@ endmacro() macro(add_flang_library name) cmake_parse_arguments(ARG - "SHARED" + "SHARED;STATIC" "" "ADDITIONAL_HEADERS" ${ARGN}) @@ -53,7 +53,7 @@ macro(add_flang_library name) else() # llvm_add_library ignores BUILD_SHARED_LIBS if STATIC is explicitly set, # so we need to handle it here. - if (BUILD_SHARED_LIBS) + if (BUILD_SHARED_LIBS AND NOT ARG_STATIC) set(LIBTYPE SHARED OBJECT) else() set(LIBTYPE STATIC OBJECT) diff --git a/flang/runtime/FortranMain/CMakeLists.txt b/flang/runtime/FortranMain/CMakeLists.txt index aa214cee31ff..1d840ee5d1a3 100644 --- a/flang/runtime/FortranMain/CMakeLists.txt +++ b/flang/runtime/FortranMain/CMakeLists.txt @@ -1,3 +1,3 @@ -llvm_add_library(Fortran_main STATIC +add_flang_library(Fortran_main STATIC Fortran_main.c )
Comment Actions
Thank you @dpalermo , you are a ⭐ ! This way we can have a much less intrusive solution, which is fantastic :)
Note that the other libs would still use add_flang_library with either of the 2 patches, so we wouldn't be diverging that much.
Yes, but I was referring to other sub-projects and the semantics of similar macros there. By creating install_flang_library, the semantics start to diverge. In particular, how would people know whether to use install_flang_library or not?
In any case, sounds like this is sorted now - great team effort! 🎉 !
Comment Actions
Thank you @rovka for the original/cleaner approach and @awarzynski for your encouragement to stick with it. Also thank you for your previous patch for adding support to flang-new for generating executables. This makes running new codes through flang-new much easier!
Comment Actions
Thank you both!
Also thank you for your previous patch for adding support to flang-new for generating executables.
You are most welcome, It's very rewarding to see that people find it helpful :)