Page MenuHomePhabricator

Fix gettid warnings and one test on FreeBSD
ClosedPublic

Authored by dim on Mar 16 2019, 4:53 AM.

Details

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'

Diff Detail

Repository
rL LLVM

Event Timeline

dim created this revision.Mar 16 2019, 4:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2019, 4:53 AM
Herald added subscribers: jdoerfert, jfb. · View Herald Transcript

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

Is this test supported on !POSIX platforms like Windows?

dim added a comment.Mar 16 2019, 5:38 AM

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.

In D59451#1431917, @dim 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 updated this revision to Diff 190963.Mar 16 2019, 8:24 AM

Update to use -pthread instead.

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 added a comment.Mar 16 2019, 12:59 PM

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.

In D59451#1432251, @dim 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 added a comment.Mar 17 2019, 4:39 AM
In D59451#1432251, @dim 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.

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.

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.

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 added a comment.EditedMar 17 2019, 5:53 AM

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

Yes (for threads).

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.

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 added a comment.Mar 17 2019, 10:56 AM
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?

In D59451#1432542, @dim 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 added a comment.Mar 23 2019, 7:45 AM

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 added a subscriber: tra.Mar 23 2019, 2:00 PM

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 added a comment.Mar 23 2019, 8:04 PM

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 updated this revision to Diff 192032.Mar 24 2019, 8:00 AM

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 requested changes to this revision.Mar 24 2019, 9:49 AM

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.

This revision now requires changes to proceed.Mar 24 2019, 9:49 AM
dim updated this revision to Diff 192041.Mar 24 2019, 11:53 AM
  • Use CMAKE_THREAD_LIBS_INIT verbatim, if it is not "-lpthread".
  • Add similar logic to DetectTestCompiler for standalone build.
dim added a comment.Apr 1 2019, 10:41 AM

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

krytarowski added inline comments.Apr 2 2019, 12:35 AM
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 added inline comments.Apr 2 2019, 5:30 AM
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 added a comment.EditedApr 2 2019, 6:09 AM

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 added inline comments.
cmake/DetectTestCompiler/CMakeLists.txt
22 ↗(On Diff #192041)

else()

dim updated this revision to Diff 193343.Apr 2 2019, 12:34 PM
  • Only set OPENMP_TEST_COMPILER_OPENMP_FLAGS once.
  • For standalone builds, call find_package(Threads) and use the result unconditionally.
  • Fix else -> else().
Hahnfeld accepted this revision.Apr 2 2019, 2:02 PM

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

This revision is now accepted and ready to land.Apr 2 2019, 2:02 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2019, 11:10 AM
daphnediane added inline comments.
openmp/trunk/cmake/OpenMPTesting.cmake
126

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 added inline comments.Apr 5 2019, 2:13 PM
openmp/trunk/cmake/OpenMPTesting.cmake
126

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 added inline comments.Apr 5 2019, 2:30 PM
openmp/trunk/cmake/OpenMPTesting.cmake
126

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.