Page MenuHomePhabricator

remove CREATE_THREADS:=yes from api/multithreaded Makefile
ClosedPublic

Authored by sbest on Oct 16 2014, 9:02 PM.

Details

Summary

This flag triggers a '-lpthead' during the linking stage. Technically this is not needed since the multithreading is handled by std library, triggered by '-std=c++11' in build command line.

The real reason I want to remove the '-lpthread' is it was causing a (linux/gcc built) test program to hang in a destructor for the std::condition_variable

Diff Detail

Event Timeline

sbest updated this revision to Diff 15063.Oct 16 2014, 9:02 PM
sbest retitled this revision from to remove CREATE_THREADS:=yes from api/multithreaded Makefile.
sbest updated this object.
sbest edited the test plan for this revision. (Show Details)
sbest added a subscriber: Unknown Object (MLST).
emaste added a subscriber: emaste.Oct 17 2014, 5:53 AM

On FreeBSD the test fails to link without -lpthread:

os command: gmake clean LD_EXTRAS='-L/tank/emaste/src/llvm/build-nodebug/bin/../lib/python2.7/site-packages/../.. -llldb' EXE='test_listener_event_description' CFLAGS_EXTRAS='-std=c++11 -stdlib=libc++ -I/tank/emaste/src/llvm/tools/lldb/test/../include' CXX_SOURCES='driver.cpp listener_test.cpp test_listener_event_description.cpp'; gmake ARCH=amd64 CC="/usr/bin/clang" LD_EXTRAS='-L/tank/emaste/src/llvm/build-nodebug/bin/../lib/python2.7/site-packages/../.. -llldb' EXE='test_listener_event_description' CFLAGS_EXTRAS='-std=c++11 -stdlib=libc++ -I/tank/emaste/src/llvm/tools/lldb/test/../include' CXX_SOURCES='driver.cpp listener_test.cpp test_listener_event_description.cpp'
with pid: 92054
stdout: rm -f "test_listener_event_description"  driver.o listener_test.o test_listener_event_description.o driver.d listener_test.d test_listener_event_description.d  
rm -f -r 
rm -rf *.o *.d *.dSYM
/usr/bin/clang++ -std=c++11 -g -O0 -m64  -std=c++11 -stdlib=libc++ -I/tank/emaste/src/llvm/tools/lldb/test/../include -I/tank/emaste/src/llvm/tools/lldb/test/make/../../include   -c -o driver.o driver.cpp
/usr/bin/clang++ -std=c++11 -g -O0 -m64  -std=c++11 -stdlib=libc++ -I/tank/emaste/src/llvm/tools/lldb/test/../include -I/tank/emaste/src/llvm/tools/lldb/test/make/../../include   -c -o listener_test.o listener_test.cpp
/usr/bin/clang++ -std=c++11 -g -O0 -m64  -std=c++11 -stdlib=libc++ -I/tank/emaste/src/llvm/tools/lldb/test/../include -I/tank/emaste/src/llvm/tools/lldb/test/make/../../include   -c -o test_listener_event_description.o test_listener_event_description.cpp
/usr/bin/clang++  driver.o listener_test.o test_listener_event_description.o -g -O0 -m64  -std=c++11 -stdlib=libc++ -I/tank/emaste/src/llvm/tools/lldb/test/../include -I/tank/emaste/src/llvm/tools/lldb/test/make/../../include -L/tank/emaste/src/llvm/build-nodebug/bin/../lib/python2.7/site-packages/../.. -llldb  -o "test_listener_event_description"

stderr: /usr/bin/ld: G: invalid DSO for symbol `pthread_create@@FBSD_1.0' definition
//lib/libthr.so.3: could not read symbols: Bad value
clang++: error: linker command failed with exit code 1 (use -v to see invocation)

What do you think about an ENABLE_STD_THREADS := YES for these tests, and then a system-dependent library addition? My quick Google search suggests linking requirements for std::thread are quite system-dependent; AFAICT some Linuxes also need -lpthread -- perhaps a combination of the specific libc, libpthread, and libc++.

tfiala added a subscriber: tfiala.Oct 17 2014, 7:57 AM

What do you think about an ENABLE_STD_THREADS := YES for these

That seems like a reasonable way to go. And we just work out in the
Makefile.rules what that needs to translate into.

sbest updated this revision to Diff 16188.Nov 13 2014, 6:42 PM

I have added another flag to test/Makefile.rules ENABLE_STD_THREADS and used it the api/Multithreaded unit tests. This can be used to selectively omit '-lpthreads' option for certain systems. All platforms except linux/gcc should behave as before.

I have also made a Bugzilla report for simple program that hangs on linux ( http://llvm.org/bugs/show_bug.cgi?id=21553 )

I see there has been some recent activity in this area with r215562 omitting -lpthreads on windows, and r218899 recently adding -lpthreads as it was needed for FreeBSD.

emaste accepted this revision.Nov 14 2014, 6:59 AM
emaste added a reviewer: emaste.

LGTM

This revision is now accepted and ready to land.Nov 14 2014, 6:59 AM
sbest closed this revision.Nov 14 2014, 11:42 AM
sbest updated this revision to Diff 16232.

Closed by commit rL222031 (authored by @sbest).