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

[abeserminji]

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.

[atanasyan]

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.

[abeserminji]

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.

[atanasyan]

In D56836#1361528, @abeserminji wrote:

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()

[abeserminji]

@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.

[atanasyan]

In D56836#1362967, @abeserminji wrote:

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

Agreed.

[abeserminji]

Agreed on this solution D52878.

include/clang/Driver/Options.td

def print_resource_dir : Flag<["-", "--"], "print-resource-dir">,
   HelpText<"Print the resource directory pathname">;
 def print_search_dirs : Flag<["-", "--"], "print-search-dirs">,
   HelpText<"Print the paths used for finding libraries and programs">;
 def private__bundle : Flag<["-"], "private_bundle">;
 def pthreads : Flag<["-"], "pthreads">;
 def pthread : Flag<["-"], "pthread">, Flags<[CC1Option]>,
   HelpText<"Support POSIX threads in generated code">;
 def no_pthread : Flag<["-"], "no-pthread">, Flags<[CC1Option]>;
+def lpthread : Flag<["-"], "lpthread">;
 def p : Flag<["-"], "p">;
 def pie : Flag<["-"], "pie">;
 def read__only__relocs : Separate<["-"], "read_only_relocs">;
 def remap : Flag<["-"], "remap">;
 def rewrite_objc : Flag<["-"], "rewrite-objc">, Flags<[DriverOption,CC1Option]>,
   HelpText<"Rewrite Objective-C source to C++">, Group<Action_Group>;
 def rewrite_legacy_objc : Flag<["-"], "rewrite-legacy-objc">, Flags<[DriverOption]>,
   HelpText<"Rewrite Legacy Objective-C source to C++">;
def v : Flag<["-"], "v">, Flags<[CC1Option, CoreOption]>,
   HelpText<"Show commands to run and use verbose output">;
 def verify_debug_info : Flag<["--"], "verify-debug-info">, Flags<[DriverOption]>,
   HelpText<"Verify the binary representation of debug output">;
 def weak_l : Joined<["-"], "weak-l">, Flags<[LinkerInput]>;
 def weak__framework : Separate<["-"], "weak_framework">, Flags<[LinkerInput]>;
 def weak__library : Separate<["-"], "weak_library">, Flags<[LinkerInput]>;
 def weak__reference__mismatches : Separate<["-"], "weak_reference_mismatches">;
 def whatsloaded : Flag<["-"], "whatsloaded">;
+def whole_archive : Flag<["--"], "whole-archive">, Group<Link_Group>;
+def no_whole_archive : Flag<["--"], "no-whole-archive">, Group<Link_Group>;
 def whyload : Flag<["-"], "whyload">;
 def w : Flag<["-"], "w">, HelpText<"Suppress all warnings">, Flags<[CC1Option]>;
 def x : JoinedOrSeparate<["-"], "x">, Flags<[DriverOption,CC1Option]>,
   HelpText<"Treat subsequent input files as having type <language>">,
   MetaVarName<"<language>">;
 def y : Joined<["-"], "y">;
 
 def fintegrated_as : Flag<["-"], "fintegrated-as">, Flags<[DriverOption]>,

lib/Driver/ToolChains/Gnu.cpp

if (!Args.hasArg(options::OPT_nodefaultlibs)) {
       if (NeedsSanitizerDeps)
         linkSanitizerRuntimeDeps(ToolChain, CmdArgs);
 
       if (NeedsXRayDeps)
         linkXRayRuntimeDeps(ToolChain, CmdArgs);
 
       bool WantPthread = Args.hasArg(options::OPT_pthread) ||
                          Args.hasArg(options::OPT_pthreads);
+      bool WantLPthread = Args.hasArg(options::OPT_lpthread);
 
       // 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;
 
       AddRunTimeLibs(ToolChain, D, CmdArgs, Args);
 
-      if (WantPthread && !isAndroid)
+      if ((WantPthread || WantLPthread) && !isAndroid) {
+        if (Triple.isMIPS() && Args.hasArg(options::OPT_static)) {
+          // When -pthread option is used in combination with -static,
+          // we want to avoid problems which weak symbols may cause,
+          // and therefore we include whole lpthread library
+          bool wholeArchiveFlag = false;
+          bool lpthreadAsWholeArchive = false;
+          for (Arg *A : Args) {
+            if (A->getOption().matches(options::OPT_whole_archive))
+              wholeArchiveFlag = true;
+            if (A->getOption().matches(options::OPT_no_whole_archive))
+              wholeArchiveFlag = false;
+            if (wholeArchiveFlag) {
+              if (A->getOption().matches(options::OPT_lpthread)) {
+                lpthreadAsWholeArchive = true;
+                break;
+              }
+            }
+          }
+
+          if (!lpthreadAsWholeArchive) {
+            CmdArgs.push_back("--whole-archive");
             CmdArgs.push_back("-lpthread");
+            CmdArgs.push_back("--no-whole-archive");
+          }
+        } else {
+          CmdArgs.push_back("-lpthread");
+        }
+      }
 
       if (Args.hasArg(options::OPT_fsplit_stack))
         CmdArgs.push_back("--wrap=pthread_create");
 
       CmdArgs.push_back("-lc");
 
       // Add IAMCU specific libs, if needed.
       if (IsIAMCU)