Fix gettid warnings and one test on FreeBSD

[dim]

Summary

While building the 8.0 releases on FreeBSD, I encountered the following
warnings in openmp quite a few times:

In file included from projects/openmp/runtime/src/kmp_settings.cpp:27:
projects/openmp/runtime/src/kmp_wrapper_getpid.h:35:2: warning: #warning is a language extension [-Wpedantic]
#warning No gettid found, use getpid instead
 ^
projects/openmp/runtime/src/kmp_wrapper_getpid.h:35:2: warning: No gettid found, use getpid instead [-W#warnings]
2 warnings generated.

I added a gettid wrapper that uses FreeBSD's pthread_getthreadid_np(3)
function for this.

Another problem occurred while running the regression tests, where the
ompt/misc/interoperability.cpp failed to compile, with:

projects/openmp/runtime/test/ompt/misc/interoperability.cpp:7:10: fatal error: 'alloca.h' file not found
#include <alloca.h>
         ^~~~~~~~~~

Like on NetBSD, alloca(3) is defined in <stdlib.h> instead. Also,
to make this test case link, I had to -lpthread to the command line,
otherwise it would fail with:

ld: /tmp/lit_tmp_o4Emdi/interoperability-ce6e27.o: undefined reference to symbol 'pthread_create@@FBSD_1.0'

[krytarowski]

If we want to depend on pthread, please go for canonical -pthread.

Is this test supported on !POSIX platforms like Windows?

[dim]

In D59451#1431899, @krytarowski wrote:

If we want to depend on pthread, please go for canonical -pthread.

Hm, I copied this from several other tests, which use the same construct, so I don't see why this one should be different. (In fact pthread detection is a can of worms, many configure scripts go through extreme lengths to determine the exact magical incantation needed... but most of the time -lpthread works everywhere, except on Windows of course. :) )

Is this test supported on !POSIX platforms like Windows?

I don't know how to check which tests are run for Windows, and which ones aren't. Can you enlighten me?

There don't seem to be many Windows references in the openmp test suite, except for the file runtime/test/omp_testsuite.h which seems to implement a pthread wrapper for Windows. But all tests which require pthreads are linked using either %libomp-compile -lpthread or %libomp-cxx-compile -lpthread, so that's why I copied that pattern.

[krytarowski]

In D59451#1431917, @dim wrote:

In D59451#1431899, @krytarowski wrote:

If we want to depend on pthread, please go for canonical -pthread.

Hm, I copied this from several other tests, which use the same construct, so I don't see why this one should be different. (In fact pthread detection is a can of worms, many configure scripts go through extreme lengths to determine the exact magical incantation needed... but most of the time -lpthread works everywhere, except on Windows of course. :) )

NetBSD requires -pthread as it defines -D_REENTRANT that enables reentran APIs. If it works with -lpthread, it's by an accident.

[dim]

Update to use -pthread instead.

[protze.joachim]

This code does not directly use pthreads, but uses C++11 threads.
Why does the compiler not link the necessary threading library, when using C++11 threads? I suggest to define a lit macro like %cxx11_thread which sets the necessary flags. I guess, that on Linux -fopenmp already implies linking the pthread library.

Searching for other use of pthread in the tests, I found

./tasking/bug_proxy_task_dep_waiting.c
./tasking/bug_nested_proxy_task.c
./misc_bugs/omp_foreign_thread_team_reuse.c

These tests use -lpthread (should this also be replaced by -pthread?)
Furthermore, one of them include omp_testsuite.h, which is supposed to provide a Windows compatible implementation of the pthread-create/join functions, but the testcase still links the pthread library. Does anyone actually run the tests on Windows?

[dim]

In D59451#1432201, @protze.joachim wrote:

This code does not directly use pthreads, but uses C++11 threads.
Why does the compiler not link the necessary threading library, when using C++11 threads?

How would the compiler know up-front what is in the code? I have not often seen compilers that influence linking flags from the code that was compiled, unless maybe the Microsoft specific #pragma lib feature (which somebody is now adding to clang, I believe). Usually, these kinds of flags are specified by the user, and passed from the frontend to the linker.

I suggest to define a lit macro like %cxx11_thread which sets the necessary flags. I guess, that on Linux -fopenmp already implies linking the pthread library.

It seems so, there is this code in clang/lib/Driver/ToolChains/Gnu.cpp:

// FIXME: Only pass GompNeedsRT = true for platforms with libgomp that
// require librt. Most modern Linux platforms do, but some may not.
if (addOpenMPRuntime(CmdArgs, ToolChain, Args,
                     JA.isHostOffloading(Action::OFK_OpenMP),
                     /* GompNeedsRT= */ true))
  // OpenMP runtimes implies pthreads when using the GNU toolchain.
  // FIXME: Does this really make sense for all GNU toolchains?
  WantPthread = true;

IMHO this is a little magic, though. Then again, -fopenmp is rather magic anyway. :)

