Page MenuHomePhabricator

[mips] Include whole lpthread when using both -pthread and -static
AbandonedPublic

Authored by abeserminji on Jan 17 2019, 2:37 AM.

Details

Summary

When executing test-suite tests with -static option, we get stdthreadbug test to fail.
Sometimes it segfault, sometimes it gets stuck in an infinite loop.

The reason for such behavior is that pthread library uses weak symbols for some functions
(ex. __pthread_mutex_lock), and such functions are not included in the executable when
-static option is used. To force the linker to include these functions as well, we use the following
options: -Wl,--whole-archive -lpthread -Wl,--no-whole-archive.

I proposed adding this options to the test-suite for the stdthreadbug test here D52878,
and got a recommendation to try to teach the clang to handle this case by always including
given options when -pthread is used.

This patch does exactly that.

My concern about this is that we are forcing whole pthread into an executable, and user
does not have an option do decide if he wants to use lpthread or something else. Now,
I am not sure what exactly that something else could be, and maybe there isn't something else
and my concerns aren't justified.

Diff Detail

Event Timeline

abeserminji created this revision.Jan 17 2019, 2:37 AM

Now I'm not sure that we really need to make this code more complicated. As to the stdthreadbug test case, maybe we can just exclude this test if LDFLAGS contains the -static option.

As to this patch, is it possible to implement the following algorithm? Will it be more easy and clear?

  1. if there is no -pthread or -pthreads flag, do nothing.
  2. if the flag provided, iterate over -l options (do not introduce new lpthread first-class argument). If there is the -lpthread (with or without whole-archive surrounds), consider a user know what he/she wants to do.
  3. if there is no -lpthread, pass --whole-archive -lpthread --no-whole-archive to the linker.

I am not sure if -static option passed as --cflag to lnt runtest enters the LDFLAGS. I guess it doesn't.
But if there is a way to check which options are passed as --cflags, we might be able to add -Wl, -whole-archive -lpthread -Wl -no-whole-archive only in case when static is used.
That would be my preferable option in this situation, but I couldn't find any clues on how to do that.

It is possible to modify an algorithm in such way. Shouldn't differ too much.

I am not sure if -static option passed as --cflag to lnt runtest enters the LDFLAGS. I guess it doesn't.
But if there is a way to check which options are passed as --cflags, we might be able to add -Wl, -whole-archive -lpthread -Wl -no-whole-archive only in case when static is used.
That would be my preferable option in this situation, but I couldn't find any clues on how to do that.

You are right, LDFLAGS does not contain the -static. But if you pass linker options to the CMake by the CMAKE_EXE_LINKER_FLAGS variable, the following code should help.

--- a/SingleSource/UnitTests/C++11/CMakeLists.txt
+++ b/SingleSource/UnitTests/C++11/CMakeLists.txt
@@ -1,3 +1,9 @@
 list(APPEND CXXFLAGS -std=c++11 -pthread)
 list(APPEND LDFLAGS -lstdc++ -pthread)
+
+file(GLOB Source RELATIVE ${CMAKE_CURRENT_SOURCE_DIR} *.c *.cpp)
+if(${CMAKE_EXE_LINKER_FLAGS} MATCHES "-static")
+  list(REMOVE_ITEM Source stdthreadbug.cpp)
+endif()
+
 llvm_singlesource()

@atanasyan That looks good. I tried to make an equivalent for the Makefile

--- SingleSource/UnitTests/C++11/Makefile	(revision 351547)
+++ SingleSource/UnitTests/C++11/Makefile	(working copy)
@@ -4,6 +4,11 @@
 
 CPPFLAGS += -std=c++11 -pthread
 
+ifeq (-static, $(findstring -static, $(TARGET_FLAGS)))
+#PROGRAMS_TO_SKIP += stdthreadbug
+LDFLAGS += -Wl,--whole-archive -lpthread -Wl,--no-whole-archive
+endif
+
 ifdef BENCHMARKING_ONLY
 PROGRAMS_TO_SKIP += stdthreadbug
 endif

As an option, we can provide necessary flags for the test to finish successfully instead of not testing it.

As an option, we can provide necessary flags for the test to finish successfully instead of not testing it.

Agreed.

abeserminji abandoned this revision.Thu, Jan 24, 2:19 AM

Agreed on this solution D52878.