[OpenMP] Use C++ to link libomp.so when LLVM libraries are included

[kparzysz]

Summary

Otherwise cmake may link it as a C library causing dozens of C++ symbols to be unresolved.

[JonChesterfield]

Tagging Shilei as this is cmake

Google suggests 'LINKER_LANGUAGE' is how cmake decides whether to invoke 'gcc' or 'g++' to build a library, as an indirect way of asking to link in libstdc++ or similar.

So my guess is that the llvm support library has link time dependencies on c++, so when linking support we also need to link a c++ runtime, and CMake chose 'LINKER_LANGUAGE' as the name to indicate that.

Does that match your understanding of the problem? If so, agreed with the fix.

[tianshilei1992]

In D95371#2520241, @JonChesterfield wrote:

Tagging Shilei as this is cmake

Google suggests 'LINKER_LANGUAGE' is how cmake decides whether to invoke 'gcc' or 'g++' to build a library, as an indirect way of asking to link in libstdc++ or similar.

So my guess is that the llvm support library has link time dependencies on c++, so when linking support we also need to link a c++ runtime, and CMake chose 'LINKER_LANGUAGE' as the name to indicate that.

Does that match your understanding of the problem? If so, agreed with the fix.

I think your guess is right. libomp.so is probably intended to be a C library (need to add @AndreyChurbanov here as he is the real expert). Not sure whether it's good to potentially break the rule.

[kparzysz]

The problem is with linking the Support library from LLVM, which has a lot of C++ code. If I'm reading the CMakeLists.txt correctly, there is still an option to build libomp.so as a "standalone" library (line 138), where the C linking should be sufficient.

In my build there are over 2800 lines of errors due to various C++ symbols. Here's a sample:

../../../../lib/libLLVMSupport.a(TimeProfiler.cpp.o): In function `std::__1::vector<llvm::TimeTraceProfiler*, std::__1::allocator<llvm::TimeTraceProfiler*> >::~vector()':
TimeProfiler.cpp:(.text._ZNSt3__16vectorIPN4llvm17TimeTraceProfilerENS_9allocatorIS3_EEED2Ev[_ZNSt3__16vectorIPN4llvm17TimeTraceProfilerENS_9allocatorIS3_EEED2Ev]+0x10): undefined reference to `operator delete(void*)'
../../../../lib/libLLVMSupport.a(TimeProfiler.cpp.o): In function `llvm::timeTraceProfilerInitialize(unsigned int, llvm::StringRef)':
TimeProfiler.cpp:(.text._ZN4llvm27timeTraceProfilerInitializeEjNS_9StringRefE+0x30): undefined reference to `operator new(unsigned long)'
../../../../lib/libLLVMSupport.a(TimeProfiler.cpp.o): In function `llvm::TimeTraceProfiler::TimeTraceProfiler(unsigned int, llvm::StringRef)':
TimeProfiler.cpp:(.text._ZN4llvm17TimeTraceProfilerC2EjNS_9StringRefE[_ZN4llvm17TimeTraceProfilerC2EjNS_9StringRefE]+0x67): undefined reference to `std::__1::chrono::system_clock::now()'
TimeProfiler.cpp:(.text._ZN4llvm17TimeTraceProfilerC2EjNS_9StringRefE[_ZN4llvm17TimeTraceProfilerC2EjNS_9StringRefE]+0x73): undefined reference to `std::__1::chrono::steady_clock::now()'
TimeProfiler.cpp:(.text._ZN4llvm17TimeTraceProfilerC2EjNS_9StringRefE[_ZN4llvm17TimeTraceProfilerC2EjNS_9StringRefE]+0xca): undefined reference to `operator new(unsigned long)'
TimeProfiler.cpp:(.text._ZN4llvm17TimeTraceProfilerC2EjNS_9StringRefE[_ZN4llvm17TimeTraceProfilerC2EjNS_9StringRefE]+0x155): undefined reference to `std::__1::__basic_string_common<true>::__throw_length_error() const'
../../../../lib/libLLVMSupport.a(TimeProfiler.cpp.o): In function `llvm::timeTraceProfilerCleanup()':

[AndreyChurbanov]

In D95371#2520436, @tianshilei1992 wrote:

In D95371#2520241, @JonChesterfield wrote:

Tagging Shilei as this is cmake

Google suggests 'LINKER_LANGUAGE' is how cmake decides whether to invoke 'gcc' or 'g++' to build a library, as an indirect way of asking to link in libstdc++ or similar.

So my guess is that the llvm support library has link time dependencies on c++, so when linking support we also need to link a c++ runtime, and CMake chose 'LINKER_LANGUAGE' as the name to indicate that.

Does that match your understanding of the problem? If so, agreed with the fix.

