This is an archive of the discontinued LLVM Phabricator instance.

[build] Link main executable with libpthread
ClosedPublic

Authored by tra on Jun 17 2016, 1:12 PM.

Details

Summary

Otherwise it gets linked in by one of the dependencies of shared
libraries which may be too late and we end up with weird crashes in
std::call_once().

Diff Detail

Event Timeline

tra updated this revision to Diff 61123.Jun 17 2016, 1:12 PM
tra retitled this revision from to [build] Make sure to link main executable with libpthread.
tra updated this object.
tra added reviewers: rafael, MatzeB, chandlerc.
tra added a subscriber: llvm-commits.
rafael edited edge metadata.Jun 17 2016, 1:21 PM
rafael added a subscriber: rafael.

Is that a documented restriction?

We should have a comment saying why we need to do this.

Cheers,
Rafael

tra added a comment.Jun 17 2016, 1:36 PM

Is that a documented restriction?

I couldn't find it stated explicitly anywhere. "add -pthread (no 'l' after dash) and magic will happen" is pretty much it.

On the other hand, it sort of makes sense.
libpthread overrides number of weak symbols in standard c library and we do want *all* symbols in our executable either use all those symbols from standard library or use all of them from libpthread. With shared liking (and lazy library loading and symbol binding?) we may end up with some symbols getting resolved from libc which is already loaded and available and others from libpthread when it gets loaded by some other shared library.

We should have a comment saying why we need to do this.

OK

tra updated this revision to Diff 61129.Jun 17 2016, 1:54 PM
tra edited edge metadata.

Added comments.

tra retitled this revision from [build] Make sure to link main executable with libpthread to [build] Link main executable with libpthread.Jun 17 2016, 3:09 PM

This is fine by me. Adding Chris to check if this is the right cmake way to do it.

beanz accepted this revision.Jun 20 2016, 2:47 PM
beanz edited edge metadata.

One comment below that needs attention. Otherwise LGTM.

cmake/config-ix.cmake
114

I'm a little concerned about the use of CMAKE_THREAD_LIBS_INIT here. That isn't guaranteed to be pthreads even if we have libpthread. On systems with other thread libraries I believe it prefers the system native library.

I think if you set CMAKE_THREAD_PREFER_PTHREAD that will force it to prefer pthreads over a system threading library.

This revision is now accepted and ready to land.Jun 20 2016, 2:47 PM
tra added inline comments.Jun 20 2016, 2:57 PM
cmake/config-ix.cmake
114

Will do.

tra added a comment.Jun 20 2016, 3:28 PM

Argh. CMAKE_THREAD_LIBS_INIT contains library name prefixed with -l. It's fine to use with target_link_libraries because it checks for prefixed names, but results in "-l-lpthread" because in some places we assume that PTHREAD_LIB is an unprefixed library name.
I'll update the patch once I know how to deal with this.

tra updated this revision to Diff 61316.Jun 20 2016, 4:06 PM
tra edited edge metadata.

Strip -l prefix from the library name in PTHREAD_LIB.

tra updated this revision to Diff 61320.Jun 20 2016, 4:17 PM

Oops. Removed debugging message that should not be committed.

beanz added inline comments.Jun 21 2016, 10:51 AM
cmake/config-ix.cmake
115

Rather than this, try setting THREADS_PREFER_PTHREAD_FLAG. That will prefer the -pthreds flag if the compiler supports it.

Even with that you can't actually assume that the flag returned here will be -pthread instead of -lpthread because I'm pretty sure we support using pthreads library with compilers that don't support -pthread.

cmake/modules/AddLLVM.cmake
677

Since this would be a flag rather than a library, maybe you should change your calls to target_link_libraries to something more like this:

set_property(TARGET ${name} APPEND_STRING PROPERTY
             LINK_FLAGS "${PTHREAD_LIB}")

I'd also suggest changing PTHREAD_LIB to PTHREAD_FLAG because that is more representative of what you're actually using here.

tra added inline comments.Jun 21 2016, 11:01 AM
cmake/config-ix.cmake
115

That would need more changes. Currently we add PTHREAD_LIB to system_libs, which eventually ends up in LLVM_SYSTEM_LIBS property which is in turn used by llvm-config for figuring out what to print for --system-libs. The way it currently does that by prepending everything in LLVM_SYSTEM_LIBS with '-l' and that breaks with both -lpthread and -pthread.

I guess we need to check for prefixed names there and only append -l to items without leading dash. Stay tuned for updated patch.

beanz added inline comments.Jun 21 2016, 11:25 AM
cmake/config-ix.cmake
115

I didn't think about the extra complication, if we want to force -lpthread to be the return here to be consistent you can set THREADS_HAVE_PTHREAD_ARG=Off. That should make this safer, and you can save the added complexity of supporting -pthread for another time.

Thoughts?

tra updated this revision to Diff 61412.Jun 21 2016, 11:43 AM

Force pthread detection to report it as -l<library>
Deal with -l<library> in llvm-config.

tra added inline comments.Jun 21 2016, 11:49 AM
cmake/config-ix.cmake
115

OK. We'll stick with -l<pthread variant> for now.

LGTM.

Thanks!
-Chris

This revision was automatically updated to reflect the committed changes.
amadio added a subscriber: amadio.Nov 23 2017, 9:10 AM
amadio added inline comments.
llvm/trunk/tools/llvm-config/CMakeLists.txt
22 ↗(On Diff #61427)

When building against system ZLIB, ${l} here resolves to /usr/lib/libz.so, which then causes the error below:

/usr/lib/gcc/x86_64-pc-linux-gnu/6.4.0/../../../../x86_64-pc-linux-gnu/bin/ld: cannot find -l/usr/lib/libz.so
collect2: error: ld returned 1 exit status

The symptom is visible in tools/llvm-config/BuildVariables.inc, where I see the following line:

#define LLVM_SYSTEM_LIBS "-lrt -ldl -lcurses -lpthread -l/usr/lib/libz.so -lm"

The correct way to link against zlib since CMake 3.1 is to use the target ZLIB::ZLIB defined in FindZLIB.cmake that comes with CMake.

The following caused error while building LLVM on riscv64 system. I think, you must use -pthread and never use -lpthread. A few points:

  • -pthread also modified CPP flags and defines _REENTRANT, which is required by standard;
  • Based on above -pthread also needs to be in your CPP/CXX FLAGS;
  • On riscv64 -pthread also implies libatomic (might change in the future).

TL;DR -pthread needs to be used and needs to be passed to compile and link flags.

espindola edited reviewers, added: espindola; removed: rafael.Apr 5 2018, 6:34 PM