Page MenuHomePhabricator

[flang] Install Fortran_main library
ClosedPublic

Authored by rovka on May 2 2022, 4:48 AM.

Details

Summary

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.

Diff Detail

Event Timeline

rovka created this revision.May 2 2022, 4:48 AM
Herald added a project: Restricted Project. · View Herald Transcript
rovka requested review of this revision.May 2 2022, 4:48 AM
schweitz accepted this revision.May 10 2022, 1:20 PM
This revision is now accepted and ready to land.May 10 2022, 1:20 PM
dpalermo requested changes to this revision.May 10 2022, 2:08 PM
dpalermo added a subscriber: dpalermo.

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=17
This revision now requires changes to proceed.May 10 2022, 2:08 PM

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)
rovka updated this revision to Diff 428603.May 11 2022, 3:11 AM
rovka edited the summary of this revision. (Show Details)

@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.

dpalermo accepted this revision.May 11 2022, 7:35 AM
dpalermo added a subscriber: awarzynski.

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.

This revision is now accepted and ready to land.May 11 2022, 7:35 AM

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!

rovka added a comment.May 12 2022, 5:16 AM

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.

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.

dpalermo requested changes to this revision.May 12 2022, 2:58 PM

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
 )
This revision now requires changes to proceed.May 12 2022, 2:58 PM
rovka updated this revision to Diff 429163.May 13 2022, 1:36 AM
rovka edited the summary of this revision. (Show Details)

Fixed. Thanks @dpalermo for investigating so promptly! :)

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! 🎉 !

dpalermo accepted this revision.May 13 2022, 7:38 AM

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!

This revision is now accepted and ready to land.May 13 2022, 7:38 AM
awarzynski accepted this revision.May 13 2022, 8:48 AM

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 :)

This revision was landed with ongoing or failed builds.May 16 2022, 1:32 AM
This revision was automatically updated to reflect the committed changes.