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

[Aleksandar Beserminji via Phabricator via cfe-commits]

abeserminji created this revision.
abeserminji added reviewers: petarj, atanasyan, MatzeB, mstojanovic.
Herald added subscribers: jfb, arichardson, sdardis, srhines.

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 <https://reviews.llvm.org/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.


Repository:
  rC Clang

https://reviews.llvm.org/D56836

Files:
  include/clang/Driver/Options.td
  lib/Driver/ToolChains/Gnu.cpp


Index: lib/Driver/ToolChains/Gnu.cpp
===================================================================
--- lib/Driver/ToolChains/Gnu.cpp
+++ lib/Driver/ToolChains/Gnu.cpp
@@ -501,6 +501,7 @@
 
       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.
@@ -513,8 +514,35 @@
 
       AddRunTimeLibs(ToolChain, D, CmdArgs, Args);
 
-      if (WantPthread && !isAndroid)
-        CmdArgs.push_back("-lpthread");
+      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");
Index: include/clang/Driver/Options.td
===================================================================
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -2491,6 +2491,7 @@
 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">;
@@ -2602,6 +2603,8 @@
 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]>,


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D56836.182229.patch
Type: text/x-patch
Size: 3095 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190117/128ecbe7/attachment.bin>