Searching for other use of pthread in the tests, I found

./tasking/bug_proxy_task_dep_waiting.c
./tasking/bug_nested_proxy_task.c
./misc_bugs/omp_foreign_thread_team_reuse.c

These tests use -lpthread (should this also be replaced by -pthread?)

Probably best to use the same flags everywhere.

Furthermore, one of them include omp_testsuite.h, which is supposed to provide a Windows compatible implementation of the pthread-create/join functions, but the testcase still links the pthread library. Does anyone actually run the tests on Windows?

I have no idea, it isn't clear to me where the test suite 'decides' which test cases are run for which operating system.

[Hahnfeld]

In D59451#1432251, @dim wrote:

In D59451#1432201, @protze.joachim wrote:

This code does not directly use pthreads, but uses C++11 threads.
Why does the compiler not link the necessary threading library, when using C++11 threads?

How would the compiler know up-front what is in the code? I have not often seen compilers that influence linking flags from the code that was compiled, unless maybe the Microsoft specific #pragma lib feature (which somebody is now adding to clang, I believe). Usually, these kinds of flags are specified by the user, and passed from the frontend to the linker.

The point is that the test case is using std::thread and the compiler needs to link all libraries that are needed for the stdlib to work. If this includes libpthread that's up to the compiler, so I agree with Joachim that adding linking flags does not seem right.

[dim]

In D59451#1432386, @Hahnfeld wrote:

In D59451#1432251, @dim wrote:

In D59451#1432201, @protze.joachim wrote:

This code does not directly use pthreads, but uses C++11 threads.
Why does the compiler not link the necessary threading library, when using C++11 threads?

How would the compiler know up-front what is in the code? I have not often seen compilers that influence linking flags from the code that was compiled, unless maybe the Microsoft specific #pragma lib feature (which somebody is now adding to clang, I believe). Usually, these kinds of flags are specified by the user, and passed from the frontend to the linker.

The point is that the test case is using std::thread and the compiler needs to link all libraries that are needed for the stdlib to work. If this includes libpthread that's up to the compiler, so I agree with Joachim that adding linking flags does not seem right.

Okay, then I admit to not understanding what you are suggesting. Do we need to start modifying the compiler to make one test case work? That seems rather... excessive.

[krytarowski]

I'm not sure about OMP specifics, but in C++ threads we are allowed to use threading routines but in order to make them functional we must link final executable with libpthread.

On NetBSD threading is in a separate library out of libc.

[Hahnfeld]

In D59451#1432389, @krytarowski wrote:

I'm not sure about OMP specifics, but in C++ threads we are allowed to use threading routines but in order to make them functional we must link final executable with libpthread.

On NetBSD threading is in a separate library out of libc.

So if a developer uses a feature from stdlib, they need to manually link libpthread? I always thought that the C++ standard guarantees you a fully working stdlib out of the box, but might be wrong here.

[Hahnfeld]

In D59451#1432390, @Hahnfeld wrote:

In D59451#1432389, @krytarowski wrote:

I'm not sure about OMP specifics, but in C++ threads we are allowed to use threading routines but in order to make them functional we must link final executable with libpthread.

On NetBSD threading is in a separate library out of libc.

So if a developer uses a feature from stdlib, they need to manually link libpthread? I always thought that the C++ standard guarantees you a fully working stdlib out of the box, but might be wrong here.

Hmm, looks like this used to be true (for example with libstdc++ from GCC 4.8 found in CentOS 7), but it's not with libc++ and newer libstdc++. @EricWF looks like this is expected if both libc++ and (newer) libstdc++ agree?

[krytarowski]

In D59451#1432390, @Hahnfeld wrote:

So if a developer uses a feature from stdlib, they need to manually link libpthread?

Yes (for threads).

[krytarowski]

Hmm, looks like this used to be true (for example with libstdc++ from GCC 4.8 found in CentOS 7), but it's not with libc++ and newer libstdc++.

There is the same behavior retained on NetBSD for at least GCC 6. As far as I'm aware it's the same for libc++ 7.0 and GCC 7 on this OS.

[krytarowski]

it's not with libc++ and newer libstdc++.