I think your guess is right. libomp.so is probably intended to be a C library (need to add @AndreyChurbanov here as he is the real expert). Not sure whether it's good to potentially break the rule.

That's right. Regardless that the code is written in C++ the intention is to not have any dependency on C++ runtime. The libomp.so should work fine with C applications as well as with Fortran codes those often cannot link C++ runtime.
And all exported from libomp.so symbols supposed to be C symbols.

[jdoerfert]

And all exported from libomp.so symbols supposed to be C symbols.

This makes sense, but that should be the case.

That's right. Regardless that the code is written in C++ the intention is to not have any dependency on C++ runtime. The libomp.so should work fine with C applications as well as with Fortran codes those often cannot link C++ runtime.

Right. With profile support we got C++ dependences. So if you disable the profile support you can remove the dependence. What if we set the linker language to CXX if the profile support is enabled? Is that a problem?

[AndreyChurbanov]

That's right. Regardless that the code is written in C++ the intention is to not have any dependency on C++ runtime. The libomp.so should work fine with C applications as well as with Fortran codes those often cannot link C++ runtime.

Right. With profile support we got C++ dependences. So if you disable the profile support you can remove the dependence. What if we set the linker language to CXX if the profile support is enabled? Is that a problem?

But if profile support is enabled by default and will got into LLVM release, then people with pure C codes of Fortran codes will be surprised to see their OpenMP application depends on C++ runtime.
E.g. if they provide Fortran redistributables with Fortran application, it won't work because of C++ runtime dependency.
Or somebody may setup flang without clang, or gfortran without gcc, and link with libomp.
Not sure it is immediate problem, but in length of time such things will happen.

[jdoerfert]

In D95371#2520747, @AndreyChurbanov wrote:

That's right. Regardless that the code is written in C++ the intention is to not have any dependency on C++ runtime. The libomp.so should work fine with C applications as well as with Fortran codes those often cannot link C++ runtime.

Right. With profile support we got C++ dependences. So if you disable the profile support you can remove the dependence. What if we set the linker language to CXX if the profile support is enabled? Is that a problem?

But if profile support is enabled by default and will got into LLVM release, then people with pure C codes of Fortran codes will be surprised to see their OpenMP application depends on C++ runtime.
E.g. if they provide Fortran redistributables with Fortran application, it won't work because of C++ runtime dependency.
Or somebody may setup flang without clang, or gfortran without gcc, and link with libomp.
Not sure it is immediate problem, but in length of time such things will happen.

This is a conceptual argument though. What is more likely/beneficial and where do we require people to compile their own libomp. Arguably, profiling alone is not the most critical feature but from a practical perspective I think C and Fortran apps without C++ available is "not necessarily the future". If there are users that want to, they can still build libomp.so w/o C++ dependence. That said, we could debate if that is worth it as we could then utilize the stdlib more broadly, which is arguably a good thing.

FWIW, even if we decide to flip profiling off after the release branch split we can still back port it as a fix. For now, I'd argue for set(LIBOMP_LINKER_LANGUAGE CXX) guarded by the profiler flag.

Final thought: It might even be not too hard to compile two libraries, libomp.so and libomp.C.so, such that the former can have C++ features, maybe even a C++ interface for certain things.

[kparzysz]

It's already guarded---it's only set to CXX when linking with the Support library.

[ggeorgakoudis]

Will this change be accepted? This fix is irrespective of whether we decide to enable time profiling by default or not.

[kparzysz]

This is fixed in https://reviews.llvm.org/D95585.

openmp/runtime/src/CMakeLists.txt

if(OPENMP_STANDALONE_BUILD OR (NOT OPENMP_ENABLE_LIBOMPTARGET_PROFILING))
   add_library(omp ${LIBOMP_LIBRARY_KIND} ${LIBOMP_SOURCE_FILES})
   # Linking command will include libraries in LIBOMP_CONFIGURED_LIBFLAGS
   target_link_libraries(omp ${LIBOMP_CONFIGURED_LIBFLAGS} ${CMAKE_DL_LIBS})
 else()
   add_llvm_library(omp ${LIBOMP_LIBRARY_KIND} ${LIBOMP_SOURCE_FILES} PARTIAL_SOURCES_INTENDED
     LINK_LIBS ${LIBOMP_CONFIGURED_LIBFLAGS} ${CMAKE_DL_LIBS}
     LINK_COMPONENTS Support
     )
+  set(LIBOMP_LINKER_LANGUAGE CXX)
 endif()
 
 set_target_properties(omp PROPERTIES
   PREFIX "" SUFFIX "" OUTPUT_NAME "${LIBOMP_LIB_FILE}"
   LINK_FLAGS "${LIBOMP_CONFIGURED_LDFLAGS}"
   LINKER_LANGUAGE ${LIBOMP_LINKER_LANGUAGE}
 )