ecmwf / fckit

A Fortran toolkit for interoperating Fortran with C/C++
https://confluence.ecmwf.int/display/fckit
Apache License 2.0
29 stars 15 forks source link

Problem building tests when using a compiler toolchain #18

Closed climbfuji closed 2 years ago

climbfuji commented 2 years ago

I believe I found a bug in the build system that manifests itself only when using a compiler toolchain (e.g. a newer libstdc++ with the Intel compilers) for certain tests that include both Fortran and C++ code. For example in src/tests/CMakeLists.txt:

add_fctest( TARGET  fckit_test_shared_ptr
            LINKER_LANGUAGE Fortran
            SOURCES
              test_shared_ptr.F90
              test_shared_ptr.cc
            LIBS    fckit)

When using a compiler toolchain on systems where the default gcc is too old, e.g. when adding -gxx-name=/path/to/a/newer/gcc/bin/g++, I get errors of undefined symbols from a newer GLIBCXX standard. That's because test_shared_ptr.cc is compiled with the toolchain, but the Fortran linker doesn't know about it.

An easy workaround when just building fckit is to set the cmake linker flags as follows:

-gxx-name=/path/to/a/newer/gcc/bin/g++ -Wl,-rpath,/path/to/a/newer/gcc/lib64 -lstdc++

However, one can't do this when building fckit as part of a software stack using a package manager like spack. And it is only putting duct tape over the problem. I think the correct solution is to explicitly link against libstdc++ for those tests that use LINKER_LANGUAGE Fortran and C++ sources:

diff --git a/src/tests/CMakeLists.txt b/src/tests/CMakeLists.txt
index 248ac67..150d574 100644
--- a/src/tests/CMakeLists.txt
+++ b/src/tests/CMakeLists.txt
@@ -31,7 +31,7 @@ add_fctest( TARGET  fckit_test_shared_ptr
             SOURCES
               test_shared_ptr.F90
               test_shared_ptr.cc
-            LIBS    fckit)
+            LIBS    fckit stdc++)

 add_fctest( TARGET  fckit_test_mpi
             LINKER_LANGUAGE Fortran
@@ -88,7 +88,7 @@ ecbuild_add_test( TARGET  fckit_test_configuration_cpp
             test_configuration_fortcode.F90
             INCLUDES ${ECKIT_INCLUDE_DIRS}
             CONDITION HAVE_ECKIT
-            LIBS    fckit  eckit eckit_mpi )
+            LIBS    fckit  eckit eckit_mpi stdc++)

 if( TEST fckit_test_configuration_cpp )
   set_property( TEST fckit_test_configuration_cpp APPEND PROPERTY LABELS "fortran" )
@@ -100,7 +100,7 @@ ecbuild_add_test( TARGET fckit_test_cpp
   INCLUDES ${ECKIT_INCLUDE_DIRS}
   ENVIRONMENT "DEBUG=1"
   CONDITION HAVE_ECKIT
-  LIBS fckit eckit eckit_mpi )
+  LIBS fckit eckit eckit_mpi stdc++)

 ### Fix linking for C++ test executables linked with Fortran linker
 if( CMAKE_Fortran_COMPILER_ID MATCHES "PGI|NVHPC" )
climbfuji commented 2 years ago

Just checking if the issues in this repository are watched?

tlmquintino commented 2 years ago

yes they are. Appologies for missing it earlier on. maybe @wdeconinck can have a look?

climbfuji commented 2 years ago

Thanks, @tlmquintino! FYI, we have been using the changes shown above in our spack-based stack installations on a wide range of systems, and have not had any problems so far.

wdeconinck commented 2 years ago

Dear @climbfuji I don't know if this is really an appropriate solution, to explicitly link to the GNU C++ library (libstdc++). There's possible conflict with the LLVM C++ library (libc++) and I am not 100% sure what other compilers are using.

Normally CMake should be capable to understand that C++ is used and when the Fortran compiler is used to link, it should add the correct c++ library.

I recognise that In certain cases this could pose a problem, and indeed we also encountered something similar (but not quite the same) like this with the Cray compiler and had to add something to our toolchain.

You could try adding following to your toolchain (or CMake command line).

set( ECBUILD_IMPLICIT_LINK_LIBRARIES stdc++ )

which will add these libraries to each "ecbuild_add_library" or "ecbuild_add_executable".

I usually found that just setting the C++ specific variable could be sufficient when building eckit

set( ECBUILD_CXX_IMPLICIT_LINK_LIBRARIES stdc++ )

as that will then be used to link "eckit" and would be propagated to any Fortran library that links with eckit.

climbfuji commented 2 years ago

Dear @climbfuji I don't know if this is really an appropriate solution, to explicitly link to the GNU C++ library (libstdc++). There's possible conflict with the LLVM C++ library (libc++) and I am not 100% sure what other compilers are using.

Normally CMake should be capable to understand that C++ is used and when the Fortran compiler is used to link, it should add the correct c++ library.

I recognise that In certain cases this could pose a problem, and indeed we also encountered something similar (but not quite the same) like this with the Cray compiler and had to add something to our toolchain.

You could try adding following to your toolchain (or CMake command line).

set( ECBUILD_IMPLICIT_LINK_LIBRARIES stdc++ )

which will add these libraries to each "ecbuild_add_library" or "ecbuild_add_executable".

I usually found that just setting the C++ specific variable could be sufficient when building eckit

set( ECBUILD_CXX_IMPLICIT_LINK_LIBRARIES stdc++ )

as that will then be used to link "eckit" and would be propagated to any Fortran library that links with eckit.

@wdeconinck Thank you, that is a good suggestion, will try. Intel uses libstdc++ as backend; LLVM clang as you said has its own libc++; Apple clang uses libstdc++. We don't use any other compilers than Intel, gcc, llvm clang, apple clang at the moment.

climbfuji commented 2 years ago

@wdeconinck This worked like a charm. I used the following logic in spack:

+        if self.spec.satisfies('%intel') or self.spec.satisfies('%gcc'):
+            cxxlib = 'stdc++'
+        elif self.spec.satisfies('%clang') or self.spec.satisfies('%apple-clang'):
+            cxxlib = 'c++'
+        else:
+            raise InstallError("C++ library not configured for compiler")
+        res.append('-DECBUILD_CXX_IMPLICIT_LINK_LIBRARIES={}'.format(cxxlib))

Guess we can close this PR as no longer needed. Thanks a lot for your help.