Some OSs merged libpthread into libc and/or implemented it almost fully in the kernel. It's not the case on NetBSD and apparently FreeBSD.

In libc there are just stubs for libpthread to allow linking but without defined interfaces, unless someone will link with the POSIX threading library.

[dim]

In D59451#1432417, @krytarowski wrote:

it's not with libc++ and newer libstdc++.

Some OSs merged libpthread into libc and/or implemented it almost fully in the kernel. It's not the case on NetBSD and apparently FreeBSD.

In libc there are just stubs for libpthread to allow linking but without defined interfaces, unless someone will link with the POSIX threading library.

Indeed, at least on FreeBSD there is a desire to fold the full pthread interface into libc, but it's apparently not very easy to do. So for now we need -pthread or -lpthread during linking.

One other thing that could be done is to link libc++.so.1 with -lpthread, but that would make every consumer of libc++ depend on the threading libraries, even if they never use any of the std::thread functionality. At least in our system version of libc++, we've never linked it with the threading libraries, but maybe it's something we should consider. @emaste any thoughts?

[krytarowski]

In D59451#1432542, @dim wrote:

In D59451#1432417, @krytarowski wrote:

it's not with libc++ and newer libstdc++.

Some OSs merged libpthread into libc and/or implemented it almost fully in the kernel. It's not the case on NetBSD and apparently FreeBSD.

In libc there are just stubs for libpthread to allow linking but without defined interfaces, unless someone will link with the POSIX threading library.

Indeed, at least on FreeBSD there is a desire to fold the full pthread interface into libc, but it's apparently not very easy to do. So for now we need -pthread or -lpthread during linking.

One other thing that could be done is to link libc++.so.1 with -lpthread, but that would make every consumer of libc++ depend on the threading libraries, even if they never use any of the std::thread functionality. At least in our system version of libc++, we've never linked it with the threading libraries, but maybe it's something we should consider. @emaste any thoughts?

Even if FreeBSD could merge libc with libpthread, the right thing to do with a threaded c++ code is to pass -pthread to build flags on POSIX platforms.

[dim]

Split off the __kmp_gettid() fix to D59735, the <alloca.h> header fix to D59736. I will rebase this review and update it to add some CMake logic for pthread flag detection.

[dim]

Using CMake's own FindThreads package is obviously the correct solution for finding the right compiler and linker options for enabling threading.

However, we unfortunately have this rather nasty hack in our top-level llvm/trunk/cmake/config-ix.cmake, introduced in rL273302 by @tra :

if(HAVE_LIBPTHREAD)
  # We want to find pthreads library and at the moment we do want to
  # have it reported as '-l<lib>' instead of '-pthread'.
  # TODO: switch to -pthread once the rest of the build system can deal with it.
  set(CMAKE_THREAD_PREFER_PTHREAD TRUE)
  set(THREADS_HAVE_PTHREAD_ARG Off)
  find_package(Threads REQUIRED)
  set(LLVM_PTHREAD_LIB ${CMAKE_THREAD_LIBS_INIT})
endif()

E.g. this prevents the use of CMAKE_THREAD_LIBS_INIT to get the correct -pthread option on systems that require it. I can obviously hack this into OpenMP's own CMake files, but it would probably be nicer to fix it in the top-level LLVM CMake files.

I am unsure whether the problem with the build system as referred to by the remark "switch to -pthread once the rest of the build system can deal with it" is now solved, though. @tra, any idea?

[tra]

I am unsure whether the problem with the build system as referred to by the remark "switch to -pthread once the rest of the build system can deal with it" is now solved, though. @tra, any idea?

I have no idea what the current state of affairs are regarding pthreads, but I suspect things didn't change much -- if it works, nobody bothers to fix it. :-/

[dim]

Readdress the issue in a different way: add -pthread in the
OpenMPTesting.cmake file instead. The top-level LLVM config-ix.cmake file
mangles the value of the needed CMAKE_THREAD_LIBS_INIT variable, so derive the
flag from this, instead of directly using it.

While here, adjust LibompHandleFlags.cmake to add -lm for FreeBSD too, and
add a check for the correct libm.so.5 to LibompMicroTests.cmake.

Finally, remove all the manual pthread flags from the test cases. They can all
just use the %libomp-compile-and-run variable now.

[Hahnfeld]

The code in cmake/OpenMPTesting.cmake should also handle standalone builds of openmp.

cmake/OpenMPTesting.cmake

125–130 ↗

(On Diff #192032)

What if the top-level logic is fixed? I think we should also consider the case that CMAKE_THREAD_LIBS_INIT correctly says -pthread and add the flag accordingly.

[dim]

• Use CMAKE_THREAD_LIBS_INIT verbatim, if it is not "-lpthread".
• Add similar logic to DetectTestCompiler for standalone build.

[dim]

So, any thoughts on the current state of the review?

[krytarowski]

cmake/OpenMPTesting.cmake

129 ↗

(On Diff #192041)

At least in the past in CMake it was better to use list APPEND instead of 2x set for the same variable, because values could cache and not change as expected.

[Hahnfeld]

cmake/DetectTestCompiler/CMakeLists.txt

17–26 ↗

(On Diff #192041)

I don't think this will currently work, did you test a standalone build?

1. CMAKE_THREAD_LIBS_INIT must not depend on OpenMP_Found, it should be added in both cases.
2. As this file is configured separately, I think you need to add find_package(Threads) (or is this done in find_package(OpenMP)?) I guess this also means we shouldn't replace -lpthread by -pthread because we don't need to workaround anything.

cmake/OpenMPTesting.cmake

130 ↗

(On Diff #192041)

else()

[krytarowski]

I think depending on CMAKE_THREAD_LIBS_INIT is the way to go. If it is broken for some platform internally, such OS has more issues than building OpenMP.

OK, looking at the docs the modern approach is to go for:

set(CMAKE_THREAD_PREFER_PTHREAD TRUE)
set(THREADS_PREFER_PTHREAD_FLAG TRUE)
find_package(Threads REQUIRED)

add_executable(test test.cpp)
target_link_libraries(test Threads::Threads)

https://cmake.org/cmake/help/v3.1/module/FindThreads.html

[AndreyChurbanov]

cmake/DetectTestCompiler/CMakeLists.txt

22 ↗

(On Diff #192041)

else()

[dim]

• Only set OPENMP_TEST_COMPILER_OPENMP_FLAGS once.
• For standalone builds, call find_package(Threads) and use the result unconditionally.
• Fix else -> else().

[Hahnfeld]

AFAICS all comments were addressed and this looks good. Please wait a day or so in case I missed something

[daphnediane]

openmp/trunk/cmake/OpenMPTesting.cmake

126 ↗

(On Diff #193550)

This line is missing quotes and breaks cmake if ${CMAKE_THREAD_LIBS_INIT} is empty see https://bugs.llvm.org/show_bug.cgi?id=41401

[Hahnfeld]

openmp/trunk/cmake/OpenMPTesting.cmake

126 ↗

(On Diff #193550)

Yes. After consulting the CMake documentation I think the correct way of doing this in CMake is if(CMAKE_THREAD_LIBS_INIT STREQUAL "-lpthread") (see https://cmake.org/cmake/help/v3.0/command/if.html for variable evaluation). Can you test if that also works for you?

[daphnediane]

openmp/trunk/cmake/OpenMPTesting.cmake

126 ↗

(On Diff #193550)

Tested and it works. That said though that the rest of file uses quoted/prefix "${...}" form with STREQUAL, see lines 137 and 139 below for example. Wasn't sure if consistency was important here. From my own experience with cmake either approach works.

runtime/src/kmp_wrapper_getpid.h

 // On Unix-like systems (Linux* OS and OS X*) getpid() is declared in standard
 // headers.
 #include <sys/syscall.h>
 #include <sys/types.h>
 #include <unistd.h>
 #if KMP_OS_DARWIN
 // OS X
 #define __kmp_gettid() syscall(SYS_thread_selfid)
+#elif KMP_OS_FREEBSD
+#include <pthread_np.h>
+#define __kmp_gettid() pthread_getthreadid_np()
 #elif KMP_OS_NETBSD
 #include <lwp.h>
 #define __kmp_gettid() _lwp_self()
 #elif defined(SYS_gettid)
 // Hopefully other Unix systems define SYS_gettid syscall for getting os thread
 // id
 #define __kmp_gettid() syscall(SYS_gettid)
 #else

runtime/test/ompt/misc/interoperability.cpp

-// RUN: %libomp-cxx-compile-and-run | %sort-threads | FileCheck %s
+// RUN: %libomp-cxx-compile -pthread && %libomp-run | %sort-threads | FileCheck %s
 // REQUIRES: ompt
 
 #include <iostream>
 #include <thread>
-#if !defined(__NetBSD__)
+#if !defined(__FreeBSD__) && !defined(__NetBSD__)
 #include <alloca.h>
 #else
 #include <cstdlib>
 #endif
 
 #include "callback.h"
 #include "omp.